🌱 Foreground deletion for MachineDeployments and MachineSets

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 {
+				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.

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -18,6 +18,7 @@ package machinedeployment
 
 import (
 	"context"
+	"sort"
 	"testing"
 	"time"
 
@@ -34,6 +35,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 +964,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 +995,96 @@ 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.DeletionTimestamp = ptr.To(metav1.Now())
+	md.Spec.Selector = metav1.LabelSelector{
+		MatchLabels: labels,
+	}
+	mdWithoutFinalizer := md.DeepCopy()
+	mdWithoutFinalizer.Finalizers = nil
+	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),
+			}
+			// Mark machineDeployment to be in deletion.
+
+			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, m := range machineSetList.Items {
+				m.ResourceVersion = ""
+				machineSetList.Items[i] = m
+			}
+
+			sort.Slice(machineSetList.Items, func(i, j int) bool {
+				return machineSetList.Items[i].GetName() < machineSetList.Items[j].GetName()
+			})
+			sort.Slice(tt.wantMachineSets, func(i, j int) bool {
+				return tt.wantMachineSets[i].GetName() < tt.wantMachineSets[j].GetName()
+			})
+
+			g.Expect(machineSetList.Items).To(BeComparableTo(tt.wantMachineSets))
+		})
+	}
+}