-
Notifications
You must be signed in to change notification settings - Fork 359
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
delete the content if creation failed and snapshot deleted #1064
base: master
Are you sure you want to change the base?
Conversation
Add bound finalizer to Retain policy snapshots to make sure the annotate logic in checkandRemoveSnapshotFinalizersAndCheckandDeleteContent() effective. The added finalizer does not change the current behaviour, because the source finalizer is always added and only removed after the vs-deleted annotation is added successfully. But we should not rely on this behaviour in case of future changes. The original annotate logic in syncContent() is not needed because all deleting snapshot will go through above logic. And natually, if a content is never bound, is will never get the vs-deleted annotation. This also makes Retain and Delete policy snapshot more similar, which should bring less surprises.
This is useful for Retain policy snapshot. When volume snapshot is created with invalid config, then deleted, all things should be cleaned up, instead of leaving a content to retry creation infinitely.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: huww98 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @huww98! |
Hi @huww98. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/cc @xing-yang |
b25db0d
to
293aa90
Compare
}, | ||
expectSuccess: false, | ||
test: testSyncContentError, | ||
expectSuccess: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should always do expectSuccess: true
? Specify false means don't check for failure, not asserting error is returned. But that is a big project. Let's leave it to another PR.
// sidecar controller depending on the deletion policy | ||
// on the content. | ||
// Snapshot won't be deleted until content is deleted | ||
// due to the finalizer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to remove this whole block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
external-snapshotter/pkg/common-controller/snapshot_controller.go
Lines 332 to 337 in 3a35a5e
// regardless of the deletion policy, set the VolumeSnapshotBeingDeleted on | |
// content object, this is to allow snapshotter sidecar controller to conduct | |
// a delete operation whenever the content has deletion timestamp set. | |
if content != nil { | |
klog.V(5).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: Set VolumeSnapshotBeingDeleted annotation on the content [%s]", utils.SnapshotKey(snapshot), content.Name) | |
updatedContent, err := ctrl.setAnnVolumeSnapshotBeingDeleted(content) |
This code should already handle it.
In the commit message of second commit, I wrote:
The original annotate logic in syncContent() is not needed because all deleting snapshot will go through above logic. And natually, if a content is never bound, is will never get the vs-deleted annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xing-yang What do you think about the overall direction of this PR? Should we continue to work on this? These change is not required for this PR. If you agree, I can defer it to another PR.
What type of PR is this?
/kind cleanup
/kind feature
What this PR does / why we need it:
This is useful for Retain policy snapshot. When volume snapshot is created with invalid config, then deleted, all things should be cleaned up, instead of leaving a content to retry creation infinitely.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Some cleanups are included, please see commit message.
A new RBAC permission is added.
Does this PR introduce a user-facing change?: