Merge pull request #1380 from detiber/ImprovePatching

🐛 Improve handling of patching and updates in MachineSet and MachineDeployment controllers

controllers/machinedeployment_controller.go

@@ -106,6 +106,8 @@ func (r *MachineDeploymentReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
 func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv1.MachineDeployment) (ctrl.Result, error) {
 	clusterv1.PopulateDefaultsMachineDeployment(d)
 
+	// Test for an empty LabelSelector and short circuit if that is the case
+	// TODO: When we have validation webhooks, we should likely reject on an empty LabelSelector
 	everything := metav1.LabelSelector{}
 	if reflect.DeepEqual(d.Spec.Selector, &everything) {
 		if d.Status.ObservedGeneration < d.Generation {
@@ -116,7 +118,7 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv
 				return ctrl.Result{}, err
 			}
 		}
-		return ctrl.Result{Requeue: true}, nil
+		return ctrl.Result{}, nil
 	}
 
 	// Make sure that label selector can match the template's labels.
@@ -144,12 +146,17 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv
 	}
 
 	if cluster != nil && r.shouldAdopt(d) {
+		patch := client.MergeFrom(d.DeepCopy())
 		d.OwnerReferences = util.EnsureOwnerRef(d.OwnerReferences, metav1.OwnerReference{
 			APIVersion: cluster.APIVersion,
 			Kind:       cluster.Kind,
 			Name:       cluster.Name,
 			UID:        cluster.UID,
 		})
+		// Patch using a deep copy to avoid overwriting any unexpected Status changes from the returned result
+		if err := r.Client.Patch(ctx, d.DeepCopy(), patch); err != nil {
+			return ctrl.Result{}, errors.Wrapf(err, "Failed to add OwnerReference to MachineDeployment %s/%s", d.Namespace, d.Name)
+		}
 	}
 
 	msList, err := r.getMachineSetsForDeployment(d)

controllers/machinedeployment_sync.go

@@ -103,14 +103,15 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(d *clusterv1.MachineDeplo
 	// latest revision.
 	if existingNewMS != nil {
 		msCopy := existingNewMS.DeepCopy()
+		patch := client.MergeFrom(msCopy)
 
 		// Set existing new machine set's annotation
 		annotationsUpdated := mdutil.SetNewMachineSetAnnotations(d, msCopy, newRevision, true)
 
 		minReadySecondsNeedsUpdate := msCopy.Spec.MinReadySeconds != *d.Spec.MinReadySeconds
 		if annotationsUpdated || minReadySecondsNeedsUpdate {
 			msCopy.Spec.MinReadySeconds = *d.Spec.MinReadySeconds
-			return nil, r.Client.Update(context.Background(), msCopy)
+			return nil, r.Client.Patch(context.Background(), msCopy, patch)
 		}
 
 		// Apply revision annotation from existingNewMS if it is missing from the deployment.
@@ -341,7 +342,8 @@ func (r *MachineDeploymentReconciler) syncDeploymentStatus(allMSs []*clusterv1.M
 
 	patch := client.MergeFrom(d.DeepCopy())
 	d.Status = newStatus
-	return r.Client.Status().Patch(context.Background(), d, patch)
+	// Patch using a deep copy to avoid overwriting any unexpected Spec/Metadata changes from the returned result
+	return r.Client.Status().Patch(context.Background(), d.DeepCopy(), patch)
 }
 
 // calculateStatus calculates the latest status for the provided deployment by looking into the provided machine sets.
@@ -520,7 +522,7 @@ func updateMachineDeployment(c client.Client, d *clusterv1.MachineDeployment, mo
 		clusterv1.PopulateDefaultsMachineDeployment(d)
 		// Apply modifications.
 		modify(d)
-		// Patch the MachineDeployment.
-		return c.Patch(context.Background(), d, patch)
+		// Patch using a deep copy to avoid overwriting any unexpected Spec/Metadata changes from the returned result
+		return c.Patch(context.Background(), d.DeepCopy(), patch)
 	})
 }

controllers/machineset_controller.go

@@ -386,9 +386,10 @@ func shouldExcludeMachine(machineSet *clusterv1.MachineSet, machine *clusterv1.M
 
 // adoptOrphan sets the MachineSet as a controller OwnerReference to the Machine.
 func (r *MachineSetReconciler) adoptOrphan(machineSet *clusterv1.MachineSet, machine *clusterv1.Machine) error {
+	patch := client.MergeFrom(machine)
 	newRef := *metav1.NewControllerRef(machineSet, machineSetKind)
 	machine.OwnerReferences = append(machine.OwnerReferences, newRef)
-	return r.Client.Update(context.Background(), machine)
+	return r.Client.Patch(context.Background(), machine, patch)
 }
 
 func (r *MachineSetReconciler) waitForMachineCreation(machineList []*clusterv1.Machine) error {