🌱 Foreground deletion for MachineDeployments and MachineSets (#11174)

* foreground deletion for machine deployments

* review fixes

* fix unit tests

* fix e2e finalizer assertions

* machinedeployment: rename helper function

* review fixes

* add unit tests for reconcileDelete functions

* review fixes

* review fix

* test: implement e2e test for cluster deletion

* review fixes

* review fixes

* Review Fixes

* lint fixes

* fix e2e test

* lint fix

* Review Fixes

---------

Co-authored-by: Dhairya-Arora01 <dhairya.arora@syself.com>

Author: chrischdi

api/v1beta1/machinedeployment_types.go

@@ -25,6 +25,10 @@ const (
 	// MachineDeploymentTopologyFinalizer is the finalizer used by the topology MachineDeployment controller to
 	// clean up referenced template resources if necessary when a MachineDeployment is being deleted.
 	MachineDeploymentTopologyFinalizer = "machinedeployment.topology.cluster.x-k8s.io"
+
+	// MachineDeploymentFinalizer is the finalizer used by the MachineDeployment controller to
+	// ensure ordered cleanup of corresponding MachineSets when a MachineDeployment is being deleted.
+	MachineDeploymentFinalizer = "cluster.x-k8s.io/machinedeployment"
 )
 
 // MachineDeploymentStrategyType defines the type of MachineDeployment rollout strategies.

api/v1beta1/machineset_types.go

@@ -29,6 +29,10 @@ const (
 	// MachineSetTopologyFinalizer is the finalizer used by the topology MachineDeployment controller to
 	// clean up referenced template resources if necessary when a MachineSet is being deleted.
 	MachineSetTopologyFinalizer = "machineset.topology.cluster.x-k8s.io"
+
+	// MachineSetFinalizer is the finalizer used by the MachineSet controller to
+	// ensure ordered cleanup of corresponding Machines when a Machineset is being deleted.
+	MachineSetFinalizer = "cluster.x-k8s.io/machineset"
 )
 
 // ANCHOR: MachineSetSpec

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -33,6 +33,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"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"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -42,6 +43,7 @@ import (
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	utilconversion "sigs.k8s.io/cluster-api/util/conversion"
+	clog "sigs.k8s.io/cluster-api/util/log"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/predicates"
 )
@@ -158,9 +160,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		}
 	}()
 
-	// Ignore deleted MachineDeployments, this can happen when foregroundDeletion
-	// is enabled
+	// Handle deletion reconciliation loop.
 	if !deployment.DeletionTimestamp.IsZero() {
+		return ctrl.Result{}, r.reconcileDelete(ctx, deployment)
+	}
+
+	// Add finalizer first if not set to avoid the race condition between init and delete.
+	// Note: Finalizers in general can only be added when the deletionTimestamp is not set.
+	if !controllerutil.ContainsFinalizer(deployment, clusterv1.MachineDeploymentFinalizer) {
+		controllerutil.AddFinalizer(deployment, clusterv1.MachineDeploymentFinalizer)
 		return ctrl.Result{}, nil
 	}
 
@@ -225,7 +233,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 		}
 	}
 
-	msList, err := r.getMachineSetsForDeployment(ctx, md)
+	msList, err := r.getAndAdoptMachineSetsForDeployment(ctx, md)
 	if err != nil {
 		return err
 	}
@@ -286,8 +294,36 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	return errors.Errorf("unexpected deployment strategy type: %s", md.Spec.Strategy.Type)
 }
 
-// getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment.
-func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) {
+func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) error {
+	log := ctrl.LoggerFrom(ctx)
+	msList, err := r.getAndAdoptMachineSetsForDeployment(ctx, md)
+	if err != nil {
+		return err
+	}
+
+	// If all the descendant machinesets are deleted, then remove the machinedeployment's finalizer.
+	if len(msList) == 0 {
+		controllerutil.RemoveFinalizer(md, clusterv1.MachineDeploymentFinalizer)
+		return nil
+	}
+
+	log.Info("Waiting for MachineSets to be deleted", "MachineSets", clog.ObjNamesString(msList))
+
+	// else delete owned machinesets.
+	for _, ms := range msList {
+		if ms.DeletionTimestamp.IsZero() {
+			log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms))
+			if err := r.Client.Delete(ctx, ms); err != nil && !apierrors.IsNotFound(err) {
+				return errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms))
+			}
+		}
+	}
+
+	return nil
+}
+
+// getAndAdoptMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment.
+func (r *Reconciler) getAndAdoptMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	// List all MachineSets to find those we own but that no longer match our selector.
@@ -299,7 +335,8 @@ func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, md *cluste
 	filtered := make([]*clusterv1.MachineSet, 0, len(machineSets.Items))
 	for idx := range machineSets.Items {
 		ms := &machineSets.Items[idx]
-		log.WithValues("MachineSet", klog.KObj(ms))
+		log := log.WithValues("MachineSet", klog.KObj(ms))
+		ctx := ctrl.LoggerInto(ctx, log)
 		selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector)
 		if err != nil {
 			log.Error(err, "Skipping MachineSet, failed to get label selector from spec selector")

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -34,6 +34,7 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
+	"sigs.k8s.io/cluster-api/internal/test/builder"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
@@ -962,7 +963,7 @@ func TestGetMachineSetsForDeployment(t *testing.T) {
 				recorder: record.NewFakeRecorder(32),
 			}
 
-			got, err := r.getMachineSetsForDeployment(ctx, &tc.machineDeployment)
+			got, err := r.getAndAdoptMachineSetsForDeployment(ctx, &tc.machineDeployment)
 			g.Expect(err).ToNot(HaveOccurred())
 			g.Expect(got).To(HaveLen(len(tc.expected)))
 
@@ -993,3 +994,90 @@ func updateMachineDeployment(ctx context.Context, c client.Client, md *clusterv1
 		return patchHelper.Patch(ctx, md)
 	})
 }
+
+func TestReconciler_reconcileDelete(t *testing.T) {
+	labels := map[string]string{
+		"some": "labelselector",
+	}
+	md := builder.MachineDeployment("default", "md0").WithClusterName("test").Build()
+	md.Finalizers = []string{
+		clusterv1.MachineDeploymentFinalizer,
+	}
+	md.DeletionTimestamp = ptr.To(metav1.Now())
+	md.Spec.Selector = metav1.LabelSelector{
+		MatchLabels: labels,
+	}
+	mdWithoutFinalizer := md.DeepCopy()
+	mdWithoutFinalizer.Finalizers = []string{}
+	tests := []struct {
+		name              string
+		machineDeployment *clusterv1.MachineDeployment
+		want              *clusterv1.MachineDeployment
+		objs              []client.Object
+		wantMachineSets   []clusterv1.MachineSet
+		expectError       bool
+	}{
+		{
+			name:              "Should do nothing when no descendant MachineSets exist and finalizer is already gone",
+			machineDeployment: mdWithoutFinalizer.DeepCopy(),
+			want:              mdWithoutFinalizer.DeepCopy(),
+			objs:              nil,
+			wantMachineSets:   nil,
+			expectError:       false,
+		},
+		{
+			name:              "Should remove finalizer when no descendant MachineSets exist",
+			machineDeployment: md.DeepCopy(),
+			want:              mdWithoutFinalizer.DeepCopy(),
+			objs:              nil,
+			wantMachineSets:   nil,
+			expectError:       false,
+		},
+		{
+			name:              "Should keep finalizer when descendant MachineSets exist and trigger deletion only for descendant MachineSets",
+			machineDeployment: md.DeepCopy(),
+			want:              md.DeepCopy(),
+			objs: []client.Object{
+				builder.MachineSet("default", "ms0").WithClusterName("test").WithLabels(labels).Build(),
+				builder.MachineSet("default", "ms1").WithClusterName("test").WithLabels(labels).Build(),
+				builder.MachineSet("default", "ms2-not-part-of-md").WithClusterName("test").Build(),
+				builder.MachineSet("default", "ms3-not-part-of-md").WithClusterName("test").Build(),
+			},
+			wantMachineSets: []clusterv1.MachineSet{
+				*builder.MachineSet("default", "ms2-not-part-of-md").WithClusterName("test").Build(),
+				*builder.MachineSet("default", "ms3-not-part-of-md").WithClusterName("test").Build(),
+			},
+			expectError: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			c := fake.NewClientBuilder().WithObjects(tt.objs...).Build()
+			r := &Reconciler{
+				Client:   c,
+				recorder: record.NewFakeRecorder(32),
+			}
+
+			err := r.reconcileDelete(ctx, tt.machineDeployment)
+			if tt.expectError {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+
+			g.Expect(tt.machineDeployment).To(BeComparableTo(tt.want))
+
+			machineSetList := &clusterv1.MachineSetList{}
+			g.Expect(c.List(ctx, machineSetList, client.InNamespace("default"))).ToNot(HaveOccurred())
+
+			// Remove ResourceVersion so we can actually compare.
+			for i := range machineSetList.Items {
+				machineSetList.Items[i].ResourceVersion = ""
+			}
+
+			g.Expect(machineSetList.Items).To(ConsistOf(tt.wantMachineSets))
+		})
+	}
+}