From 7c66320d7192f3020af5c54c8ebe62b7edb84e94 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Tue, 27 Jul 2021 17:17:05 -0700 Subject: [PATCH] fix: zero-value abortScaleDownDelay was not honored Signed-off-by: Jesse Suen --- test/e2e/istio_test.go | 5 ++-- utils/replicaset/canary.go | 8 +++---- utils/replicaset/canary_test.go | 41 +++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/test/e2e/istio_test.go b/test/e2e/istio_test.go index 6316d67c29..8d63093fed 100644 --- a/test/e2e/istio_test.go +++ b/test/e2e/istio_test.go @@ -250,6 +250,7 @@ func (s *IstioSuite) TestIstioAbortUpdateDeleteAllCanaryPods() { AbortRollout(). WaitForRolloutStatus("Degraded"). Then(). - ExpectRevisionPodCount("2", 0). - ExpectRevisionPodCount("1", 5) + ExpectRevisionPodCount("1", 5). + ExpectRevisionPodCount("2", 4). // canary pods remained scaled + ExpectRevisionScaleDown("2", true) // but have a scale down delay } diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go index 1956a11152..378665f50e 100644 --- a/utils/replicaset/canary.go +++ b/utils/replicaset/canary.go @@ -310,9 +310,6 @@ func GetCanaryReplicasOrWeight(rollout *v1alpha1.Rollout) (*int32, int32) { // until it finds a setWeight step. The controller defaults to 100 if it iterates through all the steps with no // setWeight or if there is no current step (i.e. the controller has already stepped through all the steps). func GetCurrentSetWeight(rollout *v1alpha1.Rollout) int32 { - if rollout.Status.Abort { - return 0 - } currentStep, currentStepIndex := GetCurrentCanaryStep(rollout) if currentStep == nil { return 100 @@ -331,8 +328,9 @@ func GetCurrentSetWeight(rollout *v1alpha1.Rollout) int32 { // TrafficRouting is required to be set for SetCanaryScale to be applicable. // If MatchTrafficWeight is set after a previous SetCanaryScale step, it will likewise be ignored. func UseSetCanaryScale(rollout *v1alpha1.Rollout) *v1alpha1.SetCanaryScale { - // Return nil when rollout is aborted - if rollout.Status.Abort { + if rollout.Status.Abort && defaults.GetAbortScaleDownDelaySecondsOrDefault(rollout) != 0 { + // If rollout is aborted, we should not use the set canary scale, *unless* the user + // explicitly indicated to leave the canary scaled up (abortScaleDownDelaySeconds: 0). return nil } currentStep, currentStepIndex := GetCurrentCanaryStep(rollout) diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go index ac4a548865..679894faf1 100644 --- a/utils/replicaset/canary_test.go +++ b/utils/replicaset/canary_test.go @@ -93,6 +93,9 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) { olderRS *appsv1.ReplicaSet setCanaryScale *v1alpha1.SetCanaryScale trafficRouting *v1alpha1.RolloutTrafficRouting + + abortScaleDownDelaySeconds *int32 + statusAbort bool }{ { name: "Do not add extra RSs in scaleDownCount when .Spec.Replica < AvailableReplicas", @@ -571,14 +574,52 @@ func TestCalculateReplicaCountsForCanary(t *testing.T) { expectedStableReplicaCount: 4, expectedCanaryReplicaCount: 3, }, + { + // verify when we are aborted, and have abortScaleDownDelaySeconds: 0, and use setCanaryScale, we dont scale down canary + name: "aborted with abortScaleDownDelaySeconds:0 and setCanaryScale", + rolloutSpecReplicas: 1, + setCanaryScale: newSetCanaryScale(intPnt(1), nil, false), + trafficRouting: &v1alpha1.RolloutTrafficRouting{}, + abortScaleDownDelaySeconds: intPnt(0), + statusAbort: true, + + stableSpecReplica: 1, + stableAvailableReplica: 1, + + canarySpecReplica: 1, + canaryAvailableReplica: 1, + + expectedStableReplicaCount: 1, + expectedCanaryReplicaCount: 1, + }, + { + // verify when we are aborted, and have abortScaleDownDelaySeconds>0, and use setCanaryScale, we scale down canary + name: "aborted with abortScaleDownDelaySeconds>0 and setCanaryScale", + rolloutSpecReplicas: 1, + setCanaryScale: newSetCanaryScale(intPnt(1), nil, false), + trafficRouting: &v1alpha1.RolloutTrafficRouting{}, + abortScaleDownDelaySeconds: intPnt(30), + statusAbort: true, + + stableSpecReplica: 1, + stableAvailableReplica: 1, + + canarySpecReplica: 1, + canaryAvailableReplica: 1, + + expectedStableReplicaCount: 1, + expectedCanaryReplicaCount: 0, + }, } for i := range tests { test := tests[i] t.Run(test.name, func(t *testing.T) { rollout := newRollout(test.rolloutSpecReplicas, test.setWeight, test.maxSurge, test.maxUnavailable, "canary", "stable", test.setCanaryScale, test.trafficRouting) rollout.Status.PromoteFull = test.promoteFull + rollout.Status.Abort = test.statusAbort stableRS := newRS("stable", test.stableSpecReplica, test.stableAvailableReplica) canaryRS := newRS("canary", test.canarySpecReplica, test.canaryAvailableReplica) + rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds = test.abortScaleDownDelaySeconds newRSReplicaCount, stableRSReplicaCount := CalculateReplicaCountsForCanary(rollout, canaryRS, stableRS, []*appsv1.ReplicaSet{test.olderRS}) assert.Equal(t, test.expectedCanaryReplicaCount, newRSReplicaCount, "check canary replica count") assert.Equal(t, test.expectedStableReplicaCount, stableRSReplicaCount, "check stable replica count")