MD controller: remove redundant defaulting + fix overwrite of local changes

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

api/v1beta1/machinedeployment_webhook.go

@@ -48,7 +48,64 @@ var _ webhook.Validator = &MachineDeployment{}
 
 // Default implements webhook.Defaulter so a webhook will be registered for the type.
 func (m *MachineDeployment) Default() {
-	PopulateDefaultsMachineDeployment(m)
+	if m.Labels == nil {
+		m.Labels = make(map[string]string)
+	}
+	m.Labels[ClusterNameLabel] = m.Spec.ClusterName
+
+	if m.Spec.MinReadySeconds == nil {
+		m.Spec.MinReadySeconds = pointer.Int32(0)
+	}
+
+	if m.Spec.RevisionHistoryLimit == nil {
+		m.Spec.RevisionHistoryLimit = pointer.Int32(1)
+	}
+
+	if m.Spec.ProgressDeadlineSeconds == nil {
+		m.Spec.ProgressDeadlineSeconds = pointer.Int32(600)
+	}
+
+	if m.Spec.Selector.MatchLabels == nil {
+		m.Spec.Selector.MatchLabels = make(map[string]string)
+	}
+
+	if m.Spec.Strategy == nil {
+		m.Spec.Strategy = &MachineDeploymentStrategy{}
+	}
+
+	if m.Spec.Strategy.Type == "" {
+		m.Spec.Strategy.Type = RollingUpdateMachineDeploymentStrategyType
+	}
+
+	if m.Spec.Template.Labels == nil {
+		m.Spec.Template.Labels = make(map[string]string)
+	}
+
+	// Default RollingUpdate strategy only if strategy type is RollingUpdate.
+	if m.Spec.Strategy.Type == RollingUpdateMachineDeploymentStrategyType {
+		if m.Spec.Strategy.RollingUpdate == nil {
+			m.Spec.Strategy.RollingUpdate = &MachineRollingUpdateDeployment{}
+		}
+		if m.Spec.Strategy.RollingUpdate.MaxSurge == nil {
+			ios1 := intstr.FromInt(1)
+			m.Spec.Strategy.RollingUpdate.MaxSurge = &ios1
+		}
+		if m.Spec.Strategy.RollingUpdate.MaxUnavailable == nil {
+			ios0 := intstr.FromInt(0)
+			m.Spec.Strategy.RollingUpdate.MaxUnavailable = &ios0
+		}
+	}
+
+	// If no selector has been provided, add label and selector for the
+	// MachineDeployment's name as a default way of providing uniqueness.
+	if len(m.Spec.Selector.MatchLabels) == 0 && len(m.Spec.Selector.MatchExpressions) == 0 {
+		m.Spec.Selector.MatchLabels[MachineDeploymentNameLabel] = m.Name
+		m.Spec.Template.Labels[MachineDeploymentNameLabel] = m.Name
+	}
+	// Make sure selector and template to be in the same cluster.
+	m.Spec.Selector.MatchLabels[ClusterNameLabel] = m.Spec.ClusterName
+	m.Spec.Template.Labels[ClusterNameLabel] = m.Spec.ClusterName
+
 	// tolerate version strings without a "v" prefix: prepend it if it's not there
 	if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") {
 		normalizedVersion := "v" + *m.Spec.Template.Spec.Version
@@ -156,65 +213,3 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error {
 
 	return apierrors.NewInvalid(GroupVersion.WithKind("MachineDeployment").GroupKind(), m.Name, allErrs)
 }
-
-// PopulateDefaultsMachineDeployment fills in default field values.
-// This is also called during MachineDeployment sync.
-func PopulateDefaultsMachineDeployment(d *MachineDeployment) {
-	if d.Labels == nil {
-		d.Labels = make(map[string]string)
-	}
-	d.Labels[ClusterNameLabel] = d.Spec.ClusterName
-
-	if d.Spec.MinReadySeconds == nil {
-		d.Spec.MinReadySeconds = pointer.Int32(0)
-	}
-
-	if d.Spec.RevisionHistoryLimit == nil {
-		d.Spec.RevisionHistoryLimit = pointer.Int32(1)
-	}
-
-	if d.Spec.ProgressDeadlineSeconds == nil {
-		d.Spec.ProgressDeadlineSeconds = pointer.Int32(600)
-	}
-
-	if d.Spec.Selector.MatchLabels == nil {
-		d.Spec.Selector.MatchLabels = make(map[string]string)
-	}
-
-	if d.Spec.Strategy == nil {
-		d.Spec.Strategy = &MachineDeploymentStrategy{}
-	}
-
-	if d.Spec.Strategy.Type == "" {
-		d.Spec.Strategy.Type = RollingUpdateMachineDeploymentStrategyType
-	}
-
-	if d.Spec.Template.Labels == nil {
-		d.Spec.Template.Labels = make(map[string]string)
-	}
-
-	// Default RollingUpdate strategy only if strategy type is RollingUpdate.
-	if d.Spec.Strategy.Type == RollingUpdateMachineDeploymentStrategyType {
-		if d.Spec.Strategy.RollingUpdate == nil {
-			d.Spec.Strategy.RollingUpdate = &MachineRollingUpdateDeployment{}
-		}
-		if d.Spec.Strategy.RollingUpdate.MaxSurge == nil {
-			ios1 := intstr.FromInt(1)
-			d.Spec.Strategy.RollingUpdate.MaxSurge = &ios1
-		}
-		if d.Spec.Strategy.RollingUpdate.MaxUnavailable == nil {
-			ios0 := intstr.FromInt(0)
-			d.Spec.Strategy.RollingUpdate.MaxUnavailable = &ios0
-		}
-	}
-
-	// If no selector has been provided, add label and selector for the
-	// MachineDeployment's name as a default way of providing uniqueness.
-	if len(d.Spec.Selector.MatchLabels) == 0 && len(d.Spec.Selector.MatchExpressions) == 0 {
-		d.Spec.Selector.MatchLabels[MachineDeploymentNameLabel] = d.Name
-		d.Spec.Template.Labels[MachineDeploymentNameLabel] = d.Name
-	}
-	// Make sure selector and template to be in the same cluster.
-	d.Spec.Selector.MatchLabels[ClusterNameLabel] = d.Spec.ClusterName
-	d.Spec.Template.Labels[ClusterNameLabel] = d.Spec.ClusterName
-}

docs/book/src/developer/providers/v1.3-to-v1.4.md

@@ -30,6 +30,7 @@ maintainers of providers and consumers of our Go API.
 - `exp/runtime/server.NewServer` has been removed.
 - `--disable-no-echo` option from `clusterctl describe cluster` subcommand has been removed
 - `api/v1beta1.ClusterTopologyManagedFieldsAnnotation` field has been removed.
+- `api/v1beta1.PopulateDefaultsMachineDeployment` func has been removed.
 
 ### API Changes
 

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -250,15 +250,18 @@ func TestMachineDeploymentReconciler(t *testing.T) {
 		//
 		secondMachineSet := machineSets.Items[0]
 		t.Log("Scaling the MachineDeployment to 3 replicas")
-		modifyFunc := func(d *clusterv1.MachineDeployment) { d.Spec.Replicas = pointer.Int32(3) }
+		desiredMachineDeploymentReplicas := int32(3)
+		modifyFunc := func(d *clusterv1.MachineDeployment) {
+			d.Spec.Replicas = pointer.Int32(desiredMachineDeploymentReplicas)
+		}
 		g.Expect(updateMachineDeployment(ctx, env, deployment, modifyFunc)).To(Succeed())
 		g.Eventually(func() int {
 			key := client.ObjectKey{Name: secondMachineSet.Name, Namespace: secondMachineSet.Namespace}
 			if err := env.Get(ctx, key, &secondMachineSet); err != nil {
 				return -1
 			}
 			return int(*secondMachineSet.Spec.Replicas)
-		}, timeout).Should(BeEquivalentTo(3))
+		}, timeout).Should(BeEquivalentTo(desiredMachineDeploymentReplicas))
 
 		//
 		// Update a MachineDeployment, expect Reconcile to be called and a new MachineSet to appear.
