🌱 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
+	// cleanup the MachineDeployment descendant 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
+	// cleanup the MachineSet descendant 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"
@@ -158,9 +159,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 +232,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 +293,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("MachineDeployment still has descendant MachineSets - deleting them first", "count", len(msList), "descendants", descendantMachineSets(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.
@@ -439,3 +474,16 @@ func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cl
 
 	return patchHelper.Patch(ctx, obj)
 }
+
+func descendantMachineSets(objs []*clusterv1.MachineSet) string {
+	objNames := make([]string, len(objs))
+	for _, obj := range objs {
+		objNames = append(objNames, obj.GetName())
+	}
+
+	if len(objNames) > 10 {
+		objNames = append(objNames[:10], "...")
+	}
+
+	return strings.Join(objNames, ",")
+}

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -962,7 +962,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)))
 

internal/controllers/machineset/machineset_controller.go

@@ -41,6 +41,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"
@@ -182,9 +183,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		}
 	}()
 
-	// Ignore deleted MachineSets, this can happen when foregroundDeletion
-	// is enabled
+	// Handle deletion reconciliation loop.
 	if !machineSet.DeletionTimestamp.IsZero() {
+		return ctrl.Result{}, r.reconcileDelete(ctx, machineSet)
+	}
+
+	// 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(machineSet, clusterv1.MachineSetFinalizer) {
+		controllerutil.AddFinalizer(machineSet, clusterv1.MachineSetFinalizer)
 		return ctrl.Result{}, nil
 	}
 
@@ -224,8 +231,6 @@ func patchMachineSet(ctx context.Context, patchHelper *patch.Helper, machineSet
 }
 
 func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, machineSet *clusterv1.MachineSet) (ctrl.Result, error) {
-	log := ctrl.LoggerFrom(ctx)
-
 	// Reconcile and retrieve the Cluster object.
 	if machineSet.Labels == nil {
 		machineSet.Labels = make(map[string]string)
@@ -266,63 +271,28 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	machineSet.Spec.Selector.MatchLabels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName
 	machineSet.Spec.Template.Labels[clusterv1.ClusterNameLabel] = machineSet.Spec.ClusterName
 
-	selectorMap, err := metav1.LabelSelectorAsMap(&machineSet.Spec.Selector)
-	if err != nil {
-		return ctrl.Result{}, errors.Wrapf(err, "failed to convert MachineSet %q label selector to a map", machineSet.Name)
-	}
-
-	// Get all Machines linked to this MachineSet.
-	allMachines := &clusterv1.MachineList{}
-	err = r.Client.List(ctx,
-		allMachines,
-		client.InNamespace(machineSet.Namespace),
-		client.MatchingLabels(selectorMap),
-	)
+	machines, err := r.getAndAdoptMachinesForMachineSet(ctx, machineSet)
 	if err != nil {
-		return ctrl.Result{}, errors.Wrap(err, "failed to list machines")
-	}
-
-	// Filter out irrelevant machines (i.e. IsControlledBy something else) and claim orphaned machines.
-	// Machines in deleted state are deliberately not excluded https://github.com/kubernetes-sigs/cluster-api/pull/3434.
-	filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items))
-	for idx := range allMachines.Items {
-		machine := &allMachines.Items[idx]
-		log := log.WithValues("Machine", klog.KObj(machine))
-		if shouldExcludeMachine(machineSet, machine) {
-			continue
-		}
-
-		// Attempt to adopt machine if it meets previous conditions and it has no controller references.
-		if metav1.GetControllerOf(machine) == nil {
-			if err := r.adoptOrphan(ctx, machineSet, machine); err != nil {
-				log.Error(err, "Failed to adopt Machine")
-				r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt Machine %q: %v", machine.Name, err)
-				continue
-			}
-			log.Info("Adopted Machine")
-			r.recorder.Eventf(machineSet, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted Machine %q", machine.Name)
-		}
-
-		filteredMachines = append(filteredMachines, machine)
+		return ctrl.Result{}, errors.Wrap(err, "failed to list Machines")
 	}
 
 	result := ctrl.Result{}
 
-	reconcileUnhealthyMachinesResult, err := r.reconcileUnhealthyMachines(ctx, cluster, machineSet, filteredMachines)
+	reconcileUnhealthyMachinesResult, err := r.reconcileUnhealthyMachines(ctx, cluster, machineSet, machines)
 	if err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to reconcile unhealthy machines")
 	}
 	result = util.LowestNonZeroResult(result, reconcileUnhealthyMachinesResult)
 
