diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index c6fa0090df73..8cdeb71a1c01 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -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.") } } } @@ -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) @@ -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() { + // 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() { @@ -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)) diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 4a8f3f472803..8f51a028b0d4 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -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", @@ -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, }, { @@ -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", @@ -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", @@ -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", @@ -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) { @@ -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) @@ -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())) + } + } } } })