fix(deployment): migrate workload update calls to patch

Signed-off-by: Nazih Ben Brahim <nazihbenbrahim9@gmail.com>

Author: Nazihbenbrahim

pkg/controller/deployment/deployment_controller.go

@@ -52,6 +52,13 @@ type DeploymentController struct {
 	runtimeClient client.Client
 }
 
+// patchDeploymentStatus patches the Deployment status subresource using a merge diff.
+// Only changed fields are sent, which avoids overwriting unknown fields from newer API versions.
+func (dc *DeploymentController) patchDeploymentStatus(ctx context.Context, oldD, newD *apps.Deployment) error {
+	patch := client.MergeFrom(oldD)
+	return dc.runtimeClient.Status().Patch(ctx, newD, patch)
+}
+
 // getReplicaSetsForDeployment uses ControllerRefManager to reconcile
 // ControllerRef by adopting and orphaning.
 // It returns the list of ReplicaSets that this Deployment should manage.
@@ -100,10 +107,13 @@ func (dc *DeploymentController) syncDeployment(ctx context.Context, deployment *
 	if reflect.DeepEqual(d.Spec.Selector, &everything) {
 		dc.eventRecorder.Eventf(d, v1.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty selector is required.")
 		if d.Status.ObservedGeneration < d.Generation {
-			d.Status.ObservedGeneration = d.Generation
-			err := dc.runtimeClient.Status().Update(ctx, d)
-			if err != nil {
-				klog.Errorf("Failed to update deployment status: %v", err)
+			// Use Patch instead of Update to avoid erasing unknown status fields
+			// that a newer API server may have set (version-skew safety).
+			dCopy := d.DeepCopy()
+			dCopy.Status.ObservedGeneration = d.Generation
+			if err := dc.runtimeClient.Status().Patch(ctx, dCopy, client.MergeFrom(d)); err != nil {
+
+				klog.Errorf("Failed to patch deployment status: %v", err)
 			}
 		}
 		return nil

pkg/controller/deployment/deployment_controller_test.go

@@ -26,6 +26,8 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/pointer"
@@ -236,3 +238,39 @@ func TestSyncDeployment(t *testing.T) {
 		})
 	}
 }
+
+func TestSyncDeploymentSelectingAll(t *testing.T) {
+	deployment := generateDeployment("busybox")
+	deployment.Spec.Selector = &metav1.LabelSelector{}
+	deployment.Generation = 7
+	deployment.Status.ObservedGeneration = 3
+	deployment.Annotations = map[string]string{
+		"test-unknown-field": "kept",
+	}
+
+	fakeCtrlClient := ctrlfake.NewClientBuilder().
+		WithStatusSubresource(&apps.Deployment{}).
+		WithObjects(&deployment).
+		Build()
+
+	dc := &DeploymentController{
+		eventRecorder: record.NewFakeRecorder(10),
+		runtimeClient: fakeCtrlClient,
+	}
+
+	if err := dc.syncDeployment(context.TODO(), &deployment); err != nil {
+		t.Fatalf("syncDeployment returned unexpected error: %v", err)
+	}
+
+	var result apps.Deployment
+	if err := fakeCtrlClient.Get(context.TODO(), ctrlclient.ObjectKeyFromObject(&deployment), &result); err != nil {
+		t.Fatalf("get deployment failed: %v", err)
+	}
+
+	if result.Status.ObservedGeneration != result.Generation {
+		t.Fatalf("expected observedGeneration=%d, got %d", result.Generation, result.Status.ObservedGeneration)
+	}
+	if got := result.Annotations["test-unknown-field"]; got != "kept" {
+		t.Fatalf("expected annotation test-unknown-field=kept, got %q", got)
+	}
+}

pkg/controller/deployment/status_patch_test.go

@@ -0,0 +1,113 @@
+/*
+Copyright 2022 The Kruise Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package deployment
+
+import (
+	"context"
+	"testing"
+
+	apps "k8s.io/api/apps/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+type statusCallCounter struct {
+	patchCalls  int
+	updateCalls int
+}
+
+type statusCountingClient struct {
+	ctrlclient.Client
+	counter statusCallCounter
+}
+
+func (c *statusCountingClient) Status() ctrlclient.SubResourceWriter {
+	return &statusCountingWriter{
+		SubResourceWriter: c.Client.Status(),
+		counter:           &c.counter,
+	}
+}
+
+type statusCountingWriter struct {
+	ctrlclient.SubResourceWriter
+	counter *statusCallCounter
+}
+
+func (w *statusCountingWriter) Update(ctx context.Context, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error {
+	w.counter.updateCalls++
+	return w.SubResourceWriter.Update(ctx, obj, opts...)
+}
+
+func (w *statusCountingWriter) Patch(ctx context.Context, obj ctrlclient.Object, patch ctrlclient.Patch, opts ...ctrlclient.SubResourcePatchOption) error {
+	w.counter.patchCalls++
+	return w.SubResourceWriter.Patch(ctx, obj, patch, opts...)
+}
+
+func TestPatchDeploymentStatusUsesStatusPatch(t *testing.T) {
+	scheme := runtime.NewScheme()
+	if err := apps.AddToScheme(scheme); err != nil {
+		t.Fatalf("add apps/v1 scheme failed: %v", err)
+	}
+
+	baseDeployment := &apps.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "demo",
+			Namespace:  "default",
+			Generation: 5,
+		},
+		Status: apps.DeploymentStatus{
+			ObservedGeneration: 1,
+		},
+	}
+
+	baseClient := ctrlfake.NewClientBuilder().
+		WithScheme(scheme).
+		WithStatusSubresource(&apps.Deployment{}).
+		WithObjects(baseDeployment).
+		Build()
+	countingClient := &statusCountingClient{Client: baseClient}
+	dc := &DeploymentController{runtimeClient: countingClient}
+
+	current := &apps.Deployment{}
+	key := ctrlclient.ObjectKeyFromObject(baseDeployment)
+	if err := countingClient.Get(context.TODO(), key, current); err != nil {
+		t.Fatalf("get deployment failed: %v", err)
+	}
+	oldD := current.DeepCopy()
+	newD := current.DeepCopy()
+	newD.Status.ObservedGeneration = newD.Generation
+
+	if err := dc.patchDeploymentStatus(context.TODO(), oldD, newD); err != nil {
+		t.Fatalf("patch deployment status failed: %v", err)
+	}
+	if countingClient.counter.patchCalls != 1 {
+		t.Fatalf("expected 1 status patch call, got %d", countingClient.counter.patchCalls)
+	}
+	if countingClient.counter.updateCalls != 0 {
+		t.Fatalf("expected 0 status update calls, got %d", countingClient.counter.updateCalls)
+	}
+
+	result := &apps.Deployment{}
+	if err := countingClient.Get(context.TODO(), key, result); err != nil {
+		t.Fatalf("get patched deployment failed: %v", err)
+	}
+	if result.Status.ObservedGeneration != result.Generation {
+		t.Fatalf("expected observedGeneration=%d, got %d", result.Generation, result.Status.ObservedGeneration)
+	}
+}

pkg/controller/deployment/sync.go

@@ -133,6 +133,7 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 			}
 			return rsCopy, nil
 		}
+		oldD := d.DeepCopy()
 
 		// Should use the revision in existingNewRS's annotation, since it set by before
 		needsUpdate := deploymentutil.SetDeploymentRevision(d, rsCopy.Annotations[deploymentutil.RevisionAnnotation])
@@ -148,8 +149,9 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 		}
 
 		if needsUpdate {
-			var err error
-			if err = dc.runtimeClient.Status().Update(ctx, d); err != nil {
+			// Patch instead of Update: only send the status delta so unknown
+			// fields from newer API servers are not erased (version-skew safety).
+			if err := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); err != nil {
 				return nil, err
 			}
 		}
@@ -234,15 +236,17 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 		if d.Status.CollisionCount == nil {
 			d.Status.CollisionCount = new(int32)
 		}
+		oldD := d.DeepCopy()
+
 		preCollisionCount := *d.Status.CollisionCount
 		*d.Status.CollisionCount++
-		// Update the collisionCount for the Deployment and let it requeue by returning the original
-		// error.
-		dErr := dc.runtimeClient.Status().Update(ctx, d)
+		// Patch instead of Update: send only the collisionCount delta.
+		dBase := oldD.DeepCopy()
+		dErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(dBase))
 		if dErr == nil {
 			klog.V(2).Infof("Found a hash collision for deployment %q - bumping collisionCount (%d->%d) to resolve it", d.Name, preCollisionCount, *d.Status.CollisionCount)
 		} else {
-			klog.Errorf("Failed to update deployment collision count: %v", dErr)
+			klog.Errorf("Failed to patch deployment collision count: %v", dErr)
 		}
 		return nil, err
 	case errors.HasStatusCause(err, v1.NamespaceTerminatingCause):
@@ -251,13 +255,15 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 	case err != nil:
 		msg := fmt.Sprintf("Failed to create new replica set %q: %v", newRS.Name, err)
 		if deploymentutil.HasProgressDeadline(d) {
+			oldD := d.DeepCopy()
 			cond := deploymentutil.NewDeploymentCondition(apps.DeploymentProgressing, v1.ConditionFalse, deploymentutil.FailedRSCreateReason, msg)
+			// Patch instead of Update: only the new condition is sent
 			deploymentutil.SetDeploymentCondition(&d.Status, *cond)
 			// We don't really care about this error at this point, since we have a bigger issue to report.
 			// TODO: Identify which errors are permanent and switch DeploymentIsFailed to take into account
 			// these reasons as well. Related issue: https://github.com/kubernetes/kubernetes/issues/18568
-			if updateErr := dc.runtimeClient.Status().Update(ctx, d); updateErr != nil {
-				klog.Errorf("Failed to update deployment status after RS creation failure: %v", updateErr)
+			if updateErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); updateErr != nil {
+				klog.Errorf("Failed to patch deployment status after RS creation failure: %v", updateErr)
 			}
 		}
 		dc.eventRecorder.Eventf(d, v1.EventTypeWarning, deploymentutil.FailedRSCreateReason, msg)
@@ -266,7 +272,7 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 	if !alreadyExists && newReplicasCount > 0 {
 		dc.eventRecorder.Eventf(d, v1.EventTypeNormal, "ScalingReplicaSet", "Scaled up replica set %s to %d", createdRS.Name, newReplicasCount)
 	}
-
+	oldD := d.DeepCopy()
 	needsUpdate := deploymentutil.SetDeploymentRevision(d, newRevision)
 	if !alreadyExists && deploymentutil.HasProgressDeadline(d) {
 		msg := fmt.Sprintf("Created new replica set %q", createdRS.Name)
@@ -275,8 +281,9 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 		needsUpdate = true
 	}
 	if needsUpdate {
-		if updateErr := dc.runtimeClient.Status().Update(ctx, d); updateErr != nil {
-			klog.Errorf("Failed to update deployment status: %v", updateErr)
+		// Patch instead of Update: only the revision / Progressing condition delta is sent.
+		if updateErr := dc.runtimeClient.Status().Patch(ctx, d, client.MergeFrom(oldD)); updateErr != nil {
+			klog.Errorf("Failed to patch deployment status: %v", updateErr)
 			err = updateErr
 		}
 	}

pkg/util/workloads_utils.go

@@ -206,21 +206,30 @@ func UpdateFinalizer(c client.Client, object client.Object, op FinalizerOpType,
 			return getErr
 		}
 		finalizers := fetchedObject.GetFinalizers()
+
 		switch op {
 		case AddFinalizerOpType:
 			if controllerutil.ContainsFinalizer(fetchedObject, finalizer) {
 				return nil
 			}
 			finalizers = append(finalizers, finalizer)
+
 		case RemoveFinalizerOpType:
 			finalizerSet := sets.NewString(finalizers...)
 			if !finalizerSet.Has(finalizer) {
 				return nil
 			}
 			finalizers = finalizerSet.Delete(finalizer).List()
 		}
+
+		// Take a snapshot before mutating so only the finalizer delta is sent.
+		// Using Patch instead of Update prevents unknown/new API fields from being
+		// erased when Rollout is built against an older k8s version than the cluster.
+		baseObject := fetchedObject.DeepCopyObject().(client.Object)
+
 		fetchedObject.SetFinalizers(finalizers)
-		return c.Update(context.TODO(), fetchedObject)
+		patch := client.MergeFromWithOptions(baseObject, client.MergeFromWithOptimisticLock{})
+		return c.Patch(context.TODO(), fetchedObject, patch)
 	})
 }