Merge pull request kubernetes-sigs#7536 from killianmuldoon/fix/machineset-finalizer

🐛  Add finalizer reconcile for Topology MachineSets and MachineDeployments

Author: k8s-ci-robot

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/cluster_controller_test.go

@@ -911,9 +911,8 @@ func assertControlPlaneReconcile(cluster *clusterv1.Cluster) error {
 // assertMachineDeploymentsReconcile checks if the MachineDeployments:
 // 1) Are created in the correct number.
 // 2) Have the correct labels (TopologyOwned, ClusterName, MachineDeploymentName).
-// 3) Have the correct finalizer applied.
-// 4) Have the correct replicas and version.
-// 6) Have the correct Kind/APIVersion and Labels/Annotations for BoostrapRef and InfrastructureRef templates.
+// 3) Have the correct replicas and version.
+// 4) Have the correct Kind/APIVersion and Labels/Annotations for BoostrapRef and InfrastructureRef templates.
 func assertMachineDeploymentsReconcile(cluster *clusterv1.Cluster) error {
 	// List all created machine deployments to assert the expected numbers are created.
 	machineDeployments := &clusterv1.MachineDeploymentList{}
@@ -950,16 +949,6 @@ func assertMachineDeploymentsReconcile(cluster *clusterv1.Cluster) error {
 				continue
 			}
 
-			// Assert that the correct Finalizer has been added to the MachineDeployment.
-			for _, f := range md.Finalizers {
-				// Break as soon as we find a matching finalizer.
-				if f == clusterv1.MachineDeploymentTopologyFinalizer {
-					break
-				}
-				// False if the finalizer is not present on the MachineDeployment.
-				return fmt.Errorf("finalizer %v not found on MachineDeployment", clusterv1.MachineDeploymentTopologyFinalizer)
-			}
-
 			// Check if the ClusterTopologyLabelName and ClusterTopologyOwnedLabel are set correctly.
 			if err := assertClusterTopologyOwnedLabel(&md); err != nil {
 				return err

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"
@@ -678,13 +677,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"
@@ -1443,7 +1442,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"))
@@ -1524,7 +1522,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
 }