[snapshot-controller] Retry PVC finalizer removal on conflict #1133
Conversation
The previous `removePVCFinalizer` function was using the PVC stored in the informer which, in cases where the PVC had been modified since, lead to conflict errors when trying to remove the PVC finalizer through an update. Now the `removePVCFinalizer` function uses the `RetryOnConflict` helper to make sure the update goes through.
k8s-ci-robot
added
release-note
|
Hi @Fricounet. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| } | ||
| newPvc = newPvc.DeepCopy() | ||
| newPvc.ObjectMeta.Finalizers = utils.RemoveString(newPvc.ObjectMeta.Finalizers, utils.PVCFinalizer) | ||
| _, err = ctrl.client.CoreV1().PersistentVolumeClaims(newPvc.Namespace).Update(context.TODO(), newPvc, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Using a "patch" instead of an "update" can reduce the conflicts.
There was a problem hiding this comment.
I do agree that a using a "patch" would get rid of all the conflicts however since the finalizers are stored in a list, using a patch seems dangerous to me because if the finalizer list changes between the get and update calls (because another controller removed its finalizer for instance), it means that we would end up removing the wrong finalizer (since we need the index to remove an element from a list using a patch).
Is there something I'm missing that could let us use a patch instead without the drawback above?
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Fricounet, 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 |
k8s-ci-robot
added
the
approved
| newPvc.ObjectMeta.Finalizers = utils.RemoveString(newPvc.ObjectMeta.Finalizers, utils.PVCFinalizer) | ||
| _, err = ctrl.client.CoreV1().PersistentVolumeClaims(newPvc.Namespace).Update(context.TODO(), newPvc, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return newControllerUpdateError(newPvc.Name, err.Error()) |
There was a problem hiding this comment.
folks, I suspect generating a new error here (newControllerUpdateError) results in IsConflict never evaluating to true, and thus the original problem still existing. I hit this on an environment with 8.2.0 which contains this commit.
I can open a PR to tackle
What type of PR is this?
What this PR does / why we need it:
The previous
removePVCFinalizerfunction was using the PVC stored in the informer which, in cases where the PVC had been modified since, lead to conflict errors when trying to remove the PVC finalizer through an update.Example error:
Unfortunately, the error was not retried so the PVC ends up stuck.
Now the
removePVCFinalizerfunction uses theRetryOnConflicthelper to make sure the update goes through.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: