Merge pull request #12788 from sbueringer/pr-remove-ssa-cleanup

🌱 Remove unused CleanUpManagedFieldsForSSAAdoption code

Author: k8s-ci-robot

controlplane/kubeadm/internal/controllers/controller.go

@@ -797,13 +797,6 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 			continue
 		}
 
-		// Cleanup managed fields of all Machines.
-		// We do this so that Machines that were created/patched before the controller adopted Server-Side-Apply (SSA)
-		// (< v1.4.0) can also work with SSA. Otherwise, fields would be co-owned by our "old" "manager" and
-		// "capi-kubeadmcontrolplane" and then we would not be able to e.g. drop labels and annotations.
-		if err := ssa.CleanUpManagedFieldsForSSAAdoption(ctx, r.Client, m, kcpManagerName); err != nil {
-			return errors.Wrapf(err, "failed to update Machine: failed to adjust the managedFields of the Machine %s", klog.KObj(m))
-		}
 		// Update Machine to propagate in-place mutable fields from KCP.
 		updatedMachine, err := r.updateMachine(ctx, m, controlPlane.KCP, controlPlane.Cluster)
 		if err != nil {

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -1803,16 +1803,15 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			InfrastructureRef: *infraMachineRef,
 			Version:           "v1.25.3",
 			FailureDomain:     fd,
-			ProviderID:        "provider-id",
+			// ProviderID is intentionally not set here, this field is set by the Machine controller.
 			Deletion: clusterv1.MachineDeletionSpec{
 				NodeDrainTimeoutSeconds:        duration5s,
 				NodeVolumeDetachTimeoutSeconds: duration5s,
 				NodeDeletionTimeoutSeconds:     duration5s,
 			},
 		},
 	}
-	// Note: use "manager" as the field owner to mimic the manager used before ClusterAPI v1.4.0.
-	g.Expect(env.Create(ctx, inPlaceMutatingMachine, client.FieldOwner("manager"))).To(Succeed())
+	g.Expect(env.PatchAndWait(ctx, inPlaceMutatingMachine, client.FieldOwner(kcpManagerName))).To(Succeed())
 
 	// Existing machine that is in deleting state
 	deletingMachine := &clusterv1.Machine{
@@ -1845,7 +1844,7 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 			ReadinessGates: mandatoryMachineReadinessGates,
 		},
 	}
-	g.Expect(env.Create(ctx, deletingMachine, client.FieldOwner(classicManager))).To(Succeed())
+	g.Expect(env.PatchAndWait(ctx, deletingMachine, client.FieldOwner(kcpManagerName))).To(Succeed())
 	// Delete the machine to put it in the deleting state
 	g.Expect(env.Delete(ctx, deletingMachine)).To(Succeed())
 	// Wait till the machine is marked for deletion
@@ -2094,13 +2093,13 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 	g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(updatedDeletingMachine), updatedDeletingMachine)).To(Succeed())
 
 	// Verify ManagedFields
-	g.Expect(updatedDeletingMachine.ManagedFields).ShouldNot(
+	g.Expect(updatedDeletingMachine.ManagedFields).Should(
 		ContainElement(ssa.MatchManagedFieldsEntry(kcpManagerName, metav1.ManagedFieldsOperationApply)),
-		"deleting machine should not contain an entry for SSA manager",
+		"deleting machine should contain an entry for SSA manager",
 	)
-	g.Expect(updatedDeletingMachine.ManagedFields).Should(
-		ContainElement(ssa.MatchManagedFieldsEntry("manager", metav1.ManagedFieldsOperationUpdate)),
-		"in-place mutable machine should still contain an entry for old manager",
+	g.Expect(updatedDeletingMachine.ManagedFields).ShouldNot(
+		ContainElement(ssa.MatchManagedFieldsEntry(classicManager, metav1.ManagedFieldsOperationUpdate)),
+		"deleting machine should not contain an entry for old manager",
 	)
 
 	// Verify the machine labels and annotations are unchanged.

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -286,19 +286,6 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope) error {
 		}
 	}
 
