🐛 remove klusterlet finalizer forcely #214

zhujian7 · 2023-07-06T15:00:08Z

Summary

Now when we delete a klusterlet CR, the klusterlet-operator will remove the klusterlet.spec.namespace namespace, before removing the finalize operator.open-cluster-management.io/klusterlet-cleanup for the klusterlet CR.
so there is a case, if the klusterlet-operator is running in the klusterlet.spec.namespace namespace, removing the namespace will trigger deleting the klusterlet-operator itself, and if the klusterlet-operator is deleted before the finalize is removed(eg, remove finalize got conflict), the klusterlet will be left and can not be deleted automatically.

this PR is trying to resolve the conflict error when removing the finalizer.

BTW, this PR will fix the warning log:

W0710 06:36:59.764545       1 warnings.go:70] unknown field "metadata.UID"

Related issue(s)

Fixes #

codecov · 2023-07-06T15:33:02Z

Codecov Report

Patch coverage: 82.85% and project coverage change: +0.08 🎉

Comparison is base (6649834) 60.18% compared to head (27e126c) 60.26%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   60.18%   60.26%   +0.08%     
==========================================
  Files         128      128              
  Lines       13373    13411      +38     
==========================================
+ Hits         8048     8082      +34     
- Misses       4581     4584       +3     
- Partials      744      745       +1

