-
Notifications
You must be signed in to change notification settings - Fork 21
[eno] Update Normalization Field Manager Logic #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ruinan-liu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respond to comments
ruinan-liu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replied to comments
jveski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending Allan's +1
ruinan-liu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reply to comments
|
The early return true, nil after NormalizeConflictingManagers is problematic. That step only adjusts managedFields to unblock SSA; it does not represent a durable desired-state change. Returning early causes the controller to requeue before reaching the SSA dry-run/apply path, so the resource is never actually applied. Since managedFields normalization alone may not converge, this can also lead to repeated requeues. We need to continue into the SSA flow in the same reconciliation so ownership converges and the next reconcile observes a stable, unmodified resource. |
ruinan-liu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments
- Only migrate `f:metadata:annotation`, `f:metadata:labels`, and `f:spec` during the migration. Tested e2e pipeline passed: https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448228&view=results https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448289&view=results https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448264&view=results https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448364&view=results https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448306&view=results https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149449489&view=results https://dev.azure.com/msazure/CloudNativeCompute/_build/results?buildId=149448405&view=results
Cherry-picking below 2 commits to release branch 1. #552 2. #555 --------- Co-authored-by: yemreinci <[email protected]> Co-authored-by: Yunus Inci <[email protected]>
f:metadata:annotation,f:metadata:labels, andf:specduring the migration.Tested e2e pipeline passed:
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448228&view=results
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448289&view=results
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448264&view=results
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448364&view=results
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149448306&view=results
https://msazure.visualstudio.com/CloudNativeCompute/_build/results?buildId=149449489&view=results
https://dev.azure.com/msazure/CloudNativeCompute/_build/results?buildId=149448405&view=results