Merge pull request #10694 from sbueringer/pr-fixup-finalizers

🌱 Preserve finalizers during MS/Machine reconciliation

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -29,7 +29,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	apirand "k8s.io/apimachinery/pkg/util/rand"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
@@ -241,11 +240,6 @@ func (r *Reconciler) computeDesiredMachineSet(ctx context.Context, deployment *c
 		name, randomSuffix = computeNewMachineSetName(deployment.Name + "-")
 		uniqueIdentifierLabelValue = fmt.Sprintf("%d-%s", templateHash, randomSuffix)
 
-		// Add foregroundDeletion finalizer to MachineSet if the MachineDeployment has it.
-		if sets.New[string](deployment.Finalizers...).Has(metav1.FinalizerDeleteDependents) {
-			finalizers = []string{metav1.FinalizerDeleteDependents}
-		}
-
 		replicas, err = mdutil.NewMSNewReplicas(deployment, oldMSs, 0)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to compute desired MachineSet")
@@ -268,15 +262,8 @@ func (r *Reconciler) computeDesiredMachineSet(ctx context.Context, deployment *c
 		name = existingMS.Name
 		uid = existingMS.UID
 
-		// Keep foregroundDeletion finalizer if the existingMS has it.
-		// Note: This case is a little different from the create case. In the update case we preserve
-		// the finalizer on the MachineSet if it already exists. Because of SSA we should not build
-		// the finalizer information from the MachineDeployment when updating a MachineSet because that could lead
-		// to dropping the finalizer from the MachineSet if it is dropped from the MachineDeployment.
-		// We should not drop the finalizer on the MachineSet if the finalizer is dropped from the MachineDeployment.
-		if sets.New[string](existingMS.Finalizers...).Has(metav1.FinalizerDeleteDependents) {
-			finalizers = []string{metav1.FinalizerDeleteDependents}
-		}
+		// Preserve all existing finalizers (including foregroundDeletion finalizer).
+		finalizers = existingMS.Finalizers
 
 		replicas = *existingMS.Spec.Replicas
 

internal/controllers/machinedeployment/machinedeployment_sync_test.go

@@ -642,6 +642,8 @@ func TestComputeDesiredMachineSet(t *testing.T) {
 			"ms-label-1":                           "ms-value-1",
 		}
 		existingMS.Annotations = nil
+		// Pre-existing finalizer should be preserved.
+		existingMS.Finalizers = []string{"pre-existing-finalizer"}
 		existingMS.Spec.Template.Labels = map[string]string{
 			clusterv1.MachineDeploymentUniqueLabel: uniqueID,
 			"ms-label-2":                           "ms-value-2",
@@ -657,6 +659,9 @@ func TestComputeDesiredMachineSet(t *testing.T) {
 		expectedMS.UID = existingMSUID
 		expectedMS.Name = deployment.Name + "-" + uniqueID
 		expectedMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
+		// Pre-existing finalizer should be preserved.
+		expectedMS.Finalizers = []string{"pre-existing-finalizer"}
+
 		expectedMS.Spec.Template.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
 
 		g := NewWithT(t)
@@ -676,6 +681,8 @@ func TestComputeDesiredMachineSet(t *testing.T) {
 			"ms-label-1":                           "ms-value-1",
 		}
 		existingMS.Annotations = nil
+		// Pre-existing finalizer should be preserved.
+		existingMS.Finalizers = []string{"pre-existing-finalizer"}
 		existingMS.Spec.Template.Labels = map[string]string{
 			clusterv1.MachineDeploymentUniqueLabel: uniqueID,
 			"ms-label-2":                           "ms-value-2",
@@ -698,6 +705,8 @@ func TestComputeDesiredMachineSet(t *testing.T) {
 		expectedMS.UID = existingMSUID
 		expectedMS.Name = deployment.Name + "-" + uniqueID
 		expectedMS.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
+		// Pre-existing finalizer should be preserved.
+		expectedMS.Finalizers = []string{"pre-existing-finalizer"}
 		expectedMS.Spec.Template.Labels[clusterv1.MachineDeploymentUniqueLabel] = uniqueID
 
 		g := NewWithT(t)
@@ -765,6 +774,9 @@ func assertMachineSet(g *WithT, actualMS *clusterv1.MachineSet, expectedMS *clus
 	// Check Namespace
 	g.Expect(actualMS.Namespace).Should(Equal(expectedMS.Namespace))
 
+	// Check finalizers
+	g.Expect(actualMS.Finalizers).Should(Equal(expectedMS.Finalizers))
+
 	// Check Replicas
 	g.Expect(actualMS.Spec.Replicas).ShouldNot(BeNil())
 	g.Expect(actualMS.Spec.Replicas).Should(HaveValue(Equal(*expectedMS.Spec.Replicas)))

internal/controllers/machineset/machineset_controller.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/client-go/tools/record"
@@ -636,10 +637,18 @@ func (r *Reconciler) computeDesiredMachine(machineSet *clusterv1.MachineSet, exi
 	if existingMachine != nil {
 		desiredMachine.SetName(existingMachine.Name)
 		desiredMachine.SetUID(existingMachine.UID)
+
+		// Preserve all existing finalizers (including foregroundDeletion finalizer).
+		finalizers := existingMachine.Finalizers
+		// Ensure MachineFinalizer is set.
+		if !sets.New[string](finalizers...).Has(clusterv1.MachineFinalizer) {
+			finalizers = append(finalizers, clusterv1.MachineFinalizer)
+		}
+		desiredMachine.Finalizers = finalizers
+
 		desiredMachine.Spec.Bootstrap.ConfigRef = existingMachine.Spec.Bootstrap.ConfigRef
 		desiredMachine.Spec.InfrastructureRef = existingMachine.Spec.InfrastructureRef
 	}
-
 	// Set the in-place mutable fields.
 	// When we create a new Machine we will just create the Machine with those fields.
 	// When we update an existing Machine will we update the fields on the existing Machine (in-place mutate).

internal/controllers/machineset/machineset_controller_test.go

@@ -1608,6 +1608,8 @@ func TestComputeDesiredMachine(t *testing.T) {
 	existingMachine.UID = "abc-123-existing-machine-1"
 	existingMachine.Labels = nil
 	existingMachine.Annotations = nil
+	// Pre-existing finalizer should be preserved.
+	existingMachine.Finalizers = []string{"pre-existing-finalizer"}
 	existingMachine.Spec.InfrastructureRef = corev1.ObjectReference{
 		Kind:       "GenericInfrastructureMachine",
 		Name:       "infra-machine-1",
@@ -1625,6 +1627,8 @@ func TestComputeDesiredMachine(t *testing.T) {
 	expectedUpdatedMachine := skeletonMachine.DeepCopy()
 	expectedUpdatedMachine.Name = existingMachine.Name
 	expectedUpdatedMachine.UID = existingMachine.UID
+	// Pre-existing finalizer should be preserved.
+	expectedUpdatedMachine.Finalizers = []string{"pre-existing-finalizer", clusterv1.MachineFinalizer}
 	expectedUpdatedMachine.Spec.InfrastructureRef = *existingMachine.Spec.InfrastructureRef.DeepCopy()
 	expectedUpdatedMachine.Spec.Bootstrap.ConfigRef = existingMachine.Spec.Bootstrap.ConfigRef.DeepCopy()
 

test/e2e/md_scale.go

@@ -25,6 +25,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/cluster-api/test/framework"
@@ -123,6 +124,17 @@ func MachineDeploymentScaleSpec(ctx context.Context, inputGetter func() MachineD
 			Replicas:                  1,
 			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
 		})
+
+		By("Deleting the MachineDeployment with foreground deletion")
+		foreground := metav1.DeletePropagationForeground
+		framework.DeleteAndWaitMachineDeployment(ctx, framework.DeleteAndWaitMachineDeploymentInput{
+			ClusterProxy:              input.BootstrapClusterProxy,
+			Cluster:                   clusterResources.Cluster,
+			MachineDeployment:         clusterResources.MachineDeployments[0],
+			WaitForMachineDeployments: input.E2EConfig.GetIntervals(specName, "wait-worker-nodes"),
+			DeletePropagationPolicy:   &foreground,
+		})
+
 		By("PASSED!")
 	})
 

test/framework/machinedeployment_helpers.go

@@ -25,6 +25,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/pkg/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -448,6 +449,46 @@ func machineDeploymentOptions(deployment clusterv1.MachineDeployment) []client.L
 	}
 }
 
+// DeleteAndWaitMachineDeploymentInput is the input for DeleteAndWaitMachineDeployment.
+type DeleteAndWaitMachineDeploymentInput struct {
+	ClusterProxy              ClusterProxy
+	Cluster                   *clusterv1.Cluster
+	MachineDeployment         *clusterv1.MachineDeployment
+	WaitForMachineDeployments []interface{}
+	DeletePropagationPolicy   *metav1.DeletionPropagation
+}
+
+// DeleteAndWaitMachineDeployment deletes MachineDeployment and waits until it is gone.
+func DeleteAndWaitMachineDeployment(ctx context.Context, input DeleteAndWaitMachineDeploymentInput) {
+	Expect(ctx).NotTo(BeNil(), "ctx is required for DeleteAndWaitMachineDeployment")
+	Expect(input.ClusterProxy).ToNot(BeNil(), "Invalid argument. input.ClusterProxy can't be nil when calling DeleteAndWaitMachineDeployment")
+	Expect(input.Cluster).ToNot(BeNil(), "Invalid argument. input.Cluster can't be nil when calling DeleteAndWaitMachineDeployment")
+	Expect(input.MachineDeployment).ToNot(BeNil(), "Invalid argument. input.MachineDeployment can't be nil when calling DeleteAndWaitMachineDeployment")
+
+	log.Logf("Deleting MachineDeployment %s", klog.KObj(input.MachineDeployment))
+	Expect(input.ClusterProxy.GetClient().Delete(ctx, input.MachineDeployment, &client.DeleteOptions{PropagationPolicy: input.DeletePropagationPolicy})).To(Succeed())
+
+	log.Logf("Waiting for MD to be deleted")
+	Eventually(func(g Gomega) {
+		selectorMap, err := metav1.LabelSelectorAsMap(&input.MachineDeployment.Spec.Selector)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		machines := &clusterv1.MachineList{}
+		err = input.ClusterProxy.GetClient().List(ctx, machines, client.InNamespace(input.Cluster.Namespace), client.MatchingLabels(selectorMap))
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(machines.Items).To(BeEmpty())
+
+		ms := &clusterv1.MachineSetList{}
+		err = input.ClusterProxy.GetClient().List(ctx, ms, client.InNamespace(input.Cluster.Namespace), client.MatchingLabels(selectorMap))
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(ms.Items).To(BeEmpty())
+
+		md := &clusterv1.MachineDeployment{}
+		err = input.ClusterProxy.GetClient().Get(ctx, client.ObjectKeyFromObject(input.MachineDeployment), md)
+		g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
+	}, input.WaitForMachineDeployments...).Should(Succeed(), "Timed out waiting for Machine Deployment deletion")
+}
+
 // ScaleAndWaitMachineDeploymentInput is the input for ScaleAndWaitMachineDeployment.
 type ScaleAndWaitMachineDeploymentInput struct {
 	ClusterProxy              ClusterProxy