Flag	Coverage Δ
unit	`60.26% <82.85%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...sterletcontroller/klusterlet_cleanup_controller.go	`54.40% <0.00%> (-0.29%)`	⬇️
...usterletcontroller/klusterlet_managed_reconcile.go	`52.88% <50.00%> (ø)`
pkg/common/patcher/patcher.go	`76.00% <86.36%> (+3.60%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

qiujian16 · 2023-07-07T01:12:32Z

pkg/common/patcher/patcher.go

@@ -131,6 +140,16 @@ func (p *patcher[R, Sp, St]) RemoveFinalizer(ctx context.Context, object R, fina
 		if errors.IsNotFound(err) {
 			return nil
 		}
+
+		// If it's a conflict, we need to get the latest version of the object and try again.
+		if errors.IsConflict(err) {


it seems too greedy, you have to set a backoff time.

but setting a backoff time will make the retry slow, which is not good for deleting the klusterlet CR.
can we set a retry times(eg.3 times) into the ctx, and if it exceeds, we stop?

zhujian7 · 2023-07-10T07:16:17Z

@qiujian16 PTAL

qiujian16 · 2023-07-10T08:12:39Z

pkg/common/patcher/patcher.go

@@ -25,6 +25,8 @@ type PatchClient[R runtime.Object] interface {
 type Patcher[R runtime.Object, Sp any, St any] interface {
 	AddFinalizer(context.Context, R, ...string) (bool, error)
 	RemoveFinalizer(context.Context, R, ...string) error


actually, could we add opts var in the RemoveFinalizer func

you mean the patch option for the p.client.Patch(ctx, accessor.GetName(), types.MergePatchType, patchBytes, metav1.PatchOptions{})？

we will not set force to true for the patch option, it is used for re-acquire conflicting fields owned by other people.
in our case, we just need to keep the resourceVersion empty.

not necessary to be a mevat.PatchOptions, but a options field that you can know whether it is force patch or not.

added a PatchOptions param for the RemoveFinalizers func.

qiujian16 · 2023-07-10T13:51:29Z

...rator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_cleanup_controller.go

@@ -191,7 +191,7 @@ func (n *klusterletCleanupController) sync(ctx context.Context, controllerContex
 		return utilerrors.NewAggregate(errs)
 	}

-	return n.patcher.RemoveFinalizerForcely(ctx, klusterlet, klusterletFinalizer, klusterletHostedFinalizer)
+	return n.patcher.RemoveFinalizer(ctx, klusterlet, patcher.PatchOptions{IgnoreResourceVersion: true}, klusterletFinalizer, klusterletHostedFinalizer)


will it be better if it is like

n.patcher.WithOptions(opts).RemoveFinalizer? ....

Do you mean to make it work for all funcs? not only for RemoveFinalizers?

yes, I think so

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 · 2023-07-11T06:33:10Z

@qiujian16 PTAL

qiujian16 · 2023-07-11T06:39:00Z

/approve
/lgtm

openshift-ci · 2023-07-11T06:39:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhujian7

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:

~~OWNERS~~ [qiujian16]

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

openshift-ci bot added the do-not-merge/work-in-progress label Jul 6, 2023

openshift-ci bot requested review from jnpacker and mikeshng July 6, 2023 15:00

zhujian7 force-pushed the remove-finalizer-conflict branch from 38a705d to b4298c3 Compare July 6, 2023 15:24

zhujian7 changed the title ~~WIP: 🐛 retry when getting a conflict error for removing finalizer~~ 🌱 retry when getting a conflict error for removing finalizer Jul 6, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 6, 2023

zhujian7 force-pushed the remove-finalizer-conflict branch from b4298c3 to 39e21d3 Compare July 6, 2023 15:51

qiujian16 reviewed Jul 7, 2023

View reviewed changes

zhujian7 mentioned this pull request Jul 9, 2023

fix e2e issues stolostron/managedcluster-import-controller#232

Merged

zhujian7 force-pushed the remove-finalizer-conflict branch from 39e21d3 to 509b231 Compare July 10, 2023 05:56

zhujian7 changed the title ~~🌱 retry when getting a conflict error for removing finalizer~~ 🐛: remove klusterlet finalizer forcely Jul 10, 2023

zhujian7 changed the title ~~🐛: remove klusterlet finalizer forcely~~ 🐛 remove klusterlet finalizer forcely Jul 10, 2023

zhujian7 force-pushed the remove-finalizer-conflict branch from 509b231 to 70523cd Compare July 10, 2023 06:35

qiujian16 reviewed Jul 10, 2023

View reviewed changes

zhujian7 added 3 commits July 11, 2023 02:34

remove klusterlet finalizer forcely

345fe5a

Signed-off-by: zhujian <jiazhu@redhat.com>

add patch options for patcher

66b9f22

Signed-off-by: zhujian <jiazhu@redhat.com>

add patch options for all patcher funcs

27e126c

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the remove-finalizer-conflict branch from c8f9215 to 27e126c Compare July 11, 2023 02:53

openshift-ci bot assigned qiujian16 Jul 11, 2023

openshift-ci bot added the lgtm label Jul 11, 2023

openshift-ci bot added the approved label Jul 11, 2023

openshift-merge-robot merged commit 5db2240 into open-cluster-management-io:main Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 remove klusterlet finalizer forcely #214

🐛 remove klusterlet finalizer forcely #214

zhujian7 commented Jul 6, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

qiujian16 Jul 7, 2023 •

edited

Loading

zhujian7 Jul 7, 2023

zhujian7 commented Jul 10, 2023

qiujian16 Jul 10, 2023

zhujian7 Jul 10, 2023

qiujian16 Jul 10, 2023

zhujian7 Jul 10, 2023

qiujian16 Jul 10, 2023

zhujian7 Jul 10, 2023 •

edited

Loading

qiujian16 Jul 11, 2023

zhujian7 Jul 11, 2023

zhujian7 commented Jul 11, 2023

qiujian16 commented Jul 11, 2023

openshift-ci bot commented Jul 11, 2023

🐛 remove klusterlet finalizer forcely #214

🐛 remove klusterlet finalizer forcely #214

Conversation

zhujian7 commented Jul 6, 2023 • edited Loading

Summary

Related issue(s)

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

qiujian16 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhujian7 commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhujian7 Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhujian7 commented Jul 11, 2023

qiujian16 commented Jul 11, 2023

openshift-ci bot commented Jul 11, 2023

zhujian7 commented Jul 6, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

qiujian16 Jul 7, 2023 •

edited

Loading

zhujian7 Jul 10, 2023 •

edited

Loading