combine patchExtraStatus and syncDeploymentStatus

Signed-off-by: zhihao jian <zhihao.jian@shopee.com>

pkg/controller/deployment/controller.go

@@ -186,10 +186,6 @@ func (r *ReconcileDeployment) Reconcile(_ context.Context, request reconcile.Req
 	if err != nil {
 		errList = append(errList, field.InternalError(field.NewPath("syncDeployment"), err))
 	}
-	err = dc.patchExtraStatus(deployment)
-	if err != nil {
-		errList = append(errList, field.InternalError(field.NewPath("patchExtraStatus"), err))
-	}
 	if len(errList) > 0 {
 		return ctrl.Result{}, errList.ToAggregate()
 	}

pkg/controller/deployment/deployment_controller.go

@@ -22,7 +22,6 @@ package deployment
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"reflect"
 	"time"
@@ -35,7 +34,6 @@ import (
 	"k8s.io/klog/v2"
 
 	rolloutsv1alpha1 "github.com/openkruise/rollouts/api/v1alpha1"
-	deploymentutil "github.com/openkruise/rollouts/pkg/controller/deployment/util"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -146,49 +144,3 @@ func (dc *DeploymentController) syncDeployment(ctx context.Context, deployment *
 
 	return dc.rolloutRolling(ctx, d, rsList)
 }
-
-// patchExtraStatus will update extra status for advancedStatus
-func (dc *DeploymentController) patchExtraStatus(deployment *apps.Deployment) error {
-	latestDeployment := &apps.Deployment{}
-	err := dc.runtimeClient.Get(context.TODO(), client.ObjectKeyFromObject(deployment), latestDeployment)
-	if err != nil {
-		klog.Errorf("Failed to get deployment: %v", err)
-		return err
-	}
-	rsList, err := dc.getReplicaSetsForDeployment(context.TODO(), latestDeployment)
-	if err != nil {
-		return err
-	}
-
-	updatedReadyReplicas := int32(0)
-	newRS := deploymentutil.FindNewReplicaSet(latestDeployment, rsList)
-	if newRS != nil {
-		updatedReadyReplicas = newRS.Status.ReadyReplicas
-	}
-
-	extraStatus := &rolloutsv1alpha1.DeploymentExtraStatus{
-		UpdatedReadyReplicas:    updatedReadyReplicas,
-		ExpectedUpdatedReplicas: deploymentutil.NewRSReplicasLimit(dc.strategy.Partition, latestDeployment),
-	}
-
-	extraStatusByte, err := json.Marshal(extraStatus)
-	if err != nil {
-		klog.Errorf("Failed to marshal extra status for Deployment %v, err: %v", klog.KObj(latestDeployment), err)
-		return nil // no need to retry
-	}
-
-	extraStatusAnno := string(extraStatusByte)
-	if latestDeployment.Annotations[rolloutsv1alpha1.DeploymentExtraStatusAnnotation] == extraStatusAnno {
-		return nil // no need to update
-	}
-	deploymentCopy := latestDeployment.DeepCopy()
-	deploymentCopy.Annotations[rolloutsv1alpha1.DeploymentExtraStatusAnnotation] = extraStatusAnno
-
-	patch := client.MergeFromWithOptions(latestDeployment, client.MergeFromWithOptimisticLock{})
-	err = dc.runtimeClient.Patch(context.TODO(), deploymentCopy, patch)
-	if err != nil {
-		klog.Errorf("Failed to patch deployment extra status: %v", err)
-		return err
-	}
-	return err
-}

pkg/controller/deployment/progress.go

@@ -20,7 +20,6 @@ package deployment
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"time"
 
 	apps "k8s.io/api/apps/v1"
@@ -105,20 +104,21 @@ func (dc *DeploymentController) syncRolloutStatus(ctx context.Context, allRSs []
 		util.RemoveDeploymentCondition(&newStatus, apps.DeploymentReplicaFailure)
 	}
 
-	// Do not update if there is nothing new to add.
-	if reflect.DeepEqual(d.Status, newStatus) {
-		// Requeue the deployment if required.
-		dc.requeueStuckDeployment(d, newStatus)
-		return nil
+	// Calculate extra status annotation
+	extraStatusAnno, err := dc.updateDeploymentExtraStatus(ctx, newRS, d)
+	if err != nil {
+		return nil // no need to retry
 	}
 
-	newDeployment := d
-	newDeployment.Status = newStatus
-	err := dc.runtimeClient.Status().Update(ctx, newDeployment)
+	// Update both status and annotation
+	err = dc.patchDeploymentStatusAndAnnotation(ctx, d, newStatus, extraStatusAnno)
 	if err != nil {
-		klog.Errorf("Failed to update deployment status in progress: %v", err)
+		return err
 	}
-	return err
+
+	// Requeue the deployment if required.
+	dc.requeueStuckDeployment(d, newStatus)
+	return nil
 }
 
 // getReplicaFailures will convert replica failure conditions from replica sets

pkg/controller/deployment/sync.go

@@ -19,6 +19,7 @@ package deployment
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"sort"
@@ -497,20 +498,80 @@ func (dc *DeploymentController) cleanupDeployment(ctx context.Context, oldRSs []
 }
 
 // syncDeploymentStatus checks if the status is up-to-date and sync it if necessary
+// It also updates the extra status annotation for advanced deployment
 func (dc *DeploymentController) syncDeploymentStatus(ctx context.Context, allRSs []*apps.ReplicaSet, newRS *apps.ReplicaSet, d *apps.Deployment) error {
 	newStatus := calculateStatus(allRSs, newRS, d, &dc.strategy)
 
-	if reflect.DeepEqual(d.Status, newStatus) {
+	// Calculate extra status annotation
+	extraStatusAnno, err := dc.updateDeploymentExtraStatus(ctx, newRS, d)
+	if err != nil {
+		return nil // no need to retry
+	}
+
+	// Update both status and annotation
+	return dc.patchDeploymentStatusAndAnnotation(ctx, d, newStatus, extraStatusAnno)
+}
+
+// updateDeploymentExtraStatus updates the extra status annotation for advanced deployment
+func (dc *DeploymentController) updateDeploymentExtraStatus(ctx context.Context, newRS *apps.ReplicaSet, d *apps.Deployment) (string, error) {
+	// For consistency with original logic, we can optionally get the latest deployment
+	// if the passed deployment might be stale. However, in most cases, the passed data
+	// should be fresh enough since it comes from the current reconciliation cycle.
+
+	updatedReadyReplicas := int32(0)
+	if newRS != nil {
+		updatedReadyReplicas = newRS.Status.ReadyReplicas
+	}
+
+	extraStatus := &rolloutsv1alpha1.DeploymentExtraStatus{
+		UpdatedReadyReplicas:    updatedReadyReplicas,
+		ExpectedUpdatedReplicas: deploymentutil.NewRSReplicasLimit(dc.strategy.Partition, d),
+	}
+
+	extraStatusByte, err := json.Marshal(extraStatus)
+	if err != nil {
+		klog.Errorf("Failed to marshal extra status for Deployment %v, err: %v", klog.KObj(d), err)
+		return "", err
+	}
+
+	return string(extraStatusByte), nil
+}
+
+// patchDeploymentStatusAndAnnotation updates both status and annotation in one operation
+func (dc *DeploymentController) patchDeploymentStatusAndAnnotation(ctx context.Context, d *apps.Deployment, newStatus apps.DeploymentStatus, extraStatusAnno string) error {
+	statusNeedsUpdate := !reflect.DeepEqual(d.Status, newStatus)
+	annotationNeedsUpdate := d.Annotations[rolloutsv1alpha1.DeploymentExtraStatusAnnotation] != extraStatusAnno
+
+	// If neither status nor annotation needs update, return early
+	if !statusNeedsUpdate && !annotationNeedsUpdate {
 		return nil
 	}
 
-	newDeployment := d
-	newDeployment.Status = newStatus
-	err := dc.runtimeClient.Status().Update(ctx, newDeployment)
-	if err != nil {
-		klog.Errorf("Failed to sync deployment status: %v", err)
+	// Create a copy for updating both status and annotation
+	deploymentCopy := d.DeepCopy()
+
+	// Update status if needed
+	if statusNeedsUpdate {
+		deploymentCopy.Status = newStatus
 	}
-	return err
+
+	// Update annotation if needed
+	if annotationNeedsUpdate {
+		if deploymentCopy.Annotations == nil {
+			deploymentCopy.Annotations = make(map[string]string)
+		}
+		deploymentCopy.Annotations[rolloutsv1alpha1.DeploymentExtraStatusAnnotation] = extraStatusAnno
+	}
+
+	// Use Strategic Merge Patch to update both status and annotation in one operation
+	patch := client.MergeFrom(d)
+	err := dc.runtimeClient.Patch(ctx, deploymentCopy, patch)
+	if err != nil {
+		klog.Errorf("Failed to patch deployment status and annotation: %v", err)
+		return err
+	}
+
+	return nil
 }
 
 // calculateStatus calculates the latest status for the provided deployment by looking into the provided replica sets.