fix: zero-value abortScaleDownDelay was not honored

Signed-off-by: Jesse Suen <[email protected]>

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
 }

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)

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")