-	if err := r.syncMachines(ctx, machineSet, filteredMachines); err != nil {
+	if err := r.syncMachines(ctx, machineSet, machines); err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "failed to update Machines")
 	}
 
-	syncReplicasResult, syncErr := r.syncReplicas(ctx, cluster, machineSet, filteredMachines)
+	syncReplicasResult, syncErr := r.syncReplicas(ctx, cluster, machineSet, machines)
 	result = util.LowestNonZeroResult(result, syncReplicasResult)
 
 	// Always updates status as machines come up or die.
-	if err := r.updateStatus(ctx, cluster, machineSet, filteredMachines); err != nil {
+	if err := r.updateStatus(ctx, cluster, machineSet, machines); err != nil {
 		return ctrl.Result{}, errors.Wrapf(kerrors.NewAggregate([]error{err, syncErr}), "failed to update MachineSet's Status")
 	}
 
@@ -359,6 +329,79 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	return result, nil
 }
 
+func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1.MachineSet) error {
+	log := ctrl.LoggerFrom(ctx)
+	machineList, err := r.getAndAdoptMachinesForMachineSet(ctx, machineSet)
+	if err != nil {
+		return err
+	}
+
+	// If all the descendant machines are deleted, then remove the machineset's finalizer.
+	if len(machineList) == 0 {
+		controllerutil.RemoveFinalizer(machineSet, clusterv1.MachineSetFinalizer)
+		return nil
+	}
+
+	log.Info("MachineSet still has descendant Machines - deleting them first", "count", len(machineList), "descendants", descendantMachines(machineList))
+
+	// else delete owned machines.
+	for _, machine := range machineList {
+		if machine.DeletionTimestamp.IsZero() {
+			log.Info("Deleting Machine", "Machine", klog.KObj(machine))
+			if err := r.Client.Delete(ctx, machine); err != nil {
+				return errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine))
+			}
+		}
+	}
+
+	return nil
+}
+
+func (r *Reconciler) getAndAdoptMachinesForMachineSet(ctx context.Context, machineSet *clusterv1.MachineSet) ([]*clusterv1.Machine, error) {
+	log := ctrl.LoggerFrom(ctx)
+	selectorMap, err := metav1.LabelSelectorAsMap(&machineSet.Spec.Selector)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to convert MachineSet %q label selector to a map", machineSet.Name)
+	}
+
+	// Get all Machines linked to this MachineSet.
+	allMachines := &clusterv1.MachineList{}
+	err = r.Client.List(ctx,
+		allMachines,
+		client.InNamespace(machineSet.Namespace),
+		client.MatchingLabels(selectorMap),
+	)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to list machines")
+	}
+
+	// Filter out irrelevant machines (i.e. IsControlledBy something else) and claim orphaned machines.
+	// Machines in deleted state are deliberately not excluded https://github.com/kubernetes-sigs/cluster-api/pull/3434.
+	filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items))
+	for idx := range allMachines.Items {
+		machine := &allMachines.Items[idx]
+		log := log.WithValues("Machine", klog.KObj(machine))
+		if shouldExcludeMachine(machineSet, machine) {
+			continue
+		}
+
+		// Attempt to adopt machine if it meets previous conditions and it has no controller references.
+		if metav1.GetControllerOf(machine) == nil {
+			if err := r.adoptOrphan(ctx, machineSet, machine); err != nil {
+				log.Error(err, "Failed to adopt Machine")
+				r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt Machine %q: %v", machine.Name, err)
+				continue
+			}
+			log.Info("Adopted Machine")
+			r.recorder.Eventf(machineSet, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted Machine %q", machine.Name)
+		}
+
+		filteredMachines = append(filteredMachines, machine)
+	}
+
+	return filteredMachines, nil
+}
+
 // syncMachines updates Machines, InfrastructureMachine and BootstrapConfig to propagate in-place mutable fields
 // from the MachineSet.
 // Note: It also cleans up managed fields of all Machines so that Machines that were
@@ -1184,3 +1227,16 @@ func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, clu
 
 	return patchHelper.Patch(ctx, obj)
 }
+
+func descendantMachines(objs []*clusterv1.Machine) string {
+	objNames := make([]string, len(objs))
+	for _, obj := range objs {
+		objNames = append(objNames, obj.GetName())
+	}
+
+	if len(objNames) > 10 {
+		objNames = append(objNames[:10], "...")
+	}
+
+	return strings.Join(objNames, ",")
+}

internal/controllers/machineset/machineset_controller_test.go

@@ -890,6 +890,9 @@ func newMachineSet(name, cluster string, replicas int32) *clusterv1.MachineSet {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
 			Namespace: metav1.NamespaceDefault,
+			Finalizers: []string{
+				clusterv1.MachineSetFinalizer,
+			},
 			Labels: map[string]string{
 				clusterv1.ClusterNameLabel: cluster,
 			},
@@ -931,6 +934,9 @@ func TestMachineSetReconcile_MachinesCreatedConditionFalseOnBadInfraRef(t *testi
 			Labels: map[string]string{
 				clusterv1.ClusterNameLabel: cluster.Name,
 			},
+			Finalizers: []string{
+				clusterv1.MachineSetFinalizer,
+			},
 		},
 		Spec: clusterv1.MachineSetSpec{
 			ClusterName: cluster.ObjectMeta.Name,

test/framework/finalizers_helpers.go

@@ -42,8 +42,10 @@ import (
 
 // CoreFinalizersAssertionWithLegacyClusters maps Cluster API core types to their expected finalizers for legacy Clusters.
 var CoreFinalizersAssertionWithLegacyClusters = map[string]func(types.NamespacedName) []string{
-	clusterKind: func(_ types.NamespacedName) []string { return []string{clusterv1.ClusterFinalizer} },
-	machineKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineFinalizer} },
+	clusterKind:           func(_ types.NamespacedName) []string { return []string{clusterv1.ClusterFinalizer} },
+	machineKind:           func(_ types.NamespacedName) []string { return []string{clusterv1.MachineFinalizer} },
+	machineSetKind:        func(_ types.NamespacedName) []string { return []string{clusterv1.MachineSetFinalizer} },
+	machineDeploymentKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineDeploymentFinalizer} },
 }
 
 // CoreFinalizersAssertionWithClassyClusters maps Cluster API core types to their expected finalizers for classy Clusters.
@@ -52,8 +54,12 @@ var CoreFinalizersAssertionWithClassyClusters = func() map[string]func(types.Nam
 	for k, v := range CoreFinalizersAssertionWithLegacyClusters {
 		r[k] = v
 	}
-	r[machineSetKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineSetTopologyFinalizer} }
-	r[machineDeploymentKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineDeploymentTopologyFinalizer} }
+	r[machineSetKind] = func(_ types.NamespacedName) []string {
+		return []string{clusterv1.MachineSetTopologyFinalizer, clusterv1.MachineSetFinalizer}
+	}
+	r[machineDeploymentKind] = func(_ types.NamespacedName) []string {
+		return []string{clusterv1.MachineDeploymentTopologyFinalizer, clusterv1.MachineDeploymentFinalizer}
+	}
 	return r
 }()