Add finalizer reconcile for Topology MachineDeployments and MachineSets

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -35,11 +35,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/controllers/machinedeployment/mdutil"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
-	"sigs.k8s.io/cluster-api/util/labels"
 	"sigs.k8s.io/cluster-api/util/patch"
 )
 
@@ -178,17 +176,6 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD
 		},
 	}
 
-	if feature.Gates.Enabled(feature.ClusterTopology) {
-		// If the MachineDeployment is owned by a Cluster Topology,
-		// add the finalizer to allow the topology controller to
-		// clean up resources when the MachineSet is deleted.
-		// MachineSets are deleted during rollout (e.g. template rotation) and
-		// after MachineDeployment deletion.
-		if labels.IsTopologyOwned(d) {
-			controllerutil.AddFinalizer(&newMS, clusterv1.MachineSetTopologyFinalizer)
-		}
-	}
-
 	if d.Spec.Strategy.RollingUpdate.DeletePolicy != nil {
 		newMS.Spec.DeletePolicy = *d.Spec.Strategy.RollingUpdate.DeletePolicy
 	}

internal/controllers/topology/cluster/desired_state.go

@@ -28,7 +28,6 @@ import (
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -623,13 +622,6 @@ func computeMachineDeployment(_ context.Context, s *scope.Scope, desiredControlP
 		},
 	}
 
-	// If it's a new MachineDeployment, set the finalizer.
-	// Note: we only add it on creation to avoid race conditions later on when
-	// the MachineDeployment topology controller removes the finalizer.
-	if currentMachineDeployment == nil {
-		controllerutil.AddFinalizer(desiredMachineDeploymentObj, clusterv1.MachineDeploymentTopologyFinalizer)
-	}
-
 	// If an existing MachineDeployment is present, override the MachineDeployment generate name
 	// re-using the existing name (this will help in reconcile).
 	if currentMachineDeployment != nil && currentMachineDeployment.Object != nil {

internal/controllers/topology/cluster/desired_state_test.go

@@ -30,7 +30,6 @@ import (
 	utilfeature "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1"
@@ -1382,7 +1381,6 @@ func TestComputeMachineDeployment(t *testing.T) {
 
 		g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
 		g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
-		g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeTrue())
 
 		g.Expect(actualMd.Spec.Selector.MatchLabels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
 		g.Expect(actualMd.Spec.Selector.MatchLabels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
@@ -1436,7 +1434,6 @@ func TestComputeMachineDeployment(t *testing.T) {
 
 		g.Expect(actualMd.Labels).To(HaveKeyWithValue(clusterv1.ClusterTopologyMachineDeploymentLabelName, "big-pool-of-machines"))
 		g.Expect(actualMd.Labels).To(HaveKey(clusterv1.ClusterTopologyOwnedLabel))
-		g.Expect(controllerutil.ContainsFinalizer(actualMd, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())
 
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("foo", "baz"))
 		g.Expect(actualMd.Spec.Template.ObjectMeta.Labels).To(HaveKeyWithValue("fizz", "buzz"))

internal/controllers/topology/machinedeployment/machinedeployment_controller.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -84,7 +85,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 //
 // We don't have to set the finalizer, as it's already set during MachineDeployment creation
 // in the cluster topology controller.
-func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	// Fetch the MachineDeployment instance.
@@ -112,12 +113,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
+	// Create a patch helper to add or remove the finalizer from the MachineDeployment.
+	patchHelper, err := patch.NewHelper(md, r.Client)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md})
+	}
+	defer func() {
+		if err := patchHelper.Patch(ctx, md); err != nil {
+			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md})})
+		}
+	}()
+
 	// Handle deletion reconciliation loop.
 	if !md.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, md)
 	}
 
-	// Nothing to do.
+	// If the MachineDeployment is not being deleted ensure the finalizer is set.
+	controllerutil.AddFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)
+
 	return ctrl.Result{}, nil
 }
 
@@ -151,16 +165,8 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD
 		return ctrl.Result{}, err
 	}
 
-	// Remove the finalizer so the MachineDeployment can be garbage collected by Kubernetes.
-	patchHelper, err := patch.NewHelper(md, r.Client)
-	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md})
-	}
-
 	controllerutil.RemoveFinalizer(md, clusterv1.MachineDeploymentTopologyFinalizer)
