reduce useless logic to improve dependencies distributor #4532

whitewindmills · 2024-01-10T11:53:59Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
The dependent distributor does not pay attention to deleted or deleting attached resource templates, so optimize this part of the logic.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

XiShanYongYe-Chang · 2024-01-12T01:58:55Z

I don't quite understand why dependencies distributor doesn't need to be concerned about deletion events. When the attached resource template is deleted, the corresponding rb should also need to be cleaned up.

whitewindmills · 2024-01-12T02:40:21Z

I don't quite understand why dependencies distributor doesn't need to be concerned about deletion events. When the attached resource template is deleted, the corresponding rb should also need to be cleaned up.

Since the attached resource template is deleted, so it's resourceBinding is deleted, cleaning up makes no sense.

XiShanYongYe-Chang · 2024-01-12T03:29:58Z

Since the attached resource template is deleted, so it's resourceBinding is deleted, cleaning up makes no sense.

Are you referring to the gc controller responsible for cleaning up the resource bindings of the resource template?

whitewindmills · 2024-01-12T03:31:55Z

Are you referring to the gc controller responsible for cleaning up the resource bindings of the resource template?

sure

XiShanYongYe-Chang · 2024-01-12T03:34:32Z

Thanks, I get it. Can you help add some comments on the event handler to explain why we don't need to care about this?

whitewindmills · 2024-01-12T03:42:46Z

Can you help add some comments on the event handler to explain why we don't need to care about this?

okay

XiShanYongYe-Chang · 2024-01-12T03:47:42Z

/cc @chaunceyjiang
/assign

codecov-commenter · 2024-01-12T04:09:19Z

Codecov Report

Attention: Patch coverage is 26.66667% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 53.26%. Comparing base (81acb31) to head (b30e537).

Files	Patch %	Lines
...ependenciesdistributor/dependencies_distributor.go	26.66%	20 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4532      +/-   ##
==========================================
- Coverage   53.29%   53.26%   -0.03%     
==========================================
  Files         252      252              
  Lines       20495    20508      +13     
==========================================
+ Hits        10922    10924       +2     
- Misses       8851     8861      +10     
- Partials      722      723       +1

