From 1fa06a6bd43e91b679e81202160190c0e7c7881b Mon Sep 17 00:00:00 2001 From: hangaoshuai Date: Mon, 2 Apr 2018 09:27:11 +0800 Subject: [PATCH 1/2] fixtodo:rsDeepCopy only when sizeNeedsUpdate or annotationsNeedUpdate --- pkg/controller/deployment/sync.go | 11 +++++------ .../deployment/util/deployment_util.go | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 99f6552e442..42891c1b340 100644 --- a/pkg/controller/deployment/sync.go +++ b/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 - // 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. - annotationsNeedUpdate := deploymentutil.SetReplicasAnnotations(rsCopy, *(deployment.Spec.Replicas), *(deployment.Spec.Replicas)+deploymentutil.MaxSurge(*deployment)) + sizeNeedsUpdate := *(rs.Spec.Replicas) != newScale + + annotationsNeedUpdate := deploymentutil.ReplicasAnnotationsNeedUpdate(rs, *(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 diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index ef8e487e4dc..88775035f96 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/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 { From 808c39387fbb2fa5f1e05c55846ebac8533c283e Mon Sep 17 00:00:00 2001 From: hangaoshuai Date: Mon, 2 Apr 2018 09:27:43 +0800 Subject: [PATCH 2/2] add unit test for new function AnnotationsNeedUpdate --- .../deployment/util/deployment_util_test.go | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index c8ce876df55..a02dbeddc75 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/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) + } + }) + } +}