-	// Loop over all MachineSets and cleanup managed fields.
-	// We do this so that MachineSets that were created/patched before (< v1.4.0) the controller adopted
-	// Server-Side-Apply (SSA) can also work with SSA. Otherwise, fields would be co-owned by our "old" "manager" and
-	// "capi-machinedeployment" and then we would not be able to e.g. drop labels and annotations.
-	// Note: We are cleaning up managed fields for all MachineSets, so we're able to remove this code in a few
-	// Cluster API releases. If we do this only for selected MachineSets, we would have to keep this code forever.
-	for idx := range s.machineSets {
-		machineSet := s.machineSets[idx]
-		if err := ssa.CleanUpManagedFieldsForSSAAdoption(ctx, r.Client, machineSet, machineDeploymentManagerName); err != nil {
-			return errors.Wrapf(err, "failed to clean up managedFields of MachineSet %s", klog.KObj(machineSet))
-		}
-	}
-
 	templateExists := s.infrastructureTemplateExists && (!md.Spec.Template.Spec.Bootstrap.ConfigRef.IsDefined() || s.bootstrapTemplateExists)
 
 	if ptr.Deref(md.Spec.Paused, false) {

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -33,7 +33,6 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
 	"sigs.k8s.io/cluster-api/controllers/external"
-	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
 	v1beta1conditions "sigs.k8s.io/cluster-api/util/conditions/deprecated/v1beta1"
 	"sigs.k8s.io/cluster-api/util/patch"
@@ -500,196 +499,6 @@ func TestMachineDeploymentReconciler(t *testing.T) {
 	})
 }
 
