Skip to content

🐛 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.

Sign up for GitHub

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

Draft
bnallapeta wants to merge 1 commit into
base: main
Choose a base branch
from
+130 −37
Draft

🐛 Fix ClusterClass MachinePool rollout on BootstrapConfig changes #13110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 57 additions & 14 deletions internal/controllers/topology/cluster/reconcile_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,7 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp *
// Best effort cleanup of the InfrastructureMachinePool;
// If this fails, the object will be garbage collected when the cluster is deleted.
if err := r.Client.Delete(ctx, mp.InfrastructureMachinePoolObject); err != nil {
infraLog.Info("WARNING! Failed to cleanup InfrastructureMachinePoolObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.")
infraLog.Error(err, "WARNING! Failed to cleanup InfrastructureMachinePoolObject for MachinePool while handling creation error. The object will be garbage collected when the cluster is deleted.")
}
}
}
Expand Down Expand Up @@ -1031,7 +1031,7 @@ func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp *
return clientutil.WaitForObjectsToBeAddedToTheCache(ctx, r.Client, "MachinePool creation", mp.Object)
}

// updateMachinePool updates a MachinePool. Also updates the corresponding objects if necessary.
// updateMachinePool updates a MachinePool. Also rotates the corresponding objects if necessary.
func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, mpTopologyName string, currentMP, desiredMP *scope.MachinePoolState) error {
log := ctrl.LoggerFrom(ctx).WithValues("MachinePool", klog.KObj(desiredMP.Object),
"machinePoolTopology", mpTopologyName)
Expand All @@ -1044,27 +1044,67 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, mpTo
}

