Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OCPBUGS-45069] Add a check for manila share type #205

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

itzikb-redhat
Copy link
Contributor

If the share type extra spec of driver_handles_share_servers is true we skip the test.

@openshift-ci openshift-ci bot requested review from EmilienM and mdbooth December 1, 2024 14:38
@itzikb-redhat
Copy link
Contributor Author

/retest

@MiguelCarpio
Copy link

/test

Copy link
Contributor

openshift-ci bot commented Dec 2, 2024

@MiguelCarpio: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test build
/test images
/test test
/test verify

The following commands are available to trigger optional jobs:

/test e2e-openstack-ccpmso
/test e2e-openstack-ccpmso-zone
/test e2e-openstack-dualstack
/test e2e-openstack-dualstack-v6primary

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-openstack-test-main-build
pull-ci-openshift-openstack-test-main-images
pull-ci-openshift-openstack-test-main-test
pull-ci-openshift-openstack-test-main-verify

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@MiguelCarpio
Copy link

/test e2e-openstack-ovn-parallel

Copy link
Contributor

openshift-ci bot commented Dec 2, 2024

@MiguelCarpio: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test images
/test test
/test verify

The following commands are available to trigger optional jobs:

/test e2e-openstack-ccpmso
/test e2e-openstack-ccpmso-zone
/test e2e-openstack-dualstack
/test e2e-openstack-dualstack-v6primary

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-openstack-test-main-build
pull-ci-openshift-openstack-test-main-images
pull-ci-openshift-openstack-test-main-test
pull-ci-openshift-openstack-test-main-verify

In response to this:

/test e2e-openstack-ovn-parallel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@MiguelCarpio
Copy link

/test e2e-openstack-ccpmso

@rlobillo
Copy link
Contributor

rlobillo commented Dec 2, 2024

If the share type extra spec of driver_handles_share_servers is true we skip the test.

Hi @itzikb-redhat.

Could you please elaborate why having driver_handles_share_servers set to false is a requirement? Do you know if that is documented somewhere? I took a look at the limitations and couldn't find it:

https://docs.openshift.com/container-platform/4.17/storage/container_storage_interface/persistent-storage-csi-manila.html#persistent-storage-csi-manila-limitations_persistent-storage-csi-manila

Maybe should be added there?

@MiguelCarpio
Copy link

Hi @rlobillo @itzikb-redhat

The test should validate if the driver_handles_share_servers is set to true or false, and in that way use the share-network or not.

  • true --> Get the share-network (or create it and add it to the compute nodes) and use it to create the manila-pvc
  • false --> Create the manila-pvc without a share-network

You can find in the following documentation the requirements and what is supported to create a manila-pvc (or share):

<share_network>: The name of the share network:

  • Required if the share type has driver_handles_share_servers set to true.
  • Unsupported if the share type has driver_handles_share_servers set to false.
  • Unsupported for CephFS-NFS and native CephFS. These protocols do not support share types that have driver_handles_share_servers set to true.

If this test is oriented to create a manila-pvc not using a share-network, it should skip when finding the driver_handles_share_servers is set to true.

@rlobillo
Copy link
Contributor

rlobillo commented Dec 2, 2024

I wonder if we should create a separate test then covering the DHSS=true case.

@MiguelCarpio
Copy link

Hi @rlobillo @itzikb-redhat
The test should validate if the driver_handles_share_servers is set to true or false, and in that way use the share-network or not.

  • true --> Get the share-network (or create it and add it to the compute nodes) and use it to create the manila-pvc
  • false --> Create the manila-pvc without a share-network

Thanks @MiguelCarpio! I still have my concerns about this. In our CI we are using below share type:

$ openstack share type show default
+----------------------+--------------------------------------+
| Field                | Value                                |
+----------------------+--------------------------------------+
| id                   | c0b38029-e7ab-48c3-bc2a-522237fcf89d |
| name                 | default                              |
| visibility           | public                               |
| is_default           | True                                 |
| required_extra_specs | driver_handles_share_servers : False |
| optional_extra_specs | snapshot_support : True              |
| description          | None                                 |
+----------------------+--------------------------------------+

and the test is passing. I believe that feature "driver_handles_share_servers" (also known as DHSS) is for instructing the backend to use different set of storage servers. We are not testing that AFAIK.

Yes, it passes because with driver_handles_share_servers set to false doesn't need a share-network to create a manila-pvc

@MiguelCarpio
Copy link

/test e2e-openstack-ccpmso

@MiguelCarpio
Copy link

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024
@rlobillo
Copy link
Contributor

rlobillo commented Dec 3, 2024

@itzikb-redhat I think it would be good to have a comment in the test itself with a link to the doc bug that explains this limitation:

https://issues.redhat.com/browse/OCPBUGS-45320

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2024
If the share type extra spec of driver_handles_share_servers is true we skip the test.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024
@rlobillo
Copy link
Contributor

rlobillo commented Dec 3, 2024

/unhold
/lgtm
/approve

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 3, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2024
Copy link
Contributor

openshift-ci bot commented Dec 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MiguelCarpio, rlobillo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2024
@MiguelCarpio
Copy link

/test test

Copy link
Contributor

openshift-ci bot commented Dec 3, 2024

@itzikb-redhat: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit e61b58d into openshift:main Dec 3, 2024
5 checks passed
@EmilienM
Copy link
Member

EmilienM commented Dec 3, 2024

We should backport it.

@EmilienM
Copy link
Member

EmilienM commented Dec 3, 2024

/cherry-pick release-4.18 release-4.17 release-4.16

@openshift-cherrypick-robot

@EmilienM: new pull request created: #206

In response to this:

/cherry-pick release-4.18 release-4.17 release-4.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@MiguelCarpio
Copy link

/cherry-pick release-4.15

@openshift-cherrypick-robot

@MiguelCarpio: #205 failed to apply on top of branch "release-4.15":

Applying: Add a check for manila share type
Using index info to reconstruct a base tree...
M	test/extended/openstack/utils.go
M	test/extended/openstack/volumes.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging test/extended/openstack/volumes.go
CONFLICT (content): Merge conflict in test/extended/openstack/volumes.go
Auto-merging test/extended/openstack/utils.go
CONFLICT (content): Merge conflict in test/extended/openstack/utils.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add a check for manila share type

In response to this:

/cherry-pick release-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@MiguelCarpio
Copy link

/cherry-pick release-4.15

@openshift-cherrypick-robot

@MiguelCarpio: #205 failed to apply on top of branch "release-4.15":

Applying: Add a check for manila share type
Using index info to reconstruct a base tree...
M	test/extended/openstack/utils.go
M	test/extended/openstack/volumes.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging test/extended/openstack/volumes.go
CONFLICT (content): Merge conflict in test/extended/openstack/volumes.go
Auto-merging test/extended/openstack/utils.go
CONFLICT (content): Merge conflict in test/extended/openstack/utils.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Add a check for manila share type

In response to this:

/cherry-pick release-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants