feat: avoid redundant process because of claim metadata change

[vie-serendipity]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR solves a problem where, when claim metadata is added to an object, the object should be processed next time because the update event will enqueue it again.

Every object will be handled by detector twice at least even though nothing changed. And accordingly every rb will be handled twice by rb controller. This should be optimized.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[gemini-code-assist]

Summary of Changes

Hello @vie-serendipity, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly improves the efficiency of the resource detector by eliminating redundant processing. Previously, when an object's claim metadata was updated, it would trigger a full re-processing cycle even if no other changes occurred. The new changes introduce a flag to detect metadata-only updates, allowing the system to short-circuit the processing and rely on the subsequent update event to handle the object, thereby reducing unnecessary computational load and improving overall performance.

Highlights

• Optimized Object Processing: Introduced a mechanism to prevent redundant processing of objects by the detector when only claim metadata is updated. This avoids unnecessary re-enqueuing and processing cycles.
• Enhanced Claim Policy Functions: The ClaimPolicyForObject and ClaimClusterPolicyForObject functions now return an additional boolean value indicating whether the object's claim metadata was actually modified during the call.
• Early Exit for Metadata-Only Updates: The ApplyPolicy and ApplyClusterPolicy functions in the resource detector now check the new boolean return value. If only claim metadata was added (and thus the object will be re-enqueued by an update event), the function returns early, preventing further redundant processing.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces an optimization to avoid redundant processing when claim metadata is added to an object. By modifying ClaimPolicyForObject and ClaimClusterPolicyForObject to return a boolean indicating if a claim was newly added, the ApplyPolicy and ApplyClusterPolicy functions can now exit early, knowing that the object update will trigger a new reconciliation. This is a sensible performance improvement. My review focuses on ensuring the documentation is updated to reflect these signature changes, in accordance with the repository's style guide.

[gemini-code-assist]

The function signature for ClaimPolicyForObject has been updated to return a boolean value indicating whether a claim was newly made, but the function's comment has not been updated to reflect this change. According to the style guide, all exported functions must be documented with clear and concise comments describing their purpose and behavior, including return values.

Suggested change

	// ClaimPolicyForObject set policy identifier which the object associated with.
	// It will add policy labels and annotations to the object, and update the obj with the content returned by the Server, ensuring that the obj used when generating the binding is the latest.
	// ClaimPolicyForObject sets the policy identifier for an object by adding labels/annotations and updating it on the server.
	// It returns the policy ID, a boolean indicating if a claim was newly added, and an error. The update ensures the latest object version is used for binding generation.

References

1. All exported functions, methods, structs, and interfaces must be documented with clear and concise comments describing their purpose and behavior. ^(link)

[gemini-code-assist]

Similar to ClaimPolicyForObject, the function signature for ClaimClusterPolicyForObject has changed, but its comment has not been updated. Please update the comment to describe the new boolean return value, as required by the style guide.

Suggested change

	// ClaimClusterPolicyForObject set cluster identifier which the object associated with.
	// It will add policy labels and annotations to the object, and update the obj with the content returned by the Server, ensuring that the obj used when generating the binding is the latest.
	// ClaimClusterPolicyForObject sets the cluster policy identifier for an object by adding labels/annotations and updating it on the server.
	// It returns the policy ID, a boolean indicating if a claim was newly added, and an error. The update ensures the latest object version is used for binding generation.

References

1. All exported functions, methods, structs, and interfaces must be documented with clear and concise comments describing their purpose and behavior. ^(link)

[RainbowMango]

/assign
Nice catch! @vie-serendipity . Could you please look at the failing tests?

[vie-serendipity]

@RainbowMango
When an object is selected by pp, it will be processed. The detector will label and annotate this object, after which the object is processed one more time.

I want it to be processed only once, so I proposed this PR. Only the objects with karmada labels and annotations will be processed.

But in the scenario where ActivationPreference is lazy, if the first time it is not processed and resourceChangedByKarmada will be true on the second time, it will not be processed, so rb will not even be created. This is the reason why e2e test failed.

If I want to achieve my goal right now, I can actually set ActivationPreference to Lazy. Right?

[RainbowMango]

But in the scenario where ActivationPreference is lazy, if the first time it is not processed and resourceChangedByKarmada will be true on the second time, it will not be processed, so rb will not even be created. This is the reason why e2e test failed.

Do you mean which E2E test?
Is this one: https://github.com/karmada-io/karmada/actions/runs/20917907135/job/60096547228?pr=7103#step:6:197

