From 7139c0dddc16c908399dbfc843b2d50dc4326253 Mon Sep 17 00:00:00 2001
From: Jason DeTiberus <detiberusj@vmware.com>
Date: Thu, 5 Sep 2019 10:18:48 -0400
Subject: [PATCH] Improve handling of patching and updates in MachineSet and
 MachineDeployment controllers

---
 controllers/machinedeployment_controller.go |  8 +++++++-
 controllers/machinedeployment_sync.go       | 10 ++++++----
 controllers/machineset_controller.go        |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/controllers/machinedeployment_controller.go b/controllers/machinedeployment_controller.go
index 2e0380c1065b..e8b0ba22ae72 100644
--- a/controllers/machinedeployment_controller.go
+++ b/controllers/machinedeployment_controller.go
@@ -111,7 +111,8 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv
 		if d.Status.ObservedGeneration < d.Generation {
 			patch := client.MergeFrom(d.DeepCopy())
 			d.Status.ObservedGeneration = d.Generation
-			if err := r.Client.Status().Patch(ctx, d, patch); err != nil {
+			// Patch using a deep copy to avoid overwriting any unexpected Spec/Metadata changes from the returned result
+			if err := r.Client.Status().Patch(ctx, d.DeepCopy(), patch); err != nil {
 				klog.Warningf("Failed to patch status for MachineDeployment %q: %v", d.Name, err)
 				return ctrl.Result{}, err
 			}
@@ -144,12 +145,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)
diff --git a/controllers/machinedeployment_sync.go b/controllers/machinedeployment_sync.go
index 35dac59a5b20..aecc6b1709af 100644
--- a/controllers/machinedeployment_sync.go
+++ b/controllers/machinedeployment_sync.go
@@ -103,6 +103,7 @@ 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)
@@ -110,7 +111,7 @@ func (r *MachineDeploymentReconciler) getNewMachineSet(d *clusterv1.MachineDeplo
 		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)
 	})
 }
diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go
index b62b3daa31c7..15c92706114d 100644
--- a/controllers/machineset_controller.go
+++ b/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 {