Add finalizer reconcile for Topology MachineSets

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

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, err})
+		}
+	}()
+
 	// 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 before returning.
+	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()
 

internal/test/builder/builders.go

@@ -1151,14 +1151,16 @@ func (m *MachineDeploymentBuilder) Build() *clusterv1.MachineDeployment {
 	return obj
 }
 
-// MachineSetBuilder holds the variables and objects needed to build a generic MachineSet.
+// MachineSetBuilder holds the variables and objects needed to build a MachineSet.
 type MachineSetBuilder struct {
 	namespace              string
 	name                   string
 	bootstrapTemplate      *unstructured.Unstructured
 	infrastructureTemplate *unstructured.Unstructured
 	replicas               *int32
 	labels                 map[string]string
+	clusterName            string
+	ownerRefs              []metav1.OwnerReference
 }
 
 // MachineSet creates a MachineSetBuilder with the given name and namespace.
@@ -1175,7 +1177,7 @@ func (m *MachineSetBuilder) WithBootstrapTemplate(ref *unstructured.Unstructured
 	return m
 }
 
-// WithInfrastructureTemplate adds the passed unstructured object to the MachineSet builder as an infrastructureMachineTemplate.
+// WithInfrastructureTemplate adds the passed unstructured object to the MachineSetBuilder as an infrastructureMachineTemplate.
 func (m *MachineSetBuilder) WithInfrastructureTemplate(ref *unstructured.Unstructured) *MachineSetBuilder {
 	m.infrastructureTemplate = ref
 	return m
@@ -1187,12 +1189,24 @@ func (m *MachineSetBuilder) WithLabels(labels map[string]string) *MachineSetBuil
 	return m
 }
 
-// WithReplicas sets the number of replicas for the MachineSetClassBuilder.
+// WithReplicas sets the number of replicas for the MachineSetBuilder.
 func (m *MachineSetBuilder) WithReplicas(replicas *int32) *MachineSetBuilder {
 	m.replicas = replicas
 	return m
 }
 
+// WithClusterName sets the number of replicas for the MachineSetBuilder.
+func (m *MachineSetBuilder) WithClusterName(name string) *MachineSetBuilder {
+	m.clusterName = name
+	return m
+}
+
+// WithOwnerReferences adds ownerReferences for the MachineSetBuilder.
+func (m *MachineSetBuilder) WithOwnerReferences(ownerRefs []metav1.OwnerReference) *MachineSetBuilder {
+	m.ownerRefs = ownerRefs
+	return m
+}
+
 // Build creates a new MachineSet with the variables and objects passed to the MachineSetBuilder.
 func (m *MachineSetBuilder) Build() *clusterv1.MachineSet {
 	obj := &clusterv1.MachineSet{
@@ -1201,11 +1215,13 @@ func (m *MachineSetBuilder) Build() *clusterv1.MachineSet {
 			APIVersion: clusterv1.GroupVersion.String(),
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      m.name,
-			Namespace: m.namespace,
-			Labels:    m.labels,
+			Name:            m.name,
+			Namespace:       m.namespace,
+			Labels:          m.labels,
+			OwnerReferences: m.ownerRefs,
 		},
 	}
+	obj.Spec.ClusterName = m.clusterName
 	obj.Spec.Replicas = m.replicas
 	if m.bootstrapTemplate != nil {
 		obj.Spec.Template.Spec.Bootstrap.ConfigRef = objToRef(m.bootstrapTemplate)