-	if err := patchHelper.Patch(ctx, md); err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md})
-	}
+
 	return ctrl.Result{}, nil
 }
 

internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go

@@ -27,11 +27,85 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
+	"sigs.k8s.io/cluster-api/util"
 )
 
+func TestMachineDeploymentTopologyFinalizer(t *testing.T) {
+	cluster := builder.Cluster(metav1.NamespaceDefault, "fake-cluster").Build()
+	mdBT := builder.BootstrapTemplate(metav1.NamespaceDefault, "mdBT").Build()
+	mdIMT := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "mdIMT").Build()
+	mdBuilder := builder.MachineDeployment(metav1.NamespaceDefault, "md").
+		WithClusterName("fake-cluster").
+		WithBootstrapTemplate(mdBT).
+		WithInfrastructureTemplate(mdIMT)
+
+	md := mdBuilder.Build()
+	mdWithFinalizer := mdBuilder.Build()
+	mdWithFinalizer.Finalizers = []string{clusterv1.MachineDeploymentTopologyFinalizer}
+	mdWithDeletionTimestamp := mdBuilder.Build()
+	deletionTimestamp := metav1.Now()
+	mdWithDeletionTimestamp.DeletionTimestamp = &deletionTimestamp
+
+	mdWithDeletionTimestampAndFinalizer := mdWithDeletionTimestamp.DeepCopy()
+	mdWithDeletionTimestampAndFinalizer.Finalizers = []string{clusterv1.MachineDeploymentTopologyFinalizer}
+
+	testCases := []struct {
+		name            string
+		md              *clusterv1.MachineDeployment
+		expectFinalizer bool
+	}{
+		{
+			name:            "should add ClusterTopology finalizer to a MachineDeployment with no finalizer",
+			md:              md,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should retain ClusterTopology finalizer on MachineDeployment with finalizer",
+			md:              mdWithFinalizer,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should not add ClusterTopology finalizer on MachineDeployment with Deletion Timestamp and no finalizer ",
+			md:              mdWithDeletionTimestamp,
+			expectFinalizer: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(fakeScheme).
+				WithObjects(tc.md, mdBT, mdIMT, cluster).
+				Build()
+
+			mdr := &Reconciler{
+				Client:    fakeClient,
+				APIReader: fakeClient,
+			}
+
+			_, err := mdr.Reconcile(ctx, reconcile.Request{
+				NamespacedName: util.ObjectKey(tc.md),
+			})
+			g.Expect(err).NotTo(HaveOccurred())
+
+			key := client.ObjectKey{Namespace: tc.md.Namespace, Name: tc.md.Name}
+			var actual clusterv1.MachineDeployment
+			g.Expect(mdr.Client.Get(ctx, key, &actual)).To(Succeed())
+			if tc.expectFinalizer {
+				g.Expect(actual.Finalizers).To(ConsistOf(clusterv1.MachineDeploymentTopologyFinalizer))
+			} else {
+				g.Expect(actual.Finalizers).To(BeEmpty())
+			}
+		})
+	}
+}
+
 func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) {
 	deletionTimeStamp := metav1.Now()
 
@@ -98,7 +172,7 @@ func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) {
 	t.Run("Should not delete templates of a MachineDeployment when they are still in use in a MachineSet", func(t *testing.T) {
 		g := NewWithT(t)
 
-		ms := builder.MachineSet(md.Namespace, "ms").
+		ms := builder.MachineSet(md.Namespace, "md").
 			WithBootstrapTemplate(mdBT).
 			WithInfrastructureTemplate(mdIMT).
 			WithLabels(map[string]string{

internal/controllers/topology/machineset/machineset_controller.go

@@ -23,6 +23,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -86,7 +87,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 //
 // We don't have to set the finalizer, as it's already set during MachineSet creation
 // in the MachineSet controller.
-func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
 	// Fetch the MachineSet instance.
 	ms := &clusterv1.MachineSet{}
 	if err := r.Client.Get(ctx, req.NamespacedName, ms); err != nil {
@@ -119,12 +120,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
+	// Create a patch helper to add or remove the finalizer from the MachineSet.
+	patchHelper, err := patch.NewHelper(ms, r.Client)
+	if err != nil {
+		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms})
+	}
+	defer func() {
+		if err := patchHelper.Patch(ctx, ms); err != nil {
+			reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms})})
+		}
+	}()
+
 	// Handle deletion reconciliation loop.
 	if !ms.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, ms)
 	}
 