-func TestMachineDeploymentReconciler_CleanUpManagedFieldsForSSAAdoption(t *testing.T) {
-	setup := func(t *testing.T, g *WithT) (*corev1.Namespace, *clusterv1.Cluster) {
-		t.Helper()
-
-		t.Log("Creating the namespace")
-		ns, err := env.CreateNamespace(ctx, machineDeploymentNamespace)
-		g.Expect(err).ToNot(HaveOccurred())
-
-		t.Log("Creating the Cluster")
-		cluster := &clusterv1.Cluster{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: ns.Name,
-				Name:      "test-cluster",
-			},
-			Spec: clusterv1.ClusterSpec{
-				ControlPlaneRef: clusterv1.ContractVersionedObjectReference{
-					APIGroup: builder.ControlPlaneGroupVersion.Group,
-					Kind:     builder.GenericControlPlaneKind,
-					Name:     "cp1",
-				},
-			},
-		}
-		g.Expect(env.Create(ctx, cluster)).To(Succeed())
-
-		t.Log("Creating the Cluster Kubeconfig Secret")
-		g.Expect(env.CreateKubeconfigSecret(ctx, cluster)).To(Succeed())
-
-		return ns, cluster
-	}
-
-	teardown := func(t *testing.T, g *WithT, ns *corev1.Namespace, cluster *clusterv1.Cluster) {
-		t.Helper()
-
-		t.Log("Deleting the Cluster")
-		g.Expect(env.Delete(ctx, cluster)).To(Succeed())
-		t.Log("Deleting the namespace")
-		g.Expect(env.Delete(ctx, ns)).To(Succeed())
-	}
-
-	g := NewWithT(t)
-	namespace, testCluster := setup(t, g)
-	defer teardown(t, g, namespace, testCluster)
-
-	labels := map[string]string{
-		"foo":                      "bar",
-		clusterv1.ClusterNameLabel: testCluster.Name,
-	}
-	version := "v1.10.3"
-	deployment := &clusterv1.MachineDeployment{
-		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: "md-",
-			Namespace:    namespace.Name,
-			Labels: map[string]string{
-				clusterv1.ClusterNameLabel: testCluster.Name,
-			},
-		},
-		Spec: clusterv1.MachineDeploymentSpec{
-			Paused:      ptr.To(true), // Set this to true as we do not want to test the other parts of the reconciler in this test.
-			ClusterName: testCluster.Name,
-			Replicas:    ptr.To[int32](2),
-			Selector: metav1.LabelSelector{
-				// We're using the same labels for spec.selector and spec.template.labels.
-				MatchLabels: labels,
-			},
-			Rollout: clusterv1.MachineDeploymentRolloutSpec{
-				Strategy: clusterv1.MachineDeploymentRolloutStrategy{
-					Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
-					RollingUpdate: clusterv1.MachineDeploymentRolloutStrategyRollingUpdate{
-						MaxUnavailable: intOrStrPtr(0),
-						MaxSurge:       intOrStrPtr(1),
-					},
-				},
-			},
-			Deletion: clusterv1.MachineDeploymentDeletionSpec{
-				Order: clusterv1.OldestMachineSetDeletionOrder,
-			},
-			Template: clusterv1.MachineTemplateSpec{
-				ObjectMeta: clusterv1.ObjectMeta{
-					Labels: labels,
-				},
-				Spec: clusterv1.MachineSpec{
-					ClusterName: testCluster.Name,
-					Version:     version,
-					InfrastructureRef: clusterv1.ContractVersionedObjectReference{
-						APIGroup: clusterv1.GroupVersionInfrastructure.Group,
-						Kind:     "GenericInfrastructureMachineTemplate",
-						Name:     "md-template",
-					},
-					Bootstrap: clusterv1.Bootstrap{
-						DataSecretName: ptr.To("data-secret-name"),
-					},
-				},
-			},
-		},
-	}
-	msListOpts := []client.ListOption{
-		client.InNamespace(namespace.Name),
-		client.MatchingLabels(labels),
-	}
-
-	// Create infrastructure template resource.
-	infraResource := map[string]interface{}{
-		"kind":       "GenericInfrastructureMachine",
-		"apiVersion": clusterv1.GroupVersionInfrastructure.String(),
-		"metadata":   map[string]interface{}{},
-		"spec": map[string]interface{}{
-			"size": "3xlarge",
-		},
-	}
-	infraTmpl := &unstructured.Unstructured{
-		Object: map[string]interface{}{
-			"kind":       "GenericInfrastructureMachineTemplate",
-			"apiVersion": clusterv1.GroupVersionInfrastructure.String(),
-			"metadata": map[string]interface{}{
-				"name":      "md-template",
-				"namespace": namespace.Name,
-			},
-			"spec": map[string]interface{}{
-				"template": infraResource,
-			},
-		},
-	}
-	t.Log("Creating the infrastructure template")
-	g.Expect(env.Create(ctx, infraTmpl)).To(Succeed())
-
-	// Create the MachineDeployment object and expect Reconcile to be called.
-	t.Log("Creating the MachineDeployment")
-	g.Expect(env.Create(ctx, deployment)).To(Succeed())
-
-	// Create a MachineSet for the MachineDeployment.
-	classicManagerMS := &clusterv1.MachineSet{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "MachineSet",
-			APIVersion: clusterv1.GroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      deployment.Name + "-" + "classic-ms",
-			Namespace: testCluster.Namespace,
-			Labels:    labels,
-		},
-		Spec: clusterv1.MachineSetSpec{
-			ClusterName: testCluster.Name,
-			Replicas:    ptr.To[int32](0),
-			Selector: metav1.LabelSelector{
-				MatchLabels: labels,
-			},
-			Template: clusterv1.MachineTemplateSpec{
-				ObjectMeta: clusterv1.ObjectMeta{
-					Labels: labels,
-				},
-				Spec: clusterv1.MachineSpec{
-					ClusterName: testCluster.Name,
-					InfrastructureRef: clusterv1.ContractVersionedObjectReference{
-						APIGroup: clusterv1.GroupVersionInfrastructure.Group,
-						Kind:     "GenericInfrastructureMachineTemplate",
-						Name:     "md-template",
-					},
-					Bootstrap: clusterv1.Bootstrap{
-						DataSecretName: ptr.To("data-secret-name"),
-					},
-					Version: version,
-				},
-			},
-		},
-	}
-	ssaManagerMS := classicManagerMS.DeepCopy()
-	ssaManagerMS.Name = deployment.Name + "-" + "ssa-ms"
-
-	// Create one using the "old manager".
-	g.Expect(env.Create(ctx, classicManagerMS, client.FieldOwner("manager"))).To(Succeed())
-
-	// Create one using SSA.
-	g.Expect(env.Patch(ctx, ssaManagerMS, client.Apply, client.FieldOwner(machineDeploymentManagerName), client.ForceOwnership)).To(Succeed())
-
-	// Verify that for both the MachineSets the ManagedFields are updated.
-	g.Eventually(func(g Gomega) {
-		machineSets := &clusterv1.MachineSetList{}
-		g.Expect(env.List(ctx, machineSets, msListOpts...)).To(Succeed())
-
-		g.Expect(machineSets.Items).To(HaveLen(2))
-		for _, ms := range machineSets.Items {
-			// Verify the ManagedFields are updated.
-			g.Expect(ms.GetManagedFields()).Should(
-				ContainElement(ssa.MatchManagedFieldsEntry(machineDeploymentManagerName, metav1.ManagedFieldsOperationApply)))
-			g.Expect(ms.GetManagedFields()).ShouldNot(
-				ContainElement(ssa.MatchManagedFieldsEntry("manager", metav1.ManagedFieldsOperationUpdate)))
-		}
-	}).Should(Succeed())
-}
-
 func TestMachineSetToDeployments(t *testing.T) {
 	g := NewWithT(t)
 

internal/controllers/machineset/machineset_controller.go

@@ -544,14 +544,6 @@ func (r *Reconciler) syncMachines(ctx context.Context, s *scope) (ctrl.Result, e
 			}
 		}
 