Flag	Coverage Δ
unittests	`53.26% <26.66%> (-0.03%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

XiShanYongYe-Chang

Other parts LGTM

pkg/dependenciesdistributor/dependencies_distributor.go

whitewindmills · 2024-01-15T09:14:23Z

ping @chaunceyjiang
could you help take a look?

XiShanYongYe-Chang · 2024-01-15T09:28:40Z

/lgtm
/kind cleanup

XiShanYongYe-Chang · 2024-01-22T14:14:01Z

Kind ping @chaunceyjiang @RainbowMango
/cc @chaunceyjiang @RainbowMango

pkg/dependenciesdistributor/dependencies_distributor.go

whitewindmills · 2024-02-29T06:55:09Z

Could you help move it forward?
/assign @RainbowMango

RainbowMango · 2024-02-29T07:07:36Z

@chaunceyjiang Do you want to take another look? As you are the related code author.

whitewindmills · 2024-04-16T08:55:14Z

@chaunceyjiang Could you help take a look?

XiShanYongYe-Chang · 2024-05-25T09:58:21Z

Hi @whitewindmills, We can move forward with this PR. Can you rebase it and confirm if there are any required changes?

karmada-bot · 2024-05-27T01:36:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rainbowmango. 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

Signed-off-by: whitewindmills <[email protected]>

whitewindmills · 2024-05-27T02:52:57Z

ping @RainbowMango @XiShanYongYe-Chang @chaunceyjiang

RainbowMango

/assign @XiShanYongYe-Chang @chaunceyjiang

RainbowMango · 2024-05-28T03:17:53Z

pkg/dependenciesdistributor/dependencies_distributor.go

+	if !unstructuredNewObj.GetDeletionTimestamp().IsZero() {
+		klog.V(4).Infof("Ignore update event of object(%s, kind=%s, %s) as being deleted.",
+			unstructuredNewObj.GetAPIVersion(), unstructuredNewObj.GetKind(), names.NamespacedKey(unstructuredNewObj.GetNamespace(), unstructuredNewObj.GetName()))


It's better to check it in the reconciling loop in case there are some clean-up tasks.

but it's for resource template, we won't have any cleanup.

+1

Our current model cannot discard this event, otherwise it would not be possible to do the cleanup.

Other parts LGTM.

I don't get what to clean up. the schedule result of attached bindings? or labels about dependencies?
since it will definitely be deleted, there is no point in us cleaning these.

For this part:

karmada/pkg/dependenciesdistributor/dependencies_distributor.go

Lines 256 to 263 in d676996

if !bindingObject.Spec.PropagateDeps || !bindingObject.DeletionTimestamp.IsZero() {

err = d.handleIndependentBindingDeletion(request.Namespace, request.Name)

if err != nil {

klog.Errorf("Failed to cleanup attached bindings for independent binding(%s): %v", request.NamespacedName, err)

return reconcile.Result{}, err

}

return reconcile.Result{}, d.removeFinalizer(ctx, bindingObject)

}

~~We have added a finalizer. The delete event does not handle this.~~

Sorry, I gave the wrong example. This is a resource template update event.

I don't have a strong opinion just feel not good to do such things in event filter.
I'm not sure that's a good pattern, my thinking is the event filter should be simple.
Let's @chaunceyjiang make the final decision.

I don't have a strong opinion just feel not good to do such things in event filter.
I'm not sure that's a good pattern, my thinking is the event filter should be simple.

I don't totally agree. if unnecessary events can be filtered out before they are queued, this can significantly improve the efficiency of the work queue. Of course, keeping it simple is also a consideration, but compared to this type of processing in the detector, this obviously does not add much complexity.

karmada/pkg/detector/detector.go

Lines 907 to 953 in 6fbdd9e

func (d *ResourceDetector) OnPropagationPolicyUpdate(oldObj, newObj interface{}) {

d.policyReconcileWorker.Enqueue(newObj)

// Temporary solution of corner case: After the priority(.spec.priority) of

// PropagationPolicy changed from high priority (e.g. 5) to low priority(e.g. 3),

// we should try to check if there is a PropagationPolicy(e.g. with priority 4)

// could preempt the targeted resources.

//

// Recognized limitations of the temporary solution are:

// - Too much logical processed in an event handler function will slow down

// the overall reconcile speed.

// - If there is an error raised during the process, the event will be lost

// and no second chance to retry.

//

// The idea of the long-term solution, perhaps PropagationPolicy could have

// a status, in that case we can record the observed priority(.status.observedPriority)

// which can be used to detect priority changes during reconcile logic.

if features.FeatureGate.Enabled(features.PolicyPreemption) {

var unstructuredOldObj *unstructured.Unstructured

var unstructuredNewObj *unstructured.Unstructured

unstructuredOldObj, err := helper.ToUnstructured(oldObj)

if err != nil {

klog.Errorf("Failed to transform oldObj, error: %v", err)

return

}

unstructuredNewObj, err = helper.ToUnstructured(newObj)

if err != nil {

klog.Errorf("Failed to transform newObj, error: %v", err)

return

}

var oldPolicy policyv1alpha1.PropagationPolicy

var newPolicy policyv1alpha1.PropagationPolicy

if err = helper.ConvertToTypedObject(unstructuredOldObj, &oldPolicy); err != nil {

klog.Errorf("Failed to convert typed PropagationPolicy(%s/%s): %v", unstructuredOldObj.GetNamespace(), unstructuredOldObj.GetName(), err)

return

}

if err = helper.ConvertToTypedObject(unstructuredNewObj, &newPolicy); err != nil {

klog.Errorf("Failed to convert typed PropagationPolicy(%s/%s): %v", newPolicy.GetNamespace(), newPolicy.GetName(), err)

return

}

if newPolicy.ExplicitPriority() < oldPolicy.ExplicitPriority() {

d.HandleDeprioritizedPropagationPolicy(oldPolicy, newPolicy)

}

It's better to check it in the reconciling loop in case there are some clean-up tasks.

I also lean towards this, I don't really recommend adding too much logic processing in the event Filter.

chaunceyjiang · 2024-05-28T03:39:09Z

/assign

whitewindmills · 2024-05-30T06:20:23Z

ping @chaunceyjiang

chaunceyjiang · 2024-05-31T03:07:10Z

pkg/dependenciesdistributor/dependencies_distributor.go

+		klog.Errorf("Failed to transform newObj, error: %v", err)
+		return
+	}
+	if !unstructuredNewObj.GetDeletionTimestamp().IsZero() {


I remember, if a resource doesn't have Finalizers, then when it is deleted, there won't be any update events.
So, we have no way to check the DeletionTimestamp.

Most resources probably don't have Finalizers.

what this PR wants to do is to ignore the resources that need to be deleted.

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 10, 2024

karmada-bot requested review from chaunceyjiang and jwcesign January 10, 2024 11:54

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 10, 2024

karmada-bot assigned XiShanYongYe-Chang Jan 12, 2024

whitewindmills force-pushed the improve_dependencies_distributor branch from 1d7989b to b901dcb Compare January 12, 2024 03:57

XiShanYongYe-Chang reviewed Jan 12, 2024

View reviewed changes

pkg/dependenciesdistributor/dependencies_distributor.go Show resolved Hide resolved

karmada-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. labels Jan 15, 2024

karmada-bot requested a review from RainbowMango January 22, 2024 14:14

RainbowMango reviewed Jan 24, 2024

View reviewed changes

pkg/dependenciesdistributor/dependencies_distributor.go Show resolved Hide resolved

karmada-bot assigned RainbowMango Feb 29, 2024

whitewindmills force-pushed the improve_dependencies_distributor branch from b901dcb to 43f0dca Compare May 27, 2024 01:36

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label May 27, 2024

reduce useless logic to improve dependencies distributor

b30e537

Signed-off-by: whitewindmills <[email protected]>

whitewindmills force-pushed the improve_dependencies_distributor branch from 43f0dca to b30e537 Compare May 27, 2024 01:48

RainbowMango reviewed May 28, 2024

View reviewed changes

karmada-bot assigned chaunceyjiang May 28, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2024

chaunceyjiang reviewed May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce useless logic to improve dependencies distributor #4532

reduce useless logic to improve dependencies distributor #4532

whitewindmills commented Jan 10, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

codecov-commenter commented Jan 12, 2024 •

edited

Loading

XiShanYongYe-Chang left a comment

whitewindmills commented Jan 15, 2024

XiShanYongYe-Chang commented Jan 15, 2024

XiShanYongYe-Chang commented Jan 22, 2024

whitewindmills commented Feb 29, 2024

RainbowMango commented Feb 29, 2024

whitewindmills commented Apr 16, 2024

XiShanYongYe-Chang commented May 25, 2024

karmada-bot commented May 27, 2024

whitewindmills commented May 27, 2024

RainbowMango left a comment

RainbowMango May 28, 2024

whitewindmills May 28, 2024

XiShanYongYe-Chang May 28, 2024

whitewindmills May 28, 2024

XiShanYongYe-Chang May 28, 2024

XiShanYongYe-Chang May 28, 2024 •

edited

Loading

XiShanYongYe-Chang May 28, 2024

RainbowMango May 28, 2024

whitewindmills May 29, 2024

chaunceyjiang May 30, 2024

chaunceyjiang commented May 28, 2024

whitewindmills commented May 30, 2024

chaunceyjiang May 31, 2024 •

edited

Loading

whitewindmills Jun 3, 2024

	if !bindingObject.Spec.PropagateDeps \|\| !bindingObject.DeletionTimestamp.IsZero() {
	err = d.handleIndependentBindingDeletion(request.Namespace, request.Name)
	if err != nil {
	klog.Errorf("Failed to cleanup attached bindings for independent binding(%s): %v", request.NamespacedName, err)
	return reconcile.Result{}, err
	}
	return reconcile.Result{}, d.removeFinalizer(ctx, bindingObject)
	}

	func (d *ResourceDetector) OnPropagationPolicyUpdate(oldObj, newObj interface{}) {
	d.policyReconcileWorker.Enqueue(newObj)

	// Temporary solution of corner case: After the priority(.spec.priority) of
	// PropagationPolicy changed from high priority (e.g. 5) to low priority(e.g. 3),
	// we should try to check if there is a PropagationPolicy(e.g. with priority 4)
	// could preempt the targeted resources.
	//
	// Recognized limitations of the temporary solution are:
	// - Too much logical processed in an event handler function will slow down
	// the overall reconcile speed.
	// - If there is an error raised during the process, the event will be lost
	// and no second chance to retry.
	//
	// The idea of the long-term solution, perhaps PropagationPolicy could have
	// a status, in that case we can record the observed priority(.status.observedPriority)
	// which can be used to detect priority changes during reconcile logic.
	if features.FeatureGate.Enabled(features.PolicyPreemption) {
	var unstructuredOldObj *unstructured.Unstructured
	var unstructuredNewObj *unstructured.Unstructured

	unstructuredOldObj, err := helper.ToUnstructured(oldObj)
	if err != nil {
	klog.Errorf("Failed to transform oldObj, error: %v", err)
	return
	}
	unstructuredNewObj, err = helper.ToUnstructured(newObj)
	if err != nil {
	klog.Errorf("Failed to transform newObj, error: %v", err)
	return
	}

	var oldPolicy policyv1alpha1.PropagationPolicy
	var newPolicy policyv1alpha1.PropagationPolicy

	if err = helper.ConvertToTypedObject(unstructuredOldObj, &oldPolicy); err != nil {
	klog.Errorf("Failed to convert typed PropagationPolicy(%s/%s): %v", unstructuredOldObj.GetNamespace(), unstructuredOldObj.GetName(), err)
	return
	}
	if err = helper.ConvertToTypedObject(unstructuredNewObj, &newPolicy); err != nil {
	klog.Errorf("Failed to convert typed PropagationPolicy(%s/%s): %v", newPolicy.GetNamespace(), newPolicy.GetName(), err)
	return
	}

	if newPolicy.ExplicitPriority() < oldPolicy.ExplicitPriority() {
	d.HandleDeprioritizedPropagationPolicy(oldPolicy, newPolicy)
	}

reduce useless logic to improve dependencies distributor #4532

Are you sure you want to change the base?

reduce useless logic to improve dependencies distributor #4532

Conversation

whitewindmills commented Jan 10, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

whitewindmills commented Jan 12, 2024

XiShanYongYe-Chang commented Jan 12, 2024

codecov-commenter commented Jan 12, 2024 • edited Loading

Codecov Report

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

whitewindmills commented Jan 15, 2024

XiShanYongYe-Chang commented Jan 15, 2024

XiShanYongYe-Chang commented Jan 22, 2024

whitewindmills commented Feb 29, 2024

RainbowMango commented Feb 29, 2024

whitewindmills commented Apr 16, 2024

XiShanYongYe-Chang commented May 25, 2024

karmada-bot commented May 27, 2024

whitewindmills commented May 27, 2024

RainbowMango left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang May 28, 2024 • edited Loading

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

chaunceyjiang commented May 28, 2024

whitewindmills commented May 30, 2024

chaunceyjiang May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 12, 2024 •

edited

Loading

XiShanYongYe-Chang May 28, 2024 •

edited

Loading

chaunceyjiang May 31, 2024 •

edited

Loading