Add finalizer in (Cluster)ResourceBinding to delete works #756

dddddai · 2021-09-24T00:35:53Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Add finalizer in ResourceBinding and ClusterResourceBinding to delete works.

If we delete workload while karmada-controller-manager crashes, this will make sure (Cluster)ResourceBinding remains until controller-manager is ready, then it handles the delete event to delete related works.

Which issue(s) this PR fixes:
Fixes #708

Special notes for your reviewer:
I followed this and verified it works in my local environment.

Does this PR introduce a user-facing change?:

NONE

RainbowMango

Looks pretty good to me.
Just some nits, otherwise LGTM.

This patch might be confit with #752. Let's merge the one which gets ready first. :)

RainbowMango · 2021-09-28T01:42:27Z

pkg/util/helper/binding.go

@@ -174,11 +173,11 @@ func GetWorks(c client.Client, ls labels.Set) (*workv1alpha1.WorkList, error) {
 }

 // DeleteWorks will delete all Work objects by labels.
-func DeleteWorks(c client.Client, selector labels.Set) (controllerruntime.Result, error) {
+func DeleteWorks(c client.Client, selector labels.Set) error {


+1
The helper function should not couple reconciler logic.

RainbowMango · 2021-09-28T01:46:07Z

pkg/util/detector/detector.go

@@ -626,7 +626,8 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure
 			OwnerReferences: []metav1.OwnerReference{
 				*metav1.NewControllerRef(object, objectKey.GroupVersionKind()),
 			},
-			Labels: labels,
+			Labels:     labels,
+			Finalizers: []string{util.BindingControllerFinalizer},


I guess we should update this place as well:

karmada/pkg/util/detector/detector.go

Lines 438 to 445 in 1947cf2

operationResult, err := controllerutil.CreateOrUpdate(context.TODO(), d.Client, bindingCopy, func() error {

// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.

bindingCopy.Labels = binding.Labels

bindingCopy.OwnerReferences = binding.OwnerReferences

bindingCopy.Spec.Resource = binding.Spec.Resource

bindingCopy.Spec.ReplicaRequirements = binding.Spec.ReplicaRequirements

bindingCopy.Spec.Replicas = binding.Spec.Replicas

return nil

Same as below.

+1, bindingCopy.Finalizers = binding.Finalizers

pkg/controllers/binding/binding_controller.go

RainbowMango · 2021-09-28T01:54:06Z

This patch is reasonable to me. Would like to take a look?
@mrlihanbo @Garrybest

RainbowMango · 2021-09-28T02:02:51Z

/hold
hold on. I just realized #708 has been claimed by @shuazi (#708 (comment)), unfortunately, the assignment seems not successful.

@shuazi Have you started your work?

Garrybest

I suppose this PR is related to #756.

pkg/controllers/binding/binding_controller.go

Garrybest · 2021-09-28T02:03:56Z

pkg/util/detector/detector.go

@@ -626,7 +626,8 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure
 			OwnerReferences: []metav1.OwnerReference{
 				*metav1.NewControllerRef(object, objectKey.GroupVersionKind()),
 			},
-			Labels: labels,
+			Labels:     labels,
+			Finalizers: []string{util.BindingControllerFinalizer},


+1, bindingCopy.Finalizers = binding.Finalizers

dddddai · 2021-09-28T02:47:45Z

@RainbowMango @Garrybest Thanks for your review!
I have modified as you addressed, PTAL

I suppose this PR is related to #756.

Sorry I didn't get your point, this is exactly #756😂

Garrybest · 2021-09-28T02:50:22Z

Sorry I didn't get your point, this is exactly #756😂

Oh I'm sorry. I mean #752.

RainbowMango · 2021-09-29T02:10:16Z

/lgtm
But, @dddddai you have to solve the conflict.

RainbowMango · 2021-09-29T02:11:52Z

/hold cancel
Didn't get a response from @shuaizi.

pkg/controllers/binding/binding_controller.go

pkg/controllers/binding/cluster_resource_binding_controller.go

dddddai · 2021-09-29T03:06:48Z

Sorry, I'm still working on the conflict and squash...

RainbowMango · 2021-09-29T03:08:15Z

Fine, no rush.

Signed-off-by: dddddai <dddwq@foxmail.com>

dddddai · 2021-09-29T03:43:47Z

@RainbowMango @XiShanYongYe-Chang Thanks for elaborating, conflict is resolved, PTAL

karmada-bot · 2021-09-29T03:48:08Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: RainbowMango

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

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

XiShanYongYe-Chang · 2021-09-29T03:49:31Z

/lgtm

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 24, 2021

karmada-bot requested review from Garrybest and mrlihanbo September 24, 2021 00:35

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2021

RainbowMango reviewed Sep 28, 2021

View reviewed changes

karmada-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021

Garrybest reviewed Sep 28, 2021

View reviewed changes

dddddai force-pushed the add-finalizer branch 2 times, most recently from bfec0a5 to 5efa01b Compare September 28, 2021 02:25

dddddai force-pushed the add-finalizer branch 2 times, most recently from c452b9f to 48cd417 Compare September 28, 2021 02:55

karmada-bot assigned RainbowMango Sep 29, 2021

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021

karmada-bot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 29, 2021

XiShanYongYe-Chang reviewed Sep 29, 2021

View reviewed changes

pkg/controllers/binding/binding_controller.go Outdated Show resolved Hide resolved

pkg/controllers/binding/cluster_resource_binding_controller.go Outdated Show resolved Hide resolved

dddddai marked this pull request as draft September 29, 2021 03:06

karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2021

dddddai force-pushed the add-finalizer branch from 8b0537f to ede9fad Compare September 29, 2021 03:21

add finalizer in resource binding and cluster resource binding

ede9fad

Signed-off-by: dddddai <dddwq@foxmail.com>

dddddai marked this pull request as ready for review September 29, 2021 03:43

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2021

RainbowMango approved these changes Sep 29, 2021

View reviewed changes

RainbowMango added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2021

karmada-bot assigned XiShanYongYe-Chang Sep 29, 2021

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021

karmada-bot merged commit 5be78ba into karmada-io:master Sep 29, 2021

dddddai deleted the add-finalizer branch October 8, 2021 03:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add finalizer in (Cluster)ResourceBinding to delete works #756

Add finalizer in (Cluster)ResourceBinding to delete works #756

dddddai commented Sep 24, 2021 •

edited

Loading

RainbowMango left a comment

RainbowMango Sep 28, 2021

RainbowMango Sep 28, 2021

Garrybest Sep 28, 2021

dddddai Sep 28, 2021

RainbowMango commented Sep 28, 2021

RainbowMango commented Sep 28, 2021

Garrybest left a comment

Garrybest Sep 28, 2021

dddddai commented Sep 28, 2021

Garrybest commented Sep 28, 2021

RainbowMango commented Sep 29, 2021

RainbowMango commented Sep 29, 2021

dddddai commented Sep 29, 2021

RainbowMango commented Sep 29, 2021

dddddai commented Sep 29, 2021

karmada-bot commented Sep 29, 2021

XiShanYongYe-Chang commented Sep 29, 2021

	operationResult, err := controllerutil.CreateOrUpdate(context.TODO(), d.Client, bindingCopy, func() error {
	// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
	bindingCopy.Labels = binding.Labels
	bindingCopy.OwnerReferences = binding.OwnerReferences
	bindingCopy.Spec.Resource = binding.Spec.Resource
	bindingCopy.Spec.ReplicaRequirements = binding.Spec.ReplicaRequirements
	bindingCopy.Spec.Replicas = binding.Spec.Replicas
	return nil

Add finalizer in (Cluster)ResourceBinding to delete works #756

Add finalizer in (Cluster)ResourceBinding to delete works #756

Conversation

dddddai commented Sep 24, 2021 • edited Loading

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango Sep 28, 2021

Choose a reason for hiding this comment

RainbowMango Sep 28, 2021

Choose a reason for hiding this comment

Garrybest Sep 28, 2021

Choose a reason for hiding this comment

dddddai Sep 28, 2021

Choose a reason for hiding this comment

RainbowMango commented Sep 28, 2021

RainbowMango commented Sep 28, 2021

Garrybest left a comment

Choose a reason for hiding this comment

Garrybest Sep 28, 2021

Choose a reason for hiding this comment

dddddai commented Sep 28, 2021

Garrybest commented Sep 28, 2021

RainbowMango commented Sep 29, 2021

RainbowMango commented Sep 29, 2021

dddddai commented Sep 29, 2021

RainbowMango commented Sep 29, 2021

dddddai commented Sep 29, 2021

karmada-bot commented Sep 29, 2021

XiShanYongYe-Chang commented Sep 29, 2021

dddddai commented Sep 24, 2021 •

edited

Loading