Add finalizer reconcile for Topology MachineSets

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
kubernetes-sigs · Nov 11, 2022 · 216b689 · 216b689
1 parent ebbd333
commit 216b689
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 0 deletions.
diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go
@@ -33,13 +33,15 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/controllers/noderefutil"
 	"sigs.k8s.io/cluster-api/controllers/remote"
+	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/controllers/machine"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
@@ -218,6 +220,18 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 		})
 	}
 
+	// If the MachineSet is topology-managed ensure the MachineSetTopologyFinalizer is reconciled.
+	if feature.Gates.Enabled(feature.ClusterTopology) {
+		if _, ok := machineSet.Labels[clusterv1.ClusterTopologyOwnedLabel]; ok {
+			// - If the MachineSet is not being deleted.
+			if machineSet.DeletionTimestamp.IsZero() ||
+				// - If the MachineSet is being deleted but currently still has the Finalizer.
+				(!machineSet.DeletionTimestamp.IsZero() &&
+					controllerutil.ContainsFinalizer(machineSet, clusterv1.MachineSetTopologyFinalizer)) {
+				controllerutil.AddFinalizer(machineSet, clusterv1.MachineSetTopologyFinalizer)
+			}
+		}
+	}
 	// Make sure to reconcile the external infrastructure reference.
 	if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, cluster, &machineSet.Spec.Template.Spec.InfrastructureRef); err != nil {
 		return ctrl.Result{}, err

diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go
@@ -26,12 +26,14 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/tools/record"
+	utilfeature "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
+	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
@@ -438,6 +440,88 @@ func TestMachineSetOwnerReference(t *testing.T) {
 	}
 }
 
+func TestMachineSetTopologyFinalizer(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)()
+	testCluster := &clusterv1.Cluster{
+		TypeMeta:   metav1.TypeMeta{Kind: "Cluster", APIVersion: clusterv1.GroupVersion.String()},
+		ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: testClusterName},
+	}
+	nonTopologyMS := newMachineSet("machineset1", testClusterName, int32(0))
+
+	topologyMS := newMachineSet("machineset1", testClusterName, int32(0))
+	topologyMS.Labels[clusterv1.ClusterTopologyOwnedLabel] = ""
+
+	topologyMSWithFinalizer := topologyMS.DeepCopy()
+	topologyMSWithFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer}
+	topologyMSWithDeletionTimestamp := topologyMS.DeepCopy()
+	deletionTimestamp := metav1.Now()
+	topologyMSWithDeletionTimestamp.DeletionTimestamp = &deletionTimestamp
+
+	topologyMSWithDeletionTimestampAndFinalizer := topologyMSWithDeletionTimestamp.DeepCopy()
+	topologyMSWithDeletionTimestampAndFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer}
+
+	testCases := []struct {
+		name            string
+		ms              *clusterv1.MachineSet
+		expectFinalizer bool
+	}{
+		{
+			name:            "should add ClusterTopology finalizer to topology-managed MachineSet with no finalizer",
+			ms:              topologyMS,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should retain ClusterTopology finalizer on MachineSet with Finalizer",
+			ms:              topologyMSWithFinalizer,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should retain ClusterTopology finalizer on MachineSet with Deletion Timestamp and finalizer ",
+			ms:              topologyMSWithDeletionTimestampAndFinalizer,
+			expectFinalizer: true,
+		},
+		{
+			name:            "should not add ClusterTopology finalizer on MachineSet with Deletion Timestamp and no finalizer ",
+			ms:              topologyMSWithDeletionTimestamp,
+			expectFinalizer: false,
+		},
+
+		{
+			name:            "should not add ClusterTopology finalizer to non topology-managed MachineSet with no finalizer",
+			ms:              nonTopologyMS,
+			expectFinalizer: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			msr := &Reconciler{
+				Client: fake.NewClientBuilder().WithObjects(
+					testCluster,
+					tc.ms,
+				).Build(),
+				recorder: record.NewFakeRecorder(32),
+			}
+
+			_, 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 TestMachineSetReconcile(t *testing.T) {
 	testCluster := &clusterv1.Cluster{
 		ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: testClusterName},