cluster := s.Current.Cluster
infraCtx := ctrl.LoggerInto(ctx, log.WithValues(desiredMP.InfrastructureMachinePoolObject.GetKind(), klog.KObj(desiredMP.InfrastructureMachinePoolObject)))
if _, err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{
cluster: cluster,
current: currentMP.InfrastructureMachinePoolObject,
desired: desiredMP.InfrastructureMachinePoolObject,
}); err != nil {

// Reconcile the InfrastructureMachinePool object, rotating it if necessary.
infraLog := log.WithValues(desiredMP.InfrastructureMachinePoolObject.GetKind(), klog.KObj(desiredMP.InfrastructureMachinePoolObject))
infraCtx := ctrl.LoggerInto(ctx, infraLog)
infrastructureMachinePoolCleanupFunc := func() {}
createdInfra, err := r.reconcileReferencedTemplate(infraCtx, reconcileReferencedTemplateInput{
cluster: cluster,
ref: &desiredMP.Object.Spec.Template.Spec.InfrastructureRef,
current: currentMP.InfrastructureMachinePoolObject,
desired: desiredMP.InfrastructureMachinePoolObject,
templateNamePrefix: topologynames.InfrastructureMachinePoolNamePrefix(cluster.Name, mpTopologyName),
compatibilityChecker: check.ObjectsAreCompatible,
})
if err != nil {
return errors.Wrapf(err, "failed to reconcile MachinePool %s", klog.KObj(currentMP.Object))
}

bootstrapCtx := ctrl.LoggerInto(ctx, log.WithValues(desiredMP.BootstrapObject.GetKind(), klog.KObj(desiredMP.BootstrapObject)))
if _, err := r.reconcileReferencedObject(bootstrapCtx, reconcileReferencedObjectInput{
cluster: cluster,
current: currentMP.BootstrapObject,
desired: desiredMP.BootstrapObject,
}); err != nil {
if createdInfra {
infrastructureMachinePoolCleanupFunc = func() {
Copy link

@mpinchover mpinchover Dec 6, 2025

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.

Copy link
Contributor Author

@bnallapeta bnallapeta Dec 13, 2025

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.

// Best effort cleanup of the InfrastructureMachinePool;
// If this fails, the object will be garbage collected when the cluster is deleted.
if err := r.Client.Delete(ctx, desiredMP.InfrastructureMachinePoolObject); err != nil {
infraLog.Error(err, "WARNING! Failed to cleanup InfrastructureMachinePool for MachinePool while handling update error. The object will be garbage collected when the cluster is deleted.")
}
}
}

// Reconcile the BootstrapConfig object, rotating it if necessary.
bootstrapLog := log.WithValues(desiredMP.BootstrapObject.GetKind(), klog.KObj(desiredMP.BootstrapObject))
bootstrapCtx := ctrl.LoggerInto(ctx, bootstrapLog)
bootstrapCleanupFunc := func() {}
createdBootstrap, err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{
cluster: cluster,
ref: &desiredMP.Object.Spec.Template.Spec.Bootstrap.ConfigRef,
current: currentMP.BootstrapObject,
desired: desiredMP.BootstrapObject,
templateNamePrefix: topologynames.BootstrapConfigNamePrefix(cluster.Name, mpTopologyName),
compatibilityChecker: check.ObjectsAreInTheSameNamespace,
})
if err != nil {
// Best effort cleanup of the InfrastructureMachinePool (only on rotation).
infrastructureMachinePoolCleanupFunc()
return errors.Wrapf(err, "failed to reconcile MachinePool %s", klog.KObj(currentMP.Object))
}

if createdBootstrap {
bootstrapCleanupFunc = func() {
// Best effort cleanup of the BootstrapConfig;
// If this fails, the object will be garbage collected when the cluster is deleted.
if err := r.Client.Delete(ctx, desiredMP.BootstrapObject); err != nil {
bootstrapLog.Error(err, "WARNING! Failed to cleanup BootstrapConfig for MachinePool while handling update error. The object will be garbage collected when the cluster is deleted.")
}
}
}

// Check differences between current and desired MachinePool, and eventually patch the current object.
patchHelper, err := structuredmerge.NewServerSidePatchHelper(ctx, currentMP.Object, desiredMP.Object, r.Client, r.ssaCache)
if err != nil {
// Best effort cleanup of the InfrastructureMachinePool & BootstrapConfig (only on rotation).
infrastructureMachinePoolCleanupFunc()
bootstrapCleanupFunc()
return errors.Wrapf(err, "failed to create patch helper for MachinePool %s", klog.KObj(currentMP.Object))
}
if !patchHelper.HasChanges() {
Expand All @@ -1080,6 +1120,9 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, s *scope.Scope, mpTo
}
modifiedResourceVersion, err := patchHelper.Patch(ctx)
if err != nil {
// Best effort cleanup of the InfrastructureMachinePool & BootstrapConfig (only on rotation).
infrastructureMachinePoolCleanupFunc()
bootstrapCleanupFunc()
return errors.Wrapf(err, "failed to patch MachinePool %s", klog.KObj(currentMP.Object))
}
r.recorder.Eventf(cluster, corev1.EventTypeNormal, updateEventReason, "Updated MachinePool %q%s", klog.KObj(currentMP.Object), logMachinePoolVersionChange(currentMP.Object, desiredMP.Object))
Expand Down
96 changes: 73 additions & 23 deletions internal/controllers/topology/cluster/reconcile_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2761,16 +2761,30 @@ func TestReconcileMachinePools(t *testing.T) {
mp9WithInstanceSpecificTemplateMetadata := newFakeMachinePoolTopologyState("mp-9m", infrastructureMachinePool9m, bootstrapConfig9m)
mp9WithInstanceSpecificTemplateMetadata.Object.Spec.Template.Labels = map[string]string{"foo": "bar"}

// mp10: Test metadata-only changes (labels/annotations) should NOT trigger rotation
infrastructureMachinePool10 := builder.TestInfrastructureMachinePool(metav1.NamespaceDefault, "infrastructure-machinepool-10").Build()
bootstrapConfig10 := builder.TestBootstrapConfig(metav1.NamespaceDefault, "bootstrap-config-10").Build()
mp10 := newFakeMachinePoolTopologyState("mp-10", infrastructureMachinePool10, bootstrapConfig10)
infrastructureMachinePool10WithMetadataChanges := infrastructureMachinePool10.DeepCopy()
infrastructureMachinePool10WithMetadataChanges.SetLabels(map[string]string{"new-label": "new-value"})
infrastructureMachinePool10WithMetadataChanges.SetAnnotations(map[string]string{"new-annotation": "new-value"})
bootstrapConfig10WithMetadataChanges := bootstrapConfig10.DeepCopy()
bootstrapConfig10WithMetadataChanges.SetLabels(map[string]string{"new-label": "new-value"})
bootstrapConfig10WithMetadataChanges.SetAnnotations(map[string]string{"new-annotation": "new-value"})
mp10WithMetadataOnlyChanges := newFakeMachinePoolTopologyState("mp-10", infrastructureMachinePool10WithMetadataChanges, bootstrapConfig10WithMetadataChanges)

tests := []struct {
name string
current []*scope.MachinePoolState
currentOnlyAPIServer []*scope.MachinePoolState
desired []*scope.MachinePoolState
upgradeTracker *scope.UpgradeTracker
want []*scope.MachinePoolState
wantInfrastructureMachinePoolObjectUpdate map[string]bool
wantBootstrapObjectUpdate map[string]bool
wantErr bool
name string
current []*scope.MachinePoolState
currentOnlyAPIServer []*scope.MachinePoolState
desired []*scope.MachinePoolState
upgradeTracker *scope.UpgradeTracker
want []*scope.MachinePoolState
wantInfrastructureMachinePoolObjectUpdate map[string]bool
wantBootstrapObjectUpdate map[string]bool
wantInfrastructureMachinePoolObjectRotation map[string]bool
wantBootstrapObjectRotation map[string]bool
wantErr bool
}{
{
name: "Should create desired MachinePool if the current does not exists yet",
Expand Down Expand Up @@ -2807,7 +2821,8 @@ func TestReconcileMachinePools(t *testing.T) {
current: []*scope.MachinePoolState{mp2},
desired: []*scope.MachinePoolState{mp2WithChangedInfrastructureMachinePool},
want: []*scope.MachinePoolState{mp2WithChangedInfrastructureMachinePool},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-2": true},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-2": true},
wantInfrastructureMachinePoolObjectRotation: map[string]bool{"mp-2": true},
wantErr: false,
},
{
Expand All @@ -2816,16 +2831,18 @@ func TestReconcileMachinePools(t *testing.T) {
desired: []*scope.MachinePoolState{mp2WithChangedInfrastructureMachinePool},
upgradeTracker: upgradeTrackerWithmp2PendingUpgrade,
want: []*scope.MachinePoolState{mp2},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-2": false},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-2": false},
wantInfrastructureMachinePoolObjectRotation: map[string]bool{"mp-2": false},
wantErr: false,
},
{
name: "Should update BootstrapConfig",
current: []*scope.MachinePoolState{mp3},
desired: []*scope.MachinePoolState{mp3WithChangedbootstrapConfig},
want: []*scope.MachinePoolState{mp3WithChangedbootstrapConfig},
wantBootstrapObjectUpdate: map[string]bool{"mp-3": true},
wantErr: false,
name: "Should update BootstrapConfig",
current: []*scope.MachinePoolState{mp3},
desired: []*scope.MachinePoolState{mp3WithChangedbootstrapConfig},
want: []*scope.MachinePoolState{mp3WithChangedbootstrapConfig},
wantBootstrapObjectUpdate: map[string]bool{"mp-3": true},
wantBootstrapObjectRotation: map[string]bool{"mp-3": true},
wantErr: false,
},
{
name: "Should fail update MachinePool because of changed BootstrapConfig kind",
Expand All @@ -2838,9 +2855,11 @@ func TestReconcileMachinePools(t *testing.T) {
current: []*scope.MachinePoolState{mp4},
desired: []*scope.MachinePoolState{mp4WithChangedObjects},
want: []*scope.MachinePoolState{mp4WithChangedObjects},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-4": true},
wantBootstrapObjectUpdate: map[string]bool{"mp-4": true},
wantErr: false,
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-4": true},
wantBootstrapObjectUpdate: map[string]bool{"mp-4": true},
wantInfrastructureMachinePoolObjectRotation: map[string]bool{"mp-4": true},
wantBootstrapObjectRotation: map[string]bool{"mp-4": true},
wantErr: false,
},
{
name: "Should fail update MachinePool because of changed InfrastructureMachinePool kind",
Expand All @@ -2866,9 +2885,11 @@ func TestReconcileMachinePools(t *testing.T) {
current: []*scope.MachinePoolState{mp8Update, mp8Delete},
desired: []*scope.MachinePoolState{mp8Create, mp8UpdateWithChangedObjects},
want: []*scope.MachinePoolState{mp8Create, mp8UpdateWithChangedObjects},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-8-update": true},
wantBootstrapObjectUpdate: map[string]bool{"mp-8-update": true},
wantErr: false,
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-8-update": true},
wantBootstrapObjectUpdate: map[string]bool{"mp-8-update": true},
wantInfrastructureMachinePoolObjectRotation: map[string]bool{"mp-8-update": true},
wantBootstrapObjectRotation: map[string]bool{"mp-8-update": true},
wantErr: false,
},
{
name: "Enforce template metadata",
Expand All @@ -2877,6 +2898,17 @@ func TestReconcileMachinePools(t *testing.T) {
want: []*scope.MachinePoolState{mp9},
wantErr: false,
},
{
name: "Should update but not rotate InfrastructureMachinePool and BootstrapConfig on metadata-only changes",
current: []*scope.MachinePoolState{mp10},
desired: []*scope.MachinePoolState{mp10WithMetadataOnlyChanges},
want: []*scope.MachinePoolState{mp10WithMetadataOnlyChanges},
wantInfrastructureMachinePoolObjectUpdate: map[string]bool{"mp-10": true},
wantBootstrapObjectUpdate: map[string]bool{"mp-10": true},
wantInfrastructureMachinePoolObjectRotation: map[string]bool{"mp-10": false},
wantBootstrapObjectRotation: map[string]bool{"mp-10": false},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -2980,6 +3012,15 @@ func TestReconcileMachinePools(t *testing.T) {
}
}

// Check BootstrapObject rotation if there was a previous MachinePool/BootstrapObject.
if currentMachinePoolState != nil && currentMachinePoolState.BootstrapObject != nil {
if tt.wantBootstrapObjectRotation[gotMachinePool.Name] {
g.Expect(currentMachinePoolState.BootstrapObject.GetName()).ToNot(Equal(gotBootstrapObject.GetName()))
} else {
g.Expect(currentMachinePoolState.BootstrapObject.GetName()).To(Equal(gotBootstrapObject.GetName()))
}
}

// Compare InfrastructureMachinePoolObject.
gotInfrastructureMachinePoolObjectRef := gotMachinePool.Spec.Template.Spec.InfrastructureRef
gotInfrastructureMachinePoolObject, err := external.GetObjectFromContractVersionedRef(ctx, env.GetAPIReader(), gotInfrastructureMachinePoolObjectRef, gotMachinePool.Namespace)
Expand All @@ -2995,6 +3036,15 @@ func TestReconcileMachinePools(t *testing.T) {
g.Expect(currentMachinePoolState.InfrastructureMachinePoolObject.GetResourceVersion()).To(Equal(gotInfrastructureMachinePoolObject.GetResourceVersion()))
}
}

// Check InfrastructureMachinePoolObject rotation if there was a previous MachinePool/InfrastructureMachinePoolObject.
if currentMachinePoolState != nil && currentMachinePoolState.InfrastructureMachinePoolObject != nil {
if tt.wantInfrastructureMachinePoolObjectRotation[gotMachinePool.Name] {
g.Expect(currentMachinePoolState.InfrastructureMachinePoolObject.GetName()).ToNot(Equal(gotInfrastructureMachinePoolObject.GetName()))
} else {
g.Expect(currentMachinePoolState.InfrastructureMachinePoolObject.GetName()).To(Equal(gotInfrastructureMachinePoolObject.GetName()))
}
}
}
}
})
Expand Down