[manila-csi-plugin] snapshot caps check #1592
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
size/L
|
Build succeeded.
|
|
Build succeeded.
|
pkg/csi/manila/controllerserver.go
Outdated
|
|
||
| for _, c := range requiredShareTypeCaps { | ||
| if !shareTypeCaps[c] { | ||
| return nil, status.Errorf(codes.InvalidArgument, |
There was a problem hiding this comment.
The main objective here is to ensure that it will be possible to create a new volume with this snapshot as its source and I'm not sure I'm going the right way about this.
Is it possible to create a share from a snapshot, but using a share type that's different than what was used to create the original share? If so, then checking share type caps here doesn't make much sense -- because maybe one type had create_share_from_snapshot_support disabled, but the other may have it enabled and share creation would succeed.
From my tests, it seems the caps on the share (not on the share type) seemed to have the final say. Even if I tried to create a share from snapshot with a type that had create_share_from_snapshot_support=True, Manila would refuse to create it if the original share reported create_share_from_snapshot_support=False. Unfortunately, there's no way for manila-csi to know at the moment. The gophercloud lib is currently missing create_share_from_snapshot_support field in shares.Share struct. I'll push a patch there. I'm not entirely sure we should merge this PR without it.
There was a problem hiding this comment.
There was a problem hiding this comment.
@gman0: no you can't specify both snapshot and share type when creating new shares; new shares from snapshots are expected to retain the share type of the parent;
and yes, the capability of the share is the best place to look since share type extra specs are mutable. While share type extra-specs are mutable, they're assumed to not change often and manila administrators know that changes aren't going to apply to resources that are already provisioned. However, i totally agree with not relying on the share type extra-specs.
your proposed way of just checking the share's "snapshot_support" and "create_share_from_snapshot_support" seems correct.
There was a problem hiding this comment.
@gouthampacha thank you for explanation and suggestions! I'll fix this like we've agreed after the create_share_from_snapshot_support patch in gophercloud is in.
|
PTAL @tombarron @gouthampacha |
|
Build succeeded.
|
pkg/csi/manila/controllerserver.go
Outdated
|
|
||
| for _, c := range requiredShareTypeCaps { | ||
| if !shareTypeCaps[c] { | ||
| return nil, status.Errorf(codes.InvalidArgument, |
There was a problem hiding this comment.
@gman0: no you can't specify both snapshot and share type when creating new shares; new shares from snapshots are expected to retain the share type of the parent;
and yes, the capability of the share is the best place to look since share type extra specs are mutable. While share type extra-specs are mutable, they're assumed to not change often and manila administrators know that changes aren't going to apply to resources that are already provisioned. However, i totally agree with not relying on the share type extra-specs.
your proposed way of just checking the share's "snapshot_support" and "create_share_from_snapshot_support" seems correct.
| @@ -75,7 +90,7 @@ func getOrCreateShare(shareName string, createOpts *shares.CreateOpts, manilaCli | |||
| return share, 0, nil | |||
| } | |||
|
|
|||
| return waitForShareStatus(share.ID, shareCreating, shareAvailable, false, manilaClient) | |||
| return waitForShareStatus(share.ID, []string{shareCreating, shareCreatingFromSnapshot}, shareAvailable, false, manilaClient) | |||
There was a problem hiding this comment.
Yeah, this looks right. (@gouthampacha reminded me yesterday)
There was a problem hiding this comment.
Thank you both! I'll update this PR once I find out how do we go about updating the gophercloud lib dependency.
|
Do we wait for new release of gophercloud or is it ok if I pin the gophercloud dependency to a particular commit @lingxiankong @ramineni (see #1592 (comment) )? I'd like this feature to still make it for v1.22 if possible |
@gman0 we can pin to particular commit , I dont see any issue in that. |
|
Thanks @ramineni . I just asked Gophercloud's Joe Topjian to find out whether v0.19.0 is planned still this month. It seems releases are 1 month apart, and the last one was 11th of June -- so maybe the next one is coming very soon? If it's not soon enough I'll pin the dependency to the commit from the PR linked earlier. |
…share This commit removes all hard-coded checks for CephFS shares, preventing them from being snapshotted. Instead, manila-csi now checks for snapshot_support and create_share_from_snapshot capabilities.
gman0
force-pushed
the
manilacsi-snapshot-caps-check
branch
from
89e7d55
to
b54ea0e
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
/test openlab/cloud-provider-openstack-e2e-test-csi-cinder |
|
@gman0: The specified target(s) for
Use In response to this:
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/test-infra repository. |
|
/test openlab/cloud-provider-openstack-multinode-csi-migration-test |
|
@gman0: The specified target(s) for
Use In response to this:
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/test-infra repository. |
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@gman0: The specified target(s) for
Use In response to this:
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/test-infra repository. |
|
/test cloud-provider-openstack-multinode-csi-migration-test |
|
@gman0: The specified target(s) for
Use In response to this:
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/test-infra repository. |
|
Jobs |
|
I've updated the caps checks like we've agreed. @tombarron @gouthampacha could you please take a look? @ramineni I'm not sure about the failing jobs. I think I've seen these ones failing in other PRs too... Is that a blocker for this PR? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ramineni, tombarron 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 |
k8s-ci-robot
added
the
approved
|
Thank you all for feedback! @ramineni your approval didn't trigger a merge so I guess that will happen once CI passes? |
|
@gman0 looks like gate resolution might take some time. Only change is gophercloud update which would effect the cinder plugin . I hope no breaking changes which would effect the plugin . Though looks safe to me to merge, If you have setup, could you quickly run e2e on cinder csi manually , so that we can override and go ahead with the merge. |
|
I manually deployed
Clean up was successful too. |
|
@gman0 Thanks. |
|
/override openlab/cloud-provider-openstack-e2e-test-csi-cinder |
|
@ramineni: Overrode contexts on behalf of ramineni: openlab/cloud-provider-openstack-e2e-test-csi-cinder In response to this:
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/test-infra repository. |
|
/override openlab/cloud-provider-openstack-multinode-csi-migration-test |
|
@ramineni: Overrode contexts on behalf of ramineni: openlab/cloud-provider-openstack-multinode-csi-migration-test In response to this:
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/test-infra repository. |
* go.mod: bumped github.com/gophercloud/gophercloud to v0.19.0 * removed hardcoded checks for cephfs, added checks for caps in parent share This commit removes all hard-coded checks for CephFS shares, preventing them from being snapshotted. Instead, manila-csi now checks for snapshot_support and create_share_from_snapshot capabilities. * share: added handling for creating_from_snapshot status * docs: added a note about snapshots * go.sum: added k8s.io/code-generator * go.mod fix 2: added github.com/gophercloud/gophercloud v0.19.0
* go.mod: bumped github.com/gophercloud/gophercloud to v0.19.0 * removed hardcoded checks for cephfs, added checks for caps in parent share This commit removes all hard-coded checks for CephFS shares, preventing them from being snapshotted. Instead, manila-csi now checks for snapshot_support and create_share_from_snapshot capabilities. * share: added handling for creating_from_snapshot status * docs: added a note about snapshots * go.sum: added k8s.io/code-generator * go.mod fix 2: added github.com/gophercloud/gophercloud v0.19.0
What this PR does / why we need it:
This PR removes hard-coded checks for CephFS shares, preventing them from being snapshotted. Instead, share type capabilities are checked and creating snapshots is accepted or denied based on that.
Which issue this PR fixes(if applicable):
fixes #720
We've abandoned the original idea described in #720. Since Manila supports snapshot restoration with CephFS backends since the Wallaby release, we're going with that instead.
Release note: