[cinder-csi-plugin] add cluster id to snapshot metadata #1544
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/XS
Fedosin
force-pushed
the
snapshot_metadata_clusterid
branch
from
c6d66ad
to
5a7b499
Compare
k8s-ci-robot
added
size/S
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
| // TODO: Delegate the check to openstack itself and ignore the conflict | ||
| snap, err = cs.Cloud.CreateSnapshot(name, volumeId, &req.Parameters) | ||
| snap, err = cs.Cloud.CreateSnapshot(name, volumeId, &properties) |
There was a problem hiding this comment.
this is not related to this PR
but seems CreateSnapshot and CreateVolume take req.Parameters in different manner
CreateVolume doesn't contain req.parameter into tags and in turn put into metadata...
https://github.com/container-storage-interface/spec/blob/master/spec.md
says param
// Plugin specific parameters passed in as opaque key-value pairs.
// This field is OPTIONAL. The Plugin is responsible for parsing and
// validating these parameters. COs will treat these as opaque.
// Use cases for opaque parameters:
// - Specify a policy to automatically clean up the snapshot.
// - Specify an expiration date for the snapshot.
// - Specify whether the snapshot is readonly or read/write.
// - Specify if the snapshot should be replicated to some place.
// - Specify primary or secondary for replication systems that
// support snapshotting only on primary.
so we should re-consider this to avoid put all those param into metadata or vice versa (line 107 above) , make all param to volume req ..
There was a problem hiding this comment.
Yes, that's what I was going to ask... As you said, for volumes we sanitize req parameters, but for snapshots we put them all in metadata.
I didn't modify this behavior in my PR to support backward compatibility, but I agree that we need to reconsider what we actually put there.
Now cluster id is set by --cluster argument at the driver's start. When it's set, cinder csi controller appends volumes metadata with the cluster id. Unfortunatelly it doesn't happen for volume snapshots, which makes it harder to find them after cluster deletion. This commit adds cluster id to snapshots metadata to unify the behavior with volumes.
Fedosin
force-pushed
the
snapshot_metadata_clusterid
branch
from
5a7b499
to
71da4d1
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build failed.
|
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@Fedosin: 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. |
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@Fedosin: 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. |
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@Fedosin: 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. |
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@Fedosin: 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. |
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@jichenjc: 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 |
|
@jichenjc: 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. |
|
Build succeeded.
|
|
Build failed.
|
|
/test cloud-provider-openstack-e2e-test-csi-cinder |
|
@Fedosin: 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. |
|
Build succeeded.
|
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ramineni 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
) Now cluster id is set by --cluster argument at the driver's start. When it's set, cinder csi controller appends volumes metadata with the cluster id. Unfortunatelly it doesn't happen for volume snapshots, which makes it harder to find them after cluster deletion. This commit adds cluster id to snapshots metadata to unify the behavior with volumes.
This reverts commit a272e59. As noted in the corresponding bug [1], the expectation was always to revert this once cloud-provided-openstack started providing cluster ID information in snapshot metadata, which has been the case for some time now [2]. Conflicts: pkg/destroy/openstack/openstack.go Changes: pkg/destroy/openstack/openstack.go NOTE(stephenfin): Conflicts are due to commit 375fe6f ("OpenStack: Optimize cluster deletion") which changed two blocks so that we now continue in a loop rather than returning early. We introduce this same logic into a newly restored block inside 'deleteSnapshots' to prevent a regression here. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1965468 [2] kubernetes/cloud-provider-openstack#1544 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This reverts commit a272e59. As noted in the corresponding bug [1], the expectation was always to revert this once cloud-provided-openstack started providing cluster ID information in snapshot metadata, which has been the case for some time now [2]. Conflicts: pkg/destroy/openstack/openstack.go Changes: pkg/destroy/openstack/openstack.go NOTE(stephenfin): Conflicts are due to commit 375fe6f ("OpenStack: Optimize cluster deletion") which changed two blocks so that we now continue in a loop rather than returning early. We introduce this same logic into a newly restored block inside 'deleteSnapshots' to prevent a regression here. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1965468 [2] kubernetes/cloud-provider-openstack#1544 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
What this PR does / why we need it:
Now cluster id is set by --cluster argument at the driver's start. When it's set, cinder CSI controller appends the cluster id to volumes metadata during creation. Unfortunately it doesn't happen for volume snapshots, which makes it harder to find them after cluster deletion.
This commit adds cluster id to snapshots metadata to unify the behavior with volumes.