• [FAILED] [420.054 seconds]
Karmadactl logs testing Test karmadactl logs for existing pod [BeforeEach] should get logs from a specific container in the pod
  [BeforeEach] /home/runner/work/karmada/karmada/test/e2e/suites/base/karmadactl_test.go:642
  [It] /home/runner/work/karmada/karmada/test/e2e/suites/base/karmadactl_test.go:670

  Captured StdOut/StdErr Output >>
  I0112 11:55:36.214755   56846 pod.go:60] Waiting for pod(karmadatest-n8gbz/pod-btv75) synced on cluster(member1)
  << Captured StdOut/StdErr Output

  [FAILED] Timed out after 420.000s.
  Expected
      <bool>: false
  to equal
      <bool>: true
  In [BeforeEach] at: /home/runner/work/karmada/karmada/test/e2e/framework/pod.go:68 @ 01/12/26 12:02:36.215

  Full Stack Trace
    github.com/karmada-io/karmada/test/e2e/framework.WaitPodPresentOnClusterFitWith({0xc000117579, 0x7}, {0xc000c0e0f0, 0x11}, {0xc000b2c0b7, 0x9}, 0x5654f08)
    	/home/runner/work/karmada/karmada/test/e2e/framework/pod.go:68 +0x634
    github.com/karmada-io/karmada/test/e2e/suites/base.init.func2.2({0xc000c0e0f0, 0x11}, {0xc000b2c0b7, 0x9})
    	/home/runner/work/karmada/karmada/test/e2e/suites/base/karmadactl_test.go:630 +0x6b
    github.com/karmada-io/karmada/test/e2e/suites/base.init.func2.3.1()
    	/home/runner/work/karmada/karmada/test/e2e/suites/base/karmadactl_test.go:655 +0xab7

If I want to achieve my goal right now, I can actually set ActivationPreference to Lazy. Right?

No, I don't think so.

1. ActivationPreference was not designed for this case
2. The ActivationPreference is an experimental feature, which might be removed in the future.

Please refer to the side effects from the Warning section of https://karmada.io/docs/next/userguide/scheduling/propagation-policy#activationpreference.

[karmada-bot]

[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 Code Review Process.

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

Details

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

[vie-serendipity]

No, I mean this one, which is about lazy_activation_policy_test.
https://github.com/karmada-io/karmada/actions/runs/21019184471/job/60430861139?pr=7103#step:6:2069

For karmadactl_test you refer to, I think that would be related to githu workflow environment. Because this time it passed.

[RainbowMango]

I see, will look at it.

[RainbowMango]

But in the scenario where ActivationPreference is lazy, if the first time it is not processed and resourceChangedByKarmada will be true on the second time, it will not be processed, so rb will not even be created. This is the reason why e2e test failed.

I see.
It is a tricky case and I don't have a clue about it so far.

Maybe, the claimed operation should be ignored(skip reconciliation), no matter considering ActivationPreference.

[zhzhuang-zju]

This PR seems to be stuck—I'll take a look.

[zhzhuang-zju]

There are generally two approaches to skip redundant processing:

• Terminate the current reconcile and defer the remaining steps to the next reconcile cycle.
  This is the approach adopted in this PR. However, it is currently affected by the lazy ActivationPreference mechanism and may fail in certain scenarios. I don't recommend modifying code related to lazy ActivationPreference, as it's difficult to assess its true impact.

• Complete the current reconcile fully and skip the next one.
  Specifically, if only the claimedMetadata of the object has changed, skip the current reconcile. Currently, eventfilter.SpecificationChanged already filters out many changes that aren't related to the specification. How about extending eventfilter.SpecificationChanged to skip redundant reconciles triggered by changes to claimedMetadata? Note that eventfilter.SpecificationChanged is also used by other controllers, so the impact needs further evaluation.

[RainbowMango]

Specifically, if only the claimedMetadata of the object has changed, skip the current reconcile. Currently, eventfilter.SpecificationChanged already filters out many changes that aren't related to the specification. How about extending eventfilter.SpecificationChanged to skip redundant reconciles triggered by changes to claimedMetadata? Note that eventfilter.SpecificationChanged is also used by other controllers, so the impact needs further evaluation.

This approach is worth exploring further. Just a reminder, we have some logic where relying on the behavior, I mean, once a PropagationPolicy is removed, the detector will cleanup the claimed metadata from the resource template and expect a following reconcile to match another PropagationPolicy.

[zhzhuang-zju]

Thanks for your reminder—this is indeed an issue. It could likely be resolved by distinguishing whether the operation is specifically adding claimMetadata. However, we still need to conduct a systematic analysis to avoid introducing unintended side effects.

[vie-serendipity]

It could likely be resolved by distinguishing whether the operation is specifically adding claimMetadata.

If there are no further changes to resources, the member cluster resources will lack claim metadata. I think we should maintain consistency; in addition, in some cases the claim metadata on resources is also helpful for troubleshooting.