Fix the race between PVC finalizer and snapshot finalizer removal #360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xing-yang 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 |
|
/hold |
k8s-ci-robot
added
approved
This was referenced Aug 17, 2020
|
I've tested this, and it addresses the issue that I was mentioning in #354 . I am able to delete a VolumeSnapshot that has a source of a non-CSI PVC. |
|
Just tested this but was unable to delete the PVC nor VolumeSnapshot. This was the manifest that was used, which tries creates a VolumeSnapshot at the same time the PVC is created. |
|
@xing-yang are we targetting this for 2.2? |
Yes. |
xing-yang
force-pushed
the
remove_finalizer
branch
from
d9e0e44
to
1eaa86f
Compare
@chrishenzie I updated the PR. Can you test again? |
|
@xing-yang Just tested using the same manifest linked above. All resources deleted successfully without any issues. Thank you! |
|
@chrishenzie Thank you for testing this! |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
ggriffiths
reviewed
Aug 27, 2020
xing-yang
force-pushed
the
remove_finalizer
branch
from
1eaa86f
to
521502e
Compare
|
@ggriffiths Addressed your comments. PTAL. Thanks. |
yuxiangqian
reviewed
Aug 28, 2020
| @@ -1303,6 +1285,16 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1 | |||
| return nil | |||
There was a problem hiding this comment.
removeSourceFinalizer will always be true, let's just remove this parameter?
There was a problem hiding this comment.
You meant the caller always passes in removeSourceFinalizer as true here? removeSnapshotFinalizer is a utility function so I'd like to make it usable even if removeSourceFinalizer is false for the completeness.
| // If we are here, it means we are going to remove finalizers from the snapshot so that the snapshot can be deleted. | ||
| // We need to check if there is still PVC finalizer that needs to be removed before removing the snapshot finalizers. | ||
| // Once snapshot is deleted, there won't be any snapshot update event that can trigger the PVC finalizer removal. | ||
| if err := ctrl.checkandRemovePVCFinalizer(snapshot, true); err != nil { |
There was a problem hiding this comment.
I am not sure this is the best place to put the fix.
checkandRemoveSnapshotFinalizersAndCheckandDeleteContent, which is the caller of this function, has quite some returning points, i.e., this:
if any of those points is hit, it will still cause the PVC not deletable.
There was a problem hiding this comment.
It's a little tricky. We have to make sure PVC finalizer is deleted before VolumeSnapshot is deleted. Once VolumeSnapshot is deleted, there's no sync routine that handles PVC finalizer deletion any more. We also can't remove PVC finalizer too early. PVC finalizer should stay on if a VolumeSnapshot is still using it.
I add the logic in the current place because this is the place where we are sure we want to delete VolumeSnapshot object.
| @@ -170,17 +170,14 @@ func (ctrl *csiSnapshotCommonController) syncSnapshot(snapshot *crdv1.VolumeSnap | |||
| klog.V(5).Infof("syncSnapshot [%s]: check if we should remove finalizer on snapshot PVC source and remove it if we can", utils.SnapshotKey(snapshot)) | |||
|
|
|||
| // Check if we should remove finalizer on PVC and remove it if we can. | |||
| if err := ctrl.checkandRemovePVCFinalizer(snapshot); err != nil { | |||
| if err := ctrl.checkandRemovePVCFinalizer(snapshot, false); err != nil { | |||
There was a problem hiding this comment.
IMHO, checkandRemovePVCFinalizer can take the responsibility to handle "snapshot.ObjectMeta.DeletionTimestamp != nil" case,
and the bool flag can be saved?
There was a problem hiding this comment.
I think it is safer to have this bool flag passed in. Even when "snapshot.ObjectMeta.DeletionTimestamp != nil", it is possible that snapshot is also used as a source for another PVC. In that case, we still want to keep PVC finalizer on the snapshot's source if snapshot is not ready yet.
k8s-ci-robot
added
the
needs-rebase
xing-yang
force-pushed
the
remove_finalizer
branch
from
e077f67
to
505a0a4
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/retest |
xing-yang
force-pushed
the
remove_finalizer
branch
from
505a0a4
to
1ba5602
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
6 tasks
6 tasks
6 tasks
|
Hi. when list snapshot When I try to delete everything is stuck No any snapshot present in AWS When I describe snapshot: Log snapshot-controller: Questins: How i can solve this issue ? please help |
|
What version of external-snapshotter are you using? |
|
I am using version: aws-ebs-csi-driver Additional test: In two different namespace I have two ready snapshots in each When I delete volumesnapshot from near namespace result is successful : But when I delete snapshot from geth namespace the command is stuck: Both snapshot created with same way When I describe this snapshot I show followed events ErrorPVCFinalyzer: Snapshotter LOG: But if I execute follow command the snapshot delete successful: |
|
Are you trying to modify VolumeSnapshot name? This field is immutable. |
No I try to delete volumesnapshot.
LOG |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #349
Special notes for your reviewer:
Does this PR introduce a user-facing change?: