Merge pull request #61981 from hanxiaoshuai/fixtodo0331

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

fixtodo:rsDeepCopy only when sizeNeedsUpdate or annotationsNeedUpdate

**What this PR does / why we need it**:
```
// TODO: Do not mutate the replica set here, instead simply compare the annotation and if they mismatch
// call SetReplicasAnnotations inside the following if clause. Then we can also move the deep-copy from
// above inside the if too.
```
fixtodo:rsDeepCopy only when sizeNeedsUpdate or annotationsNeedUpdate
**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

pkg/controller/deployment/sync.go

@@ -402,18 +402,17 @@ func (dc *DeploymentController) scaleReplicaSetAndRecordEvent(rs *extensions.Rep
 }
 
 func (dc *DeploymentController) scaleReplicaSet(rs *extensions.ReplicaSet, newScale int32, deployment *extensions.Deployment, scalingOperation string) (bool, *extensions.ReplicaSet, error) {
-	rsCopy := rs.DeepCopy()
 
-	sizeNeedsUpdate := *(rsCopy.Spec.Replicas) != newScale
+	sizeNeedsUpdate := *(rs.Spec.Replicas) != newScale
-	// TODO: Do not mutate the replica set here, instead simply compare the annotation and if they mismatch
+
-	// call SetReplicasAnnotations inside the following if clause. Then we can also move the deep-copy from
+	annotationsNeedUpdate := deploymentutil.ReplicasAnnotationsNeedUpdate(rs, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+deploymentutil.MaxSurge(*deployment))
-	// above inside the if too.
-	annotationsNeedUpdate := deploymentutil.SetReplicasAnnotations(rsCopy, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+deploymentutil.MaxSurge(*deployment))
 
 	scaled := false
 	var err error
 	if sizeNeedsUpdate || annotationsNeedUpdate {
+		rsCopy := rs.DeepCopy()
 		*(rsCopy.Spec.Replicas) = newScale
+		deploymentutil.SetReplicasAnnotations(rsCopy, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+deploymentutil.MaxSurge(*deployment))
 		rs, err = dc.client.ExtensionsV1beta1().ReplicaSets(rsCopy.Namespace).Update(rsCopy)
 		if err == nil && sizeNeedsUpdate {
 			scaled = true

pkg/controller/deployment/util/deployment_util.go

@@ -400,6 +400,22 @@ func SetReplicasAnnotations(rs *extensions.ReplicaSet, desiredReplicas, maxRepli
 	return updated
 }
 
+// AnnotationsNeedUpdate return true if ReplicasAnnotations need to be updated
+func ReplicasAnnotationsNeedUpdate(rs *extensions.ReplicaSet, desiredReplicas, maxReplicas int32) bool {
+	if rs.Annotations == nil {
+		return true
+	}
+	desiredString := fmt.Sprintf("%d", desiredReplicas)
+	if hasString := rs.Annotations[DesiredReplicasAnnotation]; hasString != desiredString {
+		return true
+	}
+	maxString := fmt.Sprintf("%d", maxReplicas)
+	if hasString := rs.Annotations[MaxReplicasAnnotation]; hasString != maxString {
+		return true
+	}
+	return false
+}
+
 // MaxUnavailable returns the maximum unavailable pods a rolling deployment can take.
 func MaxUnavailable(deployment extensions.Deployment) int32 {
 	if !IsRollingUpdate(&deployment) || *(deployment.Spec.Replicas) == 0 {

pkg/controller/deployment/util/deployment_util_test.go

@@ -1265,3 +1265,77 @@ func TestAnnotationUtils(t *testing.T) {
 	})
 	//Tear Down
 }
+
+func TestReplicasAnnotationsNeedUpdate(t *testing.T) {
+
+	desiredReplicas := fmt.Sprintf("%d", int32(10))
+	maxReplicas := fmt.Sprintf("%d", int32(20))
+
+	tests := []struct {
+		name       string
+		replicaSet *extensions.ReplicaSet
+		expected   bool
+	}{
+		{
+			name: "test Annotations nil",
+			replicaSet: &extensions.ReplicaSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "hello", Namespace: "test"},
+				Spec: extensions.ReplicaSetSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "test desiredReplicas update",
+			replicaSet: &extensions.ReplicaSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "hello",
+					Namespace:   "test",
+					Annotations: map[string]string{DesiredReplicasAnnotation: "8", MaxReplicasAnnotation: maxReplicas},
+				},
+				Spec: extensions.ReplicaSetSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "test maxReplicas update",
+			replicaSet: &extensions.ReplicaSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "hello",
+					Namespace:   "test",
+					Annotations: map[string]string{DesiredReplicasAnnotation: desiredReplicas, MaxReplicasAnnotation: "16"},
+				},
+				Spec: extensions.ReplicaSetSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "test needn't update",
+			replicaSet: &extensions.ReplicaSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:        "hello",
+					Namespace:   "test",
+					Annotations: map[string]string{DesiredReplicasAnnotation: desiredReplicas, MaxReplicasAnnotation: maxReplicas},
+				},
+				Spec: extensions.ReplicaSetSpec{
+					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
+				},
+			},
+			expected: false,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := ReplicasAnnotationsNeedUpdate(test.replicaSet, 10, 20)
+			if result != test.expected {
+				t.Errorf("case[%d]:%s Expected %v, Got: %v", i, test.name, test.expected, result)
+			}
+		})
+	}
+}