-	// Nothing to do.
+	// If the MachineSet is not being deleted ensure the finalizer is set.
+	controllerutil.AddFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)
+
 	return ctrl.Result{}, nil
 }
 
@@ -172,14 +186,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, ms *clusterv1.MachineS
 	}
 
 	// Remove the finalizer so the MachineSet can be garbage collected by Kubernetes.
-	patchHelper, err := patch.NewHelper(ms, r.Client)
-	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms})
-	}
 	controllerutil.RemoveFinalizer(ms, clusterv1.MachineSetTopologyFinalizer)
-	if err := patchHelper.Patch(ctx, ms); err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms})
-	}
 
 	return ctrl.Result{}, nil
 }

internal/controllers/topology/machineset/machineset_controller_test.go

@@ -27,11 +27,99 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
+	"sigs.k8s.io/cluster-api/util"
 )
 
+func TestMachineSetTopologyFinalizer(t *testing.T) {
+	mdName := "md"
+
+	msBT := builder.BootstrapTemplate(metav1.NamespaceDefault, "msBT").Build()
+	msIMT := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "msIMT").Build()
+
+	cluster := builder.Cluster(metav1.NamespaceDefault, "fake-cluster").Build()
+	msBuilder := builder.MachineSet(metav1.NamespaceDefault, "ms").
+		WithBootstrapTemplate(msBT).
+		WithInfrastructureTemplate(msIMT).
+		WithClusterName(cluster.Name).
+		WithOwnerReferences([]metav1.OwnerReference{
+			{
+				Kind:       "MachineDeployment",
+				APIVersion: clusterv1.GroupVersion.String(),
+				Name:       "md",
+			},
+		}).
+		WithLabels(map[string]string{
+			clusterv1.MachineDeploymentLabelName: mdName,
+			clusterv1.ClusterTopologyOwnedLabel:  "",
+		})
+
+	ms := msBuilder.Build()
+	msWithFinalizer := msBuilder.Build()
+	msWithFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer}
+	msWithDeletionTimestamp := msBuilder.Build()
+	deletionTimestamp := metav1.Now()
+	msWithDeletionTimestamp.DeletionTimestamp = &deletionTimestamp
+
+	msWithDeletionTimestampAndFinalizer := msWithDeletionTimestamp.DeepCopy()
+	msWithDeletionTimestampAndFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer}
+
+	testCases := []struct {
+		name            string
+		ms              *clusterv1.MachineSet
+		expectFinalizer bool
+	}{
+		{
+			name:            "should add ClusterTopology finalizer to a MachineSet with no finalizer",
+			ms:              ms,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should retain ClusterTopology finalizer on MachineSet with finalizer",
+			ms:              msWithFinalizer,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should not add ClusterTopology finalizer on MachineSet with Deletion Timestamp and no finalizer ",
+			ms:              msWithDeletionTimestamp,
+			expectFinalizer: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(fakeScheme).
+				WithObjects(tc.ms, msBT, msIMT, cluster).
+				Build()
+
+			msr := &Reconciler{
+				Client:    fakeClient,
+				APIReader: fakeClient,
+			}
+
+			_, err := msr.Reconcile(ctx, reconcile.Request{
+				NamespacedName: util.ObjectKey(tc.ms),
+			})
+			g.Expect(err).NotTo(HaveOccurred())
+
+			key := client.ObjectKey{Namespace: tc.ms.Namespace, Name: tc.ms.Name}
+			var actual clusterv1.MachineSet
+			g.Expect(msr.Client.Get(ctx, key, &actual)).To(Succeed())
+			if tc.expectFinalizer {
+				g.Expect(actual.Finalizers).To(ConsistOf(clusterv1.MachineSetTopologyFinalizer))
+			} else {
+				g.Expect(actual.Finalizers).To(BeEmpty())
+			}
+		})
+	}
+}
+
 func TestMachineSetReconciler_ReconcileDelete(t *testing.T) {
 	deletionTimeStamp := metav1.Now()