re-fetch work status inside RetryOnConflict loop#7694
Conversation
Signed-off-by: Varsha <varsha@Varshas-MacBook-Pro.local>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! 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 addresses a race condition in the Karmada controller manager where the status of ResourceBinding and ClusterResourceBinding objects could become stale during reconciliation retries. By relocating the status calculation logic into the retry loop, the system now guarantees that status updates are based on the latest state, ensuring accuracy even when concurrent modifications occur. Additionally, the PR includes a robust unit test to validate this behavior and improves the reliability of the codegen cleanup script. Highlights
Using Gemini Code AssistThe 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
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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. Footnotes
|
karmada-bot
added
the
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the work status aggregation logic in pkg/util/helper/workstatus.go by moving the retrieval and assembly of work statuses inside the retry.RetryOnConflict block. This ensures that retries operate on the most up-to-date work status data in case of conflicts. Additionally, a new unit test was added to verify this retry behavior under conflict conditions, and a minor permission fix was applied to the codegen cleanup script. The review feedback requests adding compile-time interface compliance checks for the newly introduced test helper structs (conflictFakeClient and conflictFakeStatusWriter) to align with the repository's style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
| } | ||
|
|
||
| type conflictFakeClient struct { |
There was a problem hiding this comment.
According to the repository style guide, any struct that explicitly implements an interface must include a compile-time interface compliance check using the pattern var _ InterfaceName = &StructName{}. Please add this check for conflictFakeClient.
| type conflictFakeClient struct { | |
| var _ client.Client = &conflictFakeClient{} | |
| type conflictFakeClient struct { |
References
- Any struct that explicitly implements an interface must include a compile-time interface compliance check using the pattern: var _ InterfaceName = &StructName{} (link)
| } | ||
| } | ||
|
|
||
| type conflictFakeStatusWriter struct { |
There was a problem hiding this comment.
According to the repository style guide, any struct that explicitly implements an interface must include a compile-time interface compliance check using the pattern var _ InterfaceName = &StructName{}. Please add this check for conflictFakeStatusWriter.
| type conflictFakeStatusWriter struct { | |
| var _ client.SubResourceWriter = &conflictFakeStatusWriter{} | |
| type conflictFakeStatusWriter struct { |
References
- Any struct that explicitly implements an interface must include a compile-time interface compliance check using the pattern: var _ InterfaceName = &StructName{} (link)
There was a problem hiding this comment.
Pull request overview
This PR fixes a conflict-retry race in AggregateResourceBindingWorkStatus / AggregateClusterResourceBindingWorkStatus where FullyApplied and AggregatedStatus could be computed from a stale snapshot when Status().Update() hit a conflict and retried. It also improves the codegen cleanup script to better handle read-only files under _go/.
Changes:
- Recomputes the Work list and aggregated status inside the
UpdateStatusmutate callback that runs withinRetryOnConflict, ensuring each retry uses the latest binding spec and current Works. - Adds a unit test that simulates a conflict + concurrent spec update (scheduler adding a new target cluster) and asserts
FullyApplied=Falseafter retry. - Updates
hack/update-codegen.shcleanup tochmod -R u+wbefore removing_go/, avoiding failures on read-only cache artifacts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/util/helper/workstatus.go |
Moves work fetching/status assembly into the retry mutation to avoid stale FullyApplied/AggregatedStatus on conflict retries. |
pkg/util/helper/workstatus_test.go |
Adds a conflict-retry unit test via a fake client status-writer wrapper to validate the race condition fix. |
hack/update-codegen.sh |
Ensures _go/ cleanup can delete read-only toolchain/cache files by making them user-writable first. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7694 +/- ##
==========================================
- Coverage 42.06% 42.05% -0.02%
==========================================
Files 879 879
Lines 54831 54831
==========================================
- Hits 23063 23057 -6
- Misses 30023 30028 +5
- Partials 1745 1746 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix a race condition where the
FullyAppliedcondition andAggregatedStatusof aResourceBindingorClusterResourceBindingcan become stale when astatus update conflicts and retries.
Previously,
GetWorksByBindingIDandassembleWorkStatuswere called once before theRetryOnConflictloop. If the update failed due to a conflict (e.g., the scheduler concurrently updatedSpec.Clusters), the retry loop would re-fetch the binding object but still apply a stale status snapshot, potentially settingFullyApplied=Trueeven though new target clusters had been added but theirWorkobjects were not yet applied.The fix moves work fetching and status assembly inside the
UpdateStatusmutation callback, ensuring that on every retry attempt, the status is computed from the latest binding spec and current work objects.Which issue(s) this PR fixes:
Fixes #7227
Special notes for your reviewer:
A new unit test
TestAggregateResourceBindingWorkStatusRetryOnConflicthas been added that simulates a concurrent scheduler update (adding a second target cluster) during a conflict retry, and verifies thatFullyAppliedis correctly set toFalseafter the retry.Also fixed the
cleanup()function inhack/update-codegen.shto runchmod -R u+wbeforerm -rfso the read-only Go toolchain cache files created in_go/are properly cleaned up.Does this PR introduce a user-facing change?: