diff --git a/internal/controllers/machinedeployment/machinedeployment_sync.go b/internal/controllers/machinedeployment/machinedeployment_sync.go index ba513922f2c6..8f4fb8cc50a0 100644 --- a/internal/controllers/machinedeployment/machinedeployment_sync.go +++ b/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 diff --git a/internal/controllers/machinedeployment/machinedeployment_sync_test.go b/internal/controllers/machinedeployment/machinedeployment_sync_test.go index dbc7d13a269b..9169f1882e40 100644 --- a/internal/controllers/machinedeployment/machinedeployment_sync_test.go +++ b/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))) diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index dc4b1c1a1a26..85254155bb8a 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/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). diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index b490632e4c13..9e11d8ec3e2d 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/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() diff --git a/test/e2e/md_scale.go b/test/e2e/md_scale.go index 205bfa06a740..89bacdf4a977 100644 --- a/test/e2e/md_scale.go +++ b/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!") }) diff --git a/test/framework/machinedeployment_helpers.go b/test/framework/machinedeployment_helpers.go index 4b1b6b0a6fba..3dc15faa8889 100644 --- a/test/framework/machinedeployment_helpers.go +++ b/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