-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🐛 Fix ClusterClass MachinePool rollout on BootstrapConfig changes #13110
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Bharath Nallapeta <[email protected]>
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@richardcase @AndiDog |
| desired: desiredMP.BootstrapObject, | ||
| }); err != nil { | ||
| if createdInfra { | ||
| infrastructureMachinePoolCleanupFunc = func() { |
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.
Just curious, why are we creating an inline function instead of defining it somewhere else? Not as familiar with Kubernetes code bases so disregard if there there's a good reason.
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.
a. I'd like to stay consistent with the current style where this is used in multiple other places already.
b. I think inline functions help us keep the logic right where we need to make the decision (should we cleanup or not). The alternative would require us to send all the context, obj, logger and handle the errors differently and this feels a bit too much for the purpose.
|
Just finished testing the fix manually with CAPD and it's working as expected! Summary:
$ kubectl get kubeadmconfig -l cluster.x-k8s.io/cluster-name=test-cluster
NAME AGE
test-cluster-mp-0-2krrr 58m # old config
test-cluster-mp-0-sw2m8 2m # new config (rotated!)
$ kubectl get machinepool $MACHINEPOOL_NAME -o jsonpath='{.spec.template.spec.bootstrap.configRef.name}'
test-cluster-mp-0-sw2m8 # <-- reference updated correctlyWhy CAPD? Next steps: (Detailed testing steps available if anyone wants to reproduce) |
What this PR does / why we need it:
ClusterClass-managed MachinePools were not triggering node rollouts when BootstrapConfig or InfrastructureMachinePool templates changed. This happened because
updateMachinePool()usedreconcileReferencedObject()which patches objects in-place without changing their names. Infrastructure providers (like CAPA) watch forconfigRef.namechanges to trigger rollouts, but since the name never changed, no rollout occurred.This PR changes
updateMachinePool()to usereconcileReferencedTemplate()instead. This is the same approach used byupdateMachineDeployment().Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #10496
/area machinepool
/area clusterclass