fix: retry SSA field manager migration on conflict#567
Open
perdasilva wants to merge 1 commit into
Open
Conversation
Contributor
Walkthrough
ChangesManaged-fields migration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
==========================================
- Coverage 82.98% 82.95% -0.03%
==========================================
Files 32 32
Lines 2744 2751 +7
==========================================
+ Hits 2277 2282 +5
- Misses 340 341 +1
- Partials 127 128 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
machinery/objects.go (1)
679-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for conflict refresh and patch recomputation.
Mock the first patch as a 409, update the reader’s object/resourceVersion, then assert that the second patch is computed from the refreshed object and succeeds. The existing no-patch test does not exercise the new retry path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@machinery/objects.go` around lines 679 - 706, Add a regression test for the retry flow surrounding UpgradeManagedFieldsPatch: make the first writer.Patch call return a conflict, update the reader’s object and resourceVersion during that conflict, then verify the second attempt recomputes the patch from the refreshed object and succeeds. Keep the existing no-patch test unchanged, and assert both the refreshed input and successful retry outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machinery/objects.go`:
- Line 679: Update the RetryOnConflict call in the visible retry callback to use
retry.DefaultBackoff instead of retry.DefaultRetry, preserving the existing
callback and conflict-handling behavior.
- Around line 708-714: Update ObjectEngine.refreshObject to always use an
uncached reader during conflict recovery, rather than falling back to e.cache.
Ensure the unfilteredReader is available for migration retries and use it for
the ObjectKeyFromObject lookup and refresh Get operation, preserving the
existing error propagation.
---
Nitpick comments:
In `@machinery/objects.go`:
- Around line 679-706: Add a regression test for the retry flow surrounding
UpgradeManagedFieldsPatch: make the first writer.Patch call return a conflict,
update the reader’s object and resourceVersion during that conflict, then verify
the second attempt recomputes the patch from the refreshed object and succeeds.
Keep the existing no-patch test unchanged, and assert both the refreshed input
and successful retry outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 943095d3-3edb-457d-bfc0-b00450058418
📒 Files selected for processing (1)
machinery/objects.go
perdasilva
force-pushed
the
fix/retry-ssa-field-manager-migration-on-conflict
branch
from
b12610e to
9712d8f
Compare
When an object is created and immediately mutated by another controller (e.g. the CRD controller adding status), the field-manager migration patch carries a stale resourceVersion and gets rejected with a 409 Conflict. Use retry.RetryOnConflict with DefaultBackoff (exponential, for unrelated controller conflicts) to re-fetch the object and retry the patch, avoiding spurious reconcile errors bubbling up to the user. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
perdasilva
force-pushed
the
fix/retry-ssa-field-manager-migration-on-conflict
branch
from
9e6f776 to
aabcb11
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@machinery/objects_test.go`:
- Around line 239-245: Add cache.AssertExpectations(t) to
TestObjectEngine_MigrateFieldManagersToSSA_NoPatch after the migration call
completes, ensuring the configured Get expectations are verified while
preserving the existing mock setup and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6c012ef3-6cf1-4acd-9d9e-43803cb5426f
📒 Files selected for processing (2)
machinery/objects.gomachinery/objects_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- machinery/objects.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
migrateFieldManagersToSSApatches the field managers to be SSA-compatible usingcsaupgrade. If another controller (e.g. theapiextensionscontroller updating CRD status) modifies the object between the create response and that patch, the API server rejects the patch with a 409 Conflict due to a staleresourceVersion.retry.RetryOnConflict(retry.DefaultBackoff, ...).DefaultBackoffuses exponential backoff (factor 5×), which is the correct choice for conflicts caused by an unrelated controller — as opposed toDefaultRetry(linear), which is for multiple clients racing on the same resource. On conflict, the object is re-fetched (viaunfilteredReaderif available, otherwisecache) to get the latestresourceVersionbefore recomputing and re-applying the patch.refreshObjecthelper used by the retry closure, called unconditionally at the top of each iteration.Motivation
Users were seeing transient errors of the form:
This is a TOCTOU race that is especially common with CRDs because the
apiextensionscontroller patches status immediately after creation. The retry absorbs the transient conflict without any user-visible error.Test plan
./do dev:unit)🤖 Generated with Claude Code