Conversation
dipti-pai
force-pushed
the
ssa-pre-dryrun-manager-takeover
branch
from
September 17, 2026 19:33
cad454f to
0ff5466
Compare
dipti-pai
marked this pull request as ready for review
September 17, 2026 20:23
matheuscscp
reviewed
Sep 18, 2026
matheuscscp
left a comment
matheuscscp
left a comment
Member
There was a problem hiding this comment.
Thanks @dipti-pai, looks good overall!
dipti-pai
force-pushed
the
ssa-pre-dryrun-manager-takeover
branch
from
September 18, 2026 16:27
0ff5466 to
75f2c19
Compare
Add ApplyCleanupOptions.FieldManagersBeforeDryRun, a separate opt-in list of field managers whose ownership is reclaimed before the server-side dry-run in Apply and ApplyAll. The existing FieldManagers takeover runs only after a successful dry-run (cleanupMetadata). When a stale or foreign manager co-owns a required field such that the merged object is invalid, the dry-run fails first and the takeover is never reached, leaving the object wedged on every reconcile. Taking over the named managers before the dry-run repairs ownership so the merge is valid. This is kept separate from FieldManagers because a pre-dry-run takeover mutates managedFields even if the apply later fails, unlike the post-dry-run cleanup. Only managers explicitly identified as problematic should be listed, so the default managers keep their transactional post-dry-run behavior. Signed-off-by: Dipti Pai <diptipai89@outlook.com> Assisted-by: GitHub Copilot/Claude Opus 4.8
dipti-pai
force-pushed
the
ssa-pre-dryrun-manager-takeover
branch
from
September 18, 2026 16:31
75f2c19 to
793ef82
Compare
Member
Author
Thanks @matheuscscp. Addressed the review comments. |
matheuscscp
approved these changes
Sep 18, 2026
matheuscscp
left a comment
matheuscscp
left a comment
Member
There was a problem hiding this comment.
LGTM! 🚀
Thanks @dipti-pai!
Lillecarl
added a commit
to Lillecarl/easykubenix
that referenced
this pull request
Sep 20, 2026
`deployment.fieldManager` stops the split happening again. It does nothing
for the objects that already crossed one, and nothing else can: the supported
way to drop a field is for its owning manager to apply again without it, and
the owning manager is the one that has stopped applying.
So this renames the old manager's Apply records onto the new one. The new
manager then owns what the old one did, and its next apply removes what it no
longer declares -- ordinary server-side apply, from then on.
**Renamed, never deleted.** A deleted record leaves its fields unowned, and
nothing removes an unowned field. That is the bug rather than the fix.
**Apply records only.** An `Update` record says a client wrote the field with
a plain update, which server-side apply does not consult for removal.
`kube-controller-manager`'s `status` is one, and renaming it would claim
ownership this cannot act on.
Two records for one manager, operation and apiVersion is a shape the API
server never produces, so a rename into an occupied slot merges the two
`fieldsV1` trees instead of appending. A `fieldsV1` value is a nested object
whose leaves are `{}`, so the union is a recursive merge.
Writing `metadata.managedFields` is the mechanism Kubernetes calls "highly
discouraged", and its own documentation names the case: `managedFields` in an
inconsistent state. This is that case, and a plain patch is the only way in --
an apply cannot write the field, because the API server computes it from the
apply.
Flux arrived at the same shape from the same failure: `fluxcd/pkg` reclaims
stale managers, but only after a dry-run the broken state cannot pass
(fluxcd/kustomize-controller#1722, fluxcd/pkg#1276). Two projects converging
is the argument for copying it rather than inventing a third.
Serial, and not a fan-out: it runs once per transition, and a readable log of
what it touched is worth more than the seconds concurrency would save.
Assisted-By: claude-opus-5[1m]
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
2 participants
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.
Add ApplyCleanupOptions.FieldManagersBeforeDryRun, a separate opt-in list of field managers whose ownership is reclaimed before the server-side dry-run in Apply and ApplyAll.
The existing FieldManagers takeover runs only after a successful dry-run (cleanupMetadata). When a stale or foreign manager co-owns a required field such that the merged object is invalid, the dry-run fails first and the takeover is never reached, leaving the object wedged on every reconcile. Taking over the named managers before the dry-run repairs ownership so the merge is valid.
This is kept separate from FieldManagers because a pre-dry-run takeover mutates managedFields even if the apply later fails, unlike the post-dry-run cleanup. Only managers explicitly identified as problematic should be listed, so the default managers keep their transactional post-dry-run behavior.
Assisted-by: GitHub Copilot/Claude Opus 4.8
Fixes: fluxcd/kustomize-controller#1722