fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6157

CharlesQQ · 2025-02-24T11:18:44Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


`karmada-controller-manager`: Fixed the issue where the `detector` unnecessary updates for RB issue.

XiShanYongYe-Chang · 2025-02-24T12:29:22Z

Nice catching @CharlesQQ

We add a new finalizer karmada.io/binding-dependencies-distributor to the binding object corresponding to the resource to be followed in the dependencies distributor in #4983, this results in inconsistency of finalizers when the binding object is updated in the detector.

Based on this change, when the controller restarts, does the check on the binding object deepequal take effect? Do you have the test result?

CharlesQQ · 2025-02-24T12:54:51Z

does the check on the binding object deepequal take effect? Do you have the test result?

After testing, the detector will not update rb that has not changed during the restart, and the results are in line with expectations.

codecov-commenter · 2025-02-24T13:00:35Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.11%. Comparing base (006cf70) to head (5d2fe25).
Report is 4 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6157      +/-   ##
==========================================
+ Coverage   48.07%   48.11%   +0.03%     
==========================================
  Files         668      668              
  Lines       55327    55344      +17     
==========================================
+ Hits        26597    26626      +29     
+ Misses      26992    26982      -10     
+ Partials     1738     1736       -2

Flag	Coverage Δ
unittests	`48.11% <100.00%> (+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

Thanks~
Do we need a release note?
/lgtm

/cc @RainbowMango

RainbowMango

/assign

RainbowMango · 2025-02-25T08:23:16Z

pkg/util/label.go

+	if existFinalizers == nil {
+		return existFinalizers
+	}


I don't understand why it needs a return here, but I don't think it should be.
Consider one situation where len(existFinalizer) != 0 and len(newFinalizers) > 0.

RainbowMango · 2025-02-25T08:28:30Z

pkg/util/label.go

+	if equality.Semantic.DeepEqual(existFinalizerSets, newFinalizerSets) {
+		return existFinalizers
+	}
+	return newFinalizerSets.UnsortedList()


Here should return a sorted list, right?

The order should be

items in exist --> items in new(but not in exist)

I'll fix it, pls review again @RainbowMango

XiShanYongYe-Chang · 2025-02-25T09:32:26Z

I update the labels.
/kind bug
/remove-kind feature

CharlesQQ · 2025-02-25T10:21:43Z

/retest

XiShanYongYe-Chang · 2025-02-26T01:20:13Z

@CharlesQQ can you help add a release note? We need it when we cherry-pick it.

RainbowMango · 2025-02-26T02:22:10Z

pkg/detector/detector.go

@@ -560,6 +560,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
 				// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
 				bindingCopy.Annotations = util.DedupeAndMergeAnnotations(bindingCopy.Annotations, binding.Annotations)
 				bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
+				bindingCopy.Finalizers = util.DedupeAndMergeFinalizers(bindingCopy.Finalizers, binding.Finalizers)


Is this the only place that needs modification?
My understanding is that after the detector creates ResourceBindng/ClusterResourceBinding, downstream controllers could potentially append finalizers. Therefore, I think all places where updating RB/CRB should be fixed.

@XiShanYongYe-Chang What's your opinion?

I think all places where updating RB/CRB should be fixed

Oh

One more place:
https://github.com/karmada-io/karmada/blob/3ae9f5e99f3153ff52f90a63af004929a51ae63f/pkg/detector/detector.go#L470-L469

For the CRB, it is OK not to modify the CRB, because the dependency distribution does not add the related finalizer to the CRB resource.

Of course, the modification is better, which can avoid the same problem in the future.

RainbowMango · 2025-02-26T02:31:29Z

/remove-kind feature
/kind bug

karmada-bot · 2025-02-26T02:31:33Z

@RainbowMango: Those labels are not set on the issue: kind/feature

In response to this:

/remove-kind feature
/kind bug

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-sigs/prow repository.

Signed-off-by: chang.qiangqiang <[email protected]>

CharlesQQ · 2025-02-26T02:46:47Z

can you help add a release note? We need it when we cherry-pick it.

Which release should I cherry-pick to? @XiShanYongYe-Chang

XiShanYongYe-Chang · 2025-02-26T03:00:49Z

Which release should I cherry-pick to? @XiShanYongYe-Chang

We introduced this problem in v1.10, and we need to maintain the last three versions, so we need to cherry-pick it to the release-1.12, release-1.11 and release-1.10 branch.

RainbowMango

/lgtm
/approve

karmada-bot · 2025-02-26T03:59:17Z

[APPROVALNOTIFIER] This PR is APPROVED

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~~ [RainbowMango]

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

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 24, 2025

karmada-bot requested review from Garrybest and whitewindmills February 24, 2025 11:18

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

CharlesQQ force-pushed the detector-update-fix branch from 7ad6bca to 7f0ba3a Compare February 24, 2025 11:27

XiShanYongYe-Chang mentioned this pull request Feb 24, 2025

Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6154

Closed

CharlesQQ force-pushed the detector-update-fix branch from 7f0ba3a to 05dced1 Compare February 24, 2025 12:46

CharlesQQ changed the title ~~fix(detector): fix new binding object~~ fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. Feb 24, 2025

XiShanYongYe-Chang reviewed Feb 24, 2025

View reviewed changes

karmada-bot requested a review from RainbowMango February 24, 2025 13:11

karmada-bot assigned XiShanYongYe-Chang Feb 24, 2025

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2025

RainbowMango reviewed Feb 25, 2025

View reviewed changes

karmada-bot assigned RainbowMango Feb 25, 2025

RainbowMango requested changes Feb 25, 2025

View reviewed changes

CharlesQQ force-pushed the detector-update-fix branch from 05dced1 to 8798d97 Compare February 25, 2025 09:14

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2025

CharlesQQ force-pushed the detector-update-fix branch from 8798d97 to 1ba6616 Compare February 25, 2025 09:25

karmada-bot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Feb 25, 2025

RainbowMango reviewed Feb 26, 2025

View reviewed changes

CharlesQQ force-pushed the detector-update-fix branch from 1ba6616 to 3ae9f5e Compare February 26, 2025 02:26

fix(detector): fix new binding object

5d2fe25

Signed-off-by: chang.qiangqiang <[email protected]>

CharlesQQ force-pushed the detector-update-fix branch from 3ae9f5e to 5d2fe25 Compare February 26, 2025 02:41

RainbowMango approved these changes Feb 26, 2025

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2025

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2025

karmada-bot merged commit 7b29afb into karmada-io:master Feb 26, 2025
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6157

fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6157

CharlesQQ commented Feb 24, 2025 •

edited by RainbowMango

Loading

XiShanYongYe-Chang commented Feb 24, 2025

CharlesQQ commented Feb 24, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading

XiShanYongYe-Chang left a comment

RainbowMango left a comment

RainbowMango Feb 25, 2025

RainbowMango Feb 25, 2025

RainbowMango Feb 25, 2025

CharlesQQ Feb 25, 2025

XiShanYongYe-Chang commented Feb 25, 2025

CharlesQQ commented Feb 25, 2025

XiShanYongYe-Chang commented Feb 26, 2025

RainbowMango Feb 26, 2025

CharlesQQ Feb 26, 2025

RainbowMango Feb 26, 2025

CharlesQQ Feb 26, 2025

XiShanYongYe-Chang Feb 26, 2025

RainbowMango commented Feb 26, 2025

karmada-bot commented Feb 26, 2025

CharlesQQ commented Feb 26, 2025

XiShanYongYe-Chang commented Feb 26, 2025

RainbowMango left a comment

karmada-bot commented Feb 26, 2025

fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6157

fix(detector): Fix the invalid behavior in the detector to determine whether the binding object needs to be updated. #6157

Conversation

CharlesQQ commented Feb 24, 2025 • edited by RainbowMango Loading

XiShanYongYe-Chang commented Feb 24, 2025

CharlesQQ commented Feb 24, 2025 • edited Loading

codecov-commenter commented Feb 24, 2025 • edited Loading

Codecov Report

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

XiShanYongYe-Chang commented Feb 25, 2025

CharlesQQ commented Feb 25, 2025

XiShanYongYe-Chang commented Feb 26, 2025

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

RainbowMango commented Feb 26, 2025

karmada-bot commented Feb 26, 2025

CharlesQQ commented Feb 26, 2025

XiShanYongYe-Chang commented Feb 26, 2025

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Feb 26, 2025

CharlesQQ commented Feb 24, 2025 •

edited by RainbowMango

Loading

CharlesQQ commented Feb 24, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading