Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023

kaovilai · 2024-02-26T21:28:54Z

Signed-off-by: Tiger Kaovilai [email protected]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

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.

Signed-off-by: Tiger Kaovilai <[email protected]> remove debugging code Signed-off-by: Tiger Kaovilai <[email protected]> remove more update calls Signed-off-by: Tiger Kaovilai <[email protected]> Fix patch json unmarshal unitTests comparison failures Signed-off-by: Tiger Kaovilai <[email protected]> Fix tests in reactor by not modifying original for patch Signed-off-by: Tiger Kaovilai <[email protected]>

Signed-off-by: Tiger Kaovilai <[email protected]>

k8s-ci-robot · 2024-02-26T21:28:56Z

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 · 2024-02-26T21:29:03Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2024-02-26T21:29:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kaovilai
Once this PR has been reviewed and has the lgtm label, please assign xing-yang for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kaovilai · 2024-02-26T21:37:46Z

pkg/utils/update.go

+			if err != nil {
+				return nil, err
+			}
+			return UpdateRemoveFinalizers(obj, client, finalizers...)


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 <[email protected]>

kaovilai · 2024-02-26T22:08:56Z

pkg/utils/update.go

+	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
+		}
+	}


This is the main difference from #1019

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai · 2024-02-27T05:29:59Z

pkg/common-controller/snapshot_finalizer_test.go

+		{
+			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)},
+			},
+		},


This is the error case we're solving for.

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai · 2024-02-27T22:04:19Z

Closing due to other priorities as the sense is this isn't going to be approved as is and to see if #876 recently cherrypicked to our nightlies would be sufficient. Feel free to takeover.

Test improvements are moved to #1024

kaovilai added 5 commits February 23, 2024 22:16

Use remove all + add instead of removing by index

2e56da8

Signed-off-by: Tiger Kaovilai <[email protected]>

fix sync tests

6740b07

Signed-off-by: Tiger Kaovilai <[email protected]>

clean up debugging content

84e574f

Signed-off-by: Tiger Kaovilai <[email protected]>

go fmt

db6559f

Signed-off-by: Tiger Kaovilai <[email protected]>

k8s-ci-robot requested a review from RaunakShah February 26, 2024 21:29

k8s-ci-robot requested a review from yuxiangqian February 26, 2024 21:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 26, 2024

kaovilai mentioned this pull request Feb 26, 2024

Use patch instead of update for GroupSnapshots, VolumeSnapshots, PVCs #1019

Closed

kaovilai changed the title ~~Use Patch to add finalizers, Get updated objects when removing finalizer with update~~ Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. Feb 26, 2024

kaovilai commented Feb 26, 2024

View reviewed changes

Use update to remove finalizers instead of patch with get retries

931529b

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai force-pushed the removeUpdateCallsForAdding,GetWhenApplyOutOfDate branch from 027dcff to 931529b Compare February 26, 2024 21:38

Use generics for Update function

9d6b138

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai commented Feb 26, 2024

View reviewed changes

kaovilai added 2 commits February 27, 2024 00:06

Remove recursion, add conflict tests

03a1484

Signed-off-by: Tiger Kaovilai <[email protected]>

go fmt

4541deb

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai commented Feb 27, 2024

View reviewed changes

add wait backoff

8c61946

Signed-off-by: Tiger Kaovilai <[email protected]>

kaovilai force-pushed the removeUpdateCallsForAdding,GetWhenApplyOutOfDate branch from 0672e8d to 8c61946 Compare February 27, 2024 05:31

kaovilai mentioned this pull request Feb 27, 2024

Test framework improvements #1024

Closed

kaovilai closed this Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023

Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023

kaovilai commented Feb 26, 2024 •

edited

Loading

k8s-ci-robot commented Feb 26, 2024

k8s-ci-robot commented Feb 26, 2024

k8s-ci-robot commented Feb 26, 2024

kaovilai Feb 26, 2024

kaovilai Feb 26, 2024

kaovilai Feb 27, 2024

kaovilai commented Feb 27, 2024 •

edited

Loading

Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023

Use Patch to add finalizers, Get updated objects when removing finalizer with update fails with conflict. #1023

Conversation

kaovilai commented Feb 26, 2024 • edited Loading

k8s-ci-robot commented Feb 26, 2024

k8s-ci-robot commented Feb 26, 2024

k8s-ci-robot commented Feb 26, 2024

kaovilai Feb 26, 2024

Choose a reason for hiding this comment

kaovilai Feb 26, 2024

Choose a reason for hiding this comment

kaovilai Feb 27, 2024

Choose a reason for hiding this comment

kaovilai commented Feb 27, 2024 • edited Loading

kaovilai commented Feb 26, 2024 •

edited

Loading

kaovilai commented Feb 27, 2024 •

edited

Loading