-		// Cleanup managed fields of all Machines.
-		// We do this so that Machines that were created/patched before the controller adopted Server-Side-Apply (SSA)
-		// (< v1.4.0) can also work with SSA. Otherwise, fields would be co-owned by our "old" "manager" and
-		// "capi-machineset" and then we would not be able to e.g. drop labels and annotations.
-		if err := ssa.CleanUpManagedFieldsForSSAAdoption(ctx, r.Client, m, machineSetManagerName); err != nil {
-			return ctrl.Result{}, errors.Wrapf(err, "failed to update machine: failed to adjust the managedFields of the Machine %q", m.Name)
-		}
-
 		// Update Machine to propagate in-place mutable fields from the MachineSet.
 		updatedMachine, err := r.computeDesiredMachine(machineSet, m)
 		if err != nil {

internal/controllers/machineset/machineset_controller_test.go

@@ -1244,7 +1244,6 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 			Kind:       "Machine",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			UID:       "abc-123-uid",
 			Name:      "in-place-mutating-machine",
 			Namespace: namespace.Name,
 			Labels: map[string]string{
@@ -1274,15 +1273,14 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 			},
 		},
 	}
-	g.Expect(env.Create(ctx, inPlaceMutatingMachine, client.FieldOwner(classicManager))).To(Succeed())
+	g.Expect(env.PatchAndWait(ctx, inPlaceMutatingMachine, client.FieldOwner(machineSetManagerName))).To(Succeed())
 
 	deletingMachine := &clusterv1.Machine{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: clusterv1.GroupVersion.String(),
 			Kind:       "Machine",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			UID:         "abc-123-uid",
 			Name:        "deleting-machine",
 			Namespace:   namespace.Name,
 			Labels:      map[string]string{},
@@ -1301,7 +1299,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 			},
 		},
 	}
-	g.Expect(env.Create(ctx, deletingMachine, client.FieldOwner(classicManager))).To(Succeed())
+	g.Expect(env.PatchAndWait(ctx, deletingMachine, client.FieldOwner(machineSetManagerName))).To(Succeed())
 	// Delete the machine to put it in the deleting state
 	g.Expect(env.Delete(ctx, deletingMachine)).To(Succeed())
 	// Wait till the machine is marked for deletion
@@ -1476,13 +1474,13 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
 		g.Expect(env.GetAPIReader().Get(ctx, client.ObjectKeyFromObject(updatedDeletingMachine), updatedDeletingMachine)).To(Succeed())
 
 		// Verify ManagedFields
-		g.Expect(updatedDeletingMachine.ManagedFields).ShouldNot(
+		g.Expect(updatedDeletingMachine.ManagedFields).Should(
 			ContainElement(ssa.MatchManagedFieldsEntry(machineSetManagerName, metav1.ManagedFieldsOperationApply)),
-			"deleting machine should not contain an entry for SSA manager",
+			"deleting machine should contain an entry for SSA manager",
 		)
-		g.Expect(updatedDeletingMachine.ManagedFields).Should(
-			ContainElement(ssa.MatchManagedFieldsEntry("manager", metav1.ManagedFieldsOperationUpdate)),
-			"in-place mutable machine should still contain an entry for old manager",
+		g.Expect(updatedDeletingMachine.ManagedFields).ShouldNot(
+			ContainElement(ssa.MatchManagedFieldsEntry(classicManager, metav1.ManagedFieldsOperationUpdate)),
+			"deleting machine should not contain an entry for old manager",
 		)
 
 		// Verify in-place mutable fields are still the same.