Merge pull request #11415 from Patryk-Stefanski/cleanup-disable-machine-create-annotation-rolling-machine-deployment

🐛 remove disableMachineCreate annotation from new machinesets during rolling machine deployment reconciliation

Author: k8s-ci-robot

internal/controllers/machinedeployment/machinedeployment_rolling.go

@@ -21,11 +21,13 @@ import (
 	"sort"
 
 	"github.com/pkg/errors"
+	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil"
+	"sigs.k8s.io/cluster-api/util/patch"
 )
 
 // rolloutRolling implements the logic for rolling a new MachineSet.
@@ -72,6 +74,10 @@ func (r *Reconciler) rolloutRolling(ctx context.Context, md *clusterv1.MachineDe
 }
 
 func (r *Reconciler) reconcileNewMachineSet(ctx context.Context, allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, deployment *clusterv1.MachineDeployment) error {
+	if err := r.cleanupDisableMachineCreateAnnotation(ctx, newMS); err != nil {
+		return err
+	}
+
 	if deployment.Spec.Replicas == nil {
 		return errors.Errorf("spec.replicas for MachineDeployment %v is nil, this is unexpected", client.ObjectKeyFromObject(deployment))
 	}
@@ -293,3 +299,25 @@ func (r *Reconciler) scaleDownOldMachineSetsForRollingUpdate(ctx context.Context
 
 	return totalScaledDown, nil
 }
+
+// cleanupDisableMachineCreateAnnotation will remove the disable machine create annotation from new MachineSets that were created during reconcileOldMachineSetsOnDelete.
+func (r *Reconciler) cleanupDisableMachineCreateAnnotation(ctx context.Context, newMS *clusterv1.MachineSet) error {
+	log := ctrl.LoggerFrom(ctx, "MachineSet", klog.KObj(newMS))
+
+	if newMS.Annotations != nil {
+		if _, ok := newMS.Annotations[clusterv1.DisableMachineCreateAnnotation]; ok {
+			log.V(4).Info("removing annotation on latest MachineSet to enable machine creation")
+			patchHelper, err := patch.NewHelper(newMS, r.Client)
+			if err != nil {
+				return err
+			}
+			delete(newMS.Annotations, clusterv1.DisableMachineCreateAnnotation)
+			err = patchHelper.Patch(ctx, newMS)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}

internal/controllers/machinedeployment/machinedeployment_rolling_test.go

@@ -217,6 +217,41 @@ func TestReconcileNewMachineSet(t *testing.T) {
 			},
 			error: nil,
 		},
+		{
+			name: "Rolling Updated Cleanup disable machine create annotation",
+			machineDeployment: &clusterv1.MachineDeployment{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "foo",
+					Name:      "bar",
+				},
+				Spec: clusterv1.MachineDeploymentSpec{
+					Strategy: &clusterv1.MachineDeploymentStrategy{
+						Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
+						RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{
+							MaxUnavailable: intOrStrPtr(0),
+							MaxSurge:       intOrStrPtr(0),
+						},
+					},
+					Replicas: ptr.To[int32](2),
+				},
+			},
+			newMachineSet: &clusterv1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "foo",
+					Name:      "bar",
+					Annotations: map[string]string{
+						clusterv1.DisableMachineCreateAnnotation: "true",
+						clusterv1.DesiredReplicasAnnotation:      "2",
+						clusterv1.MaxReplicasAnnotation:          "2",
+					},
+				},
+				Spec: clusterv1.MachineSetSpec{
+					Replicas: ptr.To[int32](2),
+				},
+			},
+			expectedNewMachineSetReplicas: 2,
+			error:                         nil,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -252,6 +287,9 @@ func TestReconcileNewMachineSet(t *testing.T) {
 
 			g.Expect(*freshNewMachineSet.Spec.Replicas).To(BeEquivalentTo(tc.expectedNewMachineSetReplicas))
 
+			_, ok := freshNewMachineSet.GetAnnotations()[clusterv1.DisableMachineCreateAnnotation]
+			g.Expect(ok).To(BeFalse())
+
 			desiredReplicasAnnotation, ok := freshNewMachineSet.GetAnnotations()[clusterv1.DesiredReplicasAnnotation]
 			g.Expect(ok).To(BeTrue())
 			g.Expect(strconv.Atoi(desiredReplicasAnnotation)).To(BeEquivalentTo(*tc.machineDeployment.Spec.Replicas))

internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go

@@ -165,22 +165,9 @@ func (r *Reconciler) reconcileOldMachineSetsOnDelete(ctx context.Context, oldMSs
 
 // reconcileNewMachineSetOnDelete handles reconciliation of the latest MachineSet associated with the MachineDeployment in the OnDelete MachineDeploymentStrategyType.
 func (r *Reconciler) reconcileNewMachineSetOnDelete(ctx context.Context, allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, deployment *clusterv1.MachineDeployment) error {
-	// logic same as reconcile logic for RollingUpdate
-	log := ctrl.LoggerFrom(ctx, "MachineSet", klog.KObj(newMS))
-
-	if newMS.Annotations != nil {
-		if _, ok := newMS.Annotations[clusterv1.DisableMachineCreateAnnotation]; ok {
-			log.V(4).Info("removing annotation on latest MachineSet to enable machine creation")
-			patchHelper, err := patch.NewHelper(newMS, r.Client)
-			if err != nil {
-				return err
-			}
-			delete(newMS.Annotations, clusterv1.DisableMachineCreateAnnotation)
-			err = patchHelper.Patch(ctx, newMS)
-			if err != nil {
-				return err
-			}
-		}
+	if err := r.cleanupDisableMachineCreateAnnotation(ctx, newMS); err != nil {
+		return err
 	}
+
 	return r.reconcileNewMachineSet(ctx, allMSs, newMS, deployment)
 }