@@ -393,7 +396,7 @@ func TestMachineDeploymentReconciler(t *testing.T) {
 			if err := env.List(ctx, machineSets, listOpts); err != nil {
 				return false
 			}
-			return machineSets.Items[0].Status.Replicas == *deployment.Spec.Replicas
+			return machineSets.Items[0].Status.Replicas == desiredMachineDeploymentReplicas
 		}, timeout*5).Should(BeTrue())
 
 		t.Log("Verifying MachineSets with old labels are deleted")

internal/controllers/machinedeployment/machinedeployment_sync.go

@@ -532,16 +532,20 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, d *clusterv1.M
 
 // We have this as standalone variant to be able to use it from the tests.
 func updateMachineDeployment(ctx context.Context, c client.Client, d *clusterv1.MachineDeployment, modify func(*clusterv1.MachineDeployment)) error {
+	mdObjectKey := util.ObjectKey(d)
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		if err := c.Get(ctx, util.ObjectKey(d), d); err != nil {
+		// Note: We intentionally don't re-use the passed in MachineDeployment d here as that would
+		// overwrite any local changes we might have previously made to the MachineDeployment with the version
+		// we get here from the apiserver.
+		md := &clusterv1.MachineDeployment{}
+		if err := c.Get(ctx, mdObjectKey, md); err != nil {
 			return err
 		}
-		patchHelper, err := patch.NewHelper(d, c)
+		patchHelper, err := patch.NewHelper(md, c)
 		if err != nil {
 			return err
 		}
-		clusterv1.PopulateDefaultsMachineDeployment(d)
-		modify(d)
-		return patchHelper.Patch(ctx, d)
+		modify(md)
+		return patchHelper.Patch(ctx, md)
 	})
 }