Clear XR field managers when upgrading to claim SSA #5555
Conversation
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
https://github.com/kubernetes/client-go/blob/v0.30.0/util/csaupgrade/upgrade.go#L165 This appears to be necessary to ensure the patch is rejected if we've derived our patch from a stale version of the object. Signed-off-by: Nic Cope <nicc@rk0n.org>
|
thanks, LGTM. |
|
This comment from @ravilr about also needing similar clearing of field managers in |
I'd rather @ravilr or I tackle that in a separate PR, rather than expand the scope of this one. |
| // | ||
| // Step 5: Eventually the XR reconciler updates a field (e.g. spec.resourceRefs) | ||
| // and becomes owner of that field. | ||
| func (u *PatchingManagedFieldsUpgrader) Upgrade(ctx context.Context, obj client.Object, ssaManager string, _ ...string) error { |
There was a problem hiding this comment.
nit: ManagedFieldsUpgrader is an internal interface and the only two implementations are this one and NopManagedFieldsUpgrader, none of them uses the csaManagers now, so we could as well directly drop it.
There was a problem hiding this comment.
I think it's useful to have two implementations (Nop and Patching) because it allows us to use the Nop implementation by default, but use the Patching implementation when the claim SSA feature flag is enabled.
There was a problem hiding this comment.
Nono, sure, I was talking about the csamanagers argument to the function.
Description of your changes
Fixes #5498
@ravlilr this PR achieves the same thing as #5550. I'm not sure whether this approach is better. For now I'm just experimenting.
Take a look at the commentary on the updated
Upgrademethod for details on how it works. The short version is that it removes all field managers. This causes the API server to create a specialbefore-first-applyfield manager, which it then also deletes (the second time it's called). The result appears to be a successful 'upgrade', with no shared field managers.I have:
make reviewableto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.