Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023
Conversation
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> remove debugging code Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> remove more update calls Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Fix patch json unmarshal unitTests comparison failures Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> Fix tests in reactor by not modifying original for patch Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
| Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
k8s-ci-robot added do-not-merge/release-note-label-needed | Hi @kaovilai. 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. |
| [APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kaovilai 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 |
k8s-ci-robot added the size/L 
kaovilai changed the title 
pkg/utils/update.go Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return UpdateRemoveFinalizers(obj, client, finalizers...) |
There was a problem hiding this comment.
I've been told before that recursion is bad.. but felt like this is visually easier to read and it's not expected to run in most cases. Can refactor to non-recursion if needed.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai force-pushed the removeUpdateCallsForAdding,GetWhenApplyOutOfDate branch from 027dcff to 931529b Compare Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
pkg/utils/update.go Outdated
| object.SetFinalizers(RemoveStrings(object.GetFinalizers(), finalizers...)) | ||
| object, err := updateFunc(context.TODO(), object, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsConflict(err) { | ||
| object, err = getFunc(context.TODO(), object.GetName(), metav1.GetOptions{}) | ||
| if err != nil { | ||
| return object, err | ||
| } | ||
| return UpdateRemoveFinalizers(object, updateFunc, getFunc, finalizers...) | ||
| } else { | ||
| return object, err | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the main difference from #1019
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
| { | ||
| name: "2-4 - successful remove Snapshot finalizer after update conflict", | ||
| initialSnapshots: newSnapshotArray("snap2-4", "snapuid2-4", "claim2-4", "", classSilver, "", &False, nil, nil, nil, false, true, nil), | ||
| initialClaims: newClaimArray("claim2-4", "pvc-uid2-4", "1Gi", "volume2-4", v1.ClaimBound, &classEmpty), | ||
| test: testRemoveSnapshotFinalizerAfterUpdateConflict, | ||
| expectSuccess: true, | ||
| errors: []reactorError{ | ||
| {"update", "volumesnapshots", errors.NewConflict(crdv1.Resource("volumesnapshots"), "snap2-4", nil)}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This is the error case we're solving for.
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai force-pushed the removeUpdateCallsForAdding,GetWhenApplyOutOfDate branch from 0672e8d to 8c61946 Compare 
2 participants
Signed-off-by: Tiger Kaovilai tkaovila@redhat.com
What type of PR is this?
What this PR does / why we need it:
This PR cleans up some of the unit test reactor code, and eliminate update calls that I can see, fixing unit tests to accommodate the changes.
Previously update call was required simply cause unit test is borked. Patch calls was modifying original input causing compare failures.
Which issue(s) this PR fixes:
Fixes #748
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
This PR extend #876 work to later added update calls.
Similar to #1019 but using Update calls for finalizer removal.