fix: zero-value abortScaleDownDelay was not honored with setCanaryScale

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

rollout/replicaset.go

@@ -53,7 +53,7 @@ func (c *rolloutContext) addScaleDownDelay(rs *appsv1.ReplicaSet, scaleDownDelay
 		}
 		return nil
 	}
-	deadline := metav1.Now().Add(scaleDownDelaySeconds * time.Second).UTC().Format(time.RFC3339)
+	deadline := metav1.Now().Add(scaleDownDelaySeconds).UTC().Format(time.RFC3339)
 	patch := fmt.Sprintf(addScaleDownAtAnnotationsPatch, v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey, deadline)
 	_, err := c.kubeclientset.AppsV1().ReplicaSets(rs.Namespace).Patch(ctx, rs.Name, patchtypes.JSONPatchType, []byte(patch), metav1.PatchOptions{})
 	if err == nil {
@@ -94,7 +94,7 @@ func (c *Controller) getReplicaSetsForRollouts(r *v1alpha1.Rollout) ([]*appsv1.R
 // in the event that we moved back to an older revision that is still within its scaleDownDelay.
 func (c *rolloutContext) removeScaleDownDeadlines() error {
 	var toRemove []*appsv1.ReplicaSet
-	if c.newRS != nil && !c.isScaleDownOnabort() {
+	if c.newRS != nil && !c.shouldDelayScaleDownOnAbort() {
 		toRemove = append(toRemove, c.newRS)
 	}
 	if c.stableRS != nil {
@@ -120,8 +120,9 @@ func (c *rolloutContext) reconcileNewReplicaSet() (bool, error) {
 		return false, err
 	}
 
-	if c.isScaleDownOnabort() {
-		c.log.Infof("Scale down new rs '%s' on abort", c.newRS.Name)
+	if c.shouldDelayScaleDownOnAbort() {
+		abortScaleDownDelaySeconds := defaults.GetAbortScaleDownDelaySecondsOrDefault(c.rollout)
+		c.log.Infof("Scale down new rs '%s' on abort (%v)", c.newRS.Name, abortScaleDownDelaySeconds)
 
 		// if the newRS has scale down annotation, check if it should be scaled down now
 		if scaleDownAtStr, ok := c.newRS.Annotations[v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey]; ok {
@@ -144,9 +145,8 @@ func (c *rolloutContext) reconcileNewReplicaSet() (bool, error) {
 					newReplicasCount = int32(0)
 				}
 			}
-		} else {
-			abortScaleDownDelaySeconds := time.Duration(defaults.GetAbortScaleDownDelaySecondsOrDefault(c.rollout))
-			err = c.addScaleDownDelay(c.newRS, abortScaleDownDelaySeconds)
+		} else if abortScaleDownDelaySeconds != nil {
+			err = c.addScaleDownDelay(c.newRS, *abortScaleDownDelaySeconds)
 			if err != nil {
 				return false, err
 			}
@@ -157,12 +157,9 @@ func (c *rolloutContext) reconcileNewReplicaSet() (bool, error) {
 	return scaled, err
 }
 
-func (c *rolloutContext) isScaleDownOnabort() bool {
-	abortScaleDownDelaySeconds := time.Duration(defaults.GetAbortScaleDownDelaySecondsOrDefault(c.rollout))
-	if c.pauseContext != nil && c.pauseContext.IsAborted() && abortScaleDownDelaySeconds > 0 {
-		return true
-	}
-	return false
+// shouldDelayScaleDownOnAbort returns if we are aborted and we should delay scaledown of canary/preview
+func (c *rolloutContext) shouldDelayScaleDownOnAbort() bool {
+	return c.pauseContext.IsAborted() && defaults.GetAbortScaleDownDelaySecondsOrDefault(c.rollout) != nil
 }
 
 // reconcileOtherReplicaSets reconciles "other" ReplicaSets.

rollout/replicaset_test.go

@@ -202,12 +202,12 @@ func TestReconcileNewReplicaSet(t *testing.T) {
 					recorder:          record.NewFakeEventRecorder(),
 					resyncPeriod:      30 * time.Second,
 				},
+				pauseContext: &pauseContext{
+					rollout: rollout,
+				},
 			}
 			roCtx.enqueueRolloutAfter = func(obj interface{}, duration time.Duration) {}
 			if test.abortScaleDownDelaySeconds > 0 {
-				roCtx.pauseContext = &pauseContext{
-					rollout: rollout,
-				}
 				rollout.Status.Abort = true
 				// rollout.Spec.ScaleDownOnAbort = true
 				rollout.Spec.Strategy = v1alpha1.RolloutStrategy{

rollout/sync.go

@@ -390,7 +390,7 @@ func (c *rolloutContext) scaleReplicaSet(rs *appsv1.ReplicaSet, newScale int32,
 		oldScale := defaults.GetReplicasOrDefault(rs.Spec.Replicas)
 		*(rsCopy.Spec.Replicas) = newScale
 		annotations.SetReplicasAnnotations(rsCopy, rolloutReplicas)
-		if fullScaleDown && !c.isScaleDownOnabort() {
+		if fullScaleDown && !c.shouldDelayScaleDownOnAbort() {
 			delete(rsCopy.Annotations, v1alpha1.DefaultReplicaSetScaleDownDeadlineAnnotationKey)
 		}
 		rs, err = c.kubeclientset.AppsV1().ReplicaSets(rsCopy.Namespace).Update(ctx, rsCopy, metav1.UpdateOptions{})
@@ -927,7 +927,7 @@ func (c *rolloutContext) promoteStable(newStatus *v1alpha1.RolloutStatus, reason
 		// Now that we've marked the desired RS as stable, start the scale-down countdown on the previous stable RS
 		previousStableRS, _ := replicasetutil.GetReplicaSetByTemplateHash(c.olderRSs, previousStableHash)
 		if replicasetutil.GetReplicaCountForReplicaSets([]*appsv1.ReplicaSet{previousStableRS}) > 0 {
-			scaleDownDelaySeconds := time.Duration(defaults.GetScaleDownDelaySecondsOrDefault(c.rollout))
+			scaleDownDelaySeconds := defaults.GetScaleDownDelaySecondsOrDefault(c.rollout)
 			err := c.addScaleDownDelay(previousStableRS, scaleDownDelaySeconds)
 			if err != nil {
 				return err

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/defaults/defaults.go

@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
+	"time"
 
 	"k8s.io/apimachinery/pkg/util/intstr"
 
@@ -102,40 +103,52 @@ func GetExperimentScaleDownDelaySecondsOrDefault(e *v1alpha1.Experiment) int32 {
 	return DefaultScaleDownDelaySeconds
 }
 
-func GetScaleDownDelaySecondsOrDefault(rollout *v1alpha1.Rollout) int32 {
+func GetScaleDownDelaySecondsOrDefault(rollout *v1alpha1.Rollout) time.Duration {
+	var delaySeconds int32
 	if rollout.Spec.Strategy.BlueGreen != nil {
+		delaySeconds = DefaultAbortScaleDownDelaySeconds
 		if rollout.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds != nil {
-			return *rollout.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds
+			delaySeconds = *rollout.Spec.Strategy.BlueGreen.ScaleDownDelaySeconds
 		}
-		return DefaultScaleDownDelaySeconds
 	}
 	if rollout.Spec.Strategy.Canary != nil {
 		if rollout.Spec.Strategy.Canary.TrafficRouting != nil {
+			delaySeconds = DefaultAbortScaleDownDelaySeconds
 			if rollout.Spec.Strategy.Canary.ScaleDownDelaySeconds != nil {
-				return *rollout.Spec.Strategy.Canary.ScaleDownDelaySeconds
+				delaySeconds = *rollout.Spec.Strategy.Canary.ScaleDownDelaySeconds
 			}
-			return DefaultScaleDownDelaySeconds
 		}
 	}
-	return 0
+	return time.Duration(delaySeconds) * time.Second
 }
 
-func GetAbortScaleDownDelaySecondsOrDefault(rollout *v1alpha1.Rollout) int32 {
+// GetAbortScaleDownDelaySecondsOrDefault returns the duration seconds to delay the scale down of
+// the canary/preview ReplicaSet in a abort situation. A nil value indicates it should not
+// scale down at all (abortScaleDownDelaySeconds: 0). A value of 0 indicates it should scale down
+// immediately.
+func GetAbortScaleDownDelaySecondsOrDefault(rollout *v1alpha1.Rollout) *time.Duration {
+	var delaySeconds int32
 	if rollout.Spec.Strategy.BlueGreen != nil {
+		delaySeconds = DefaultAbortScaleDownDelaySeconds
 		if rollout.Spec.Strategy.BlueGreen.AbortScaleDownDelaySeconds != nil {
-			return *rollout.Spec.Strategy.BlueGreen.AbortScaleDownDelaySeconds
+			if *rollout.Spec.Strategy.BlueGreen.AbortScaleDownDelaySeconds == 0 {
+				return nil
+			}
+			delaySeconds = *rollout.Spec.Strategy.BlueGreen.AbortScaleDownDelaySeconds
 		}
-		return DefaultAbortScaleDownDelaySeconds
-	}
-	if rollout.Spec.Strategy.Canary != nil {
+	} else if rollout.Spec.Strategy.Canary != nil {
 		if rollout.Spec.Strategy.Canary.TrafficRouting != nil {
+			delaySeconds = DefaultAbortScaleDownDelaySeconds
 			if rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds != nil {
-				return *rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds
+				if *rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds == 0 {
+					return nil
+				}
+				delaySeconds = *rollout.Spec.Strategy.Canary.AbortScaleDownDelaySeconds
 			}
-			return DefaultAbortScaleDownDelaySeconds
 		}
 	}
-	return 0
+	dur := time.Duration(delaySeconds) * time.Second
+	return &dur
 }
 
 func GetAutoPromotionEnabledOrDefault(rollout *v1alpha1.Rollout) bool {

utils/defaults/defaults_test.go

@@ -2,6 +2,7 @@ package defaults
 
 import (
 	"testing"
+	"time"
 
 	"k8s.io/utils/pointer"
 
@@ -136,11 +137,11 @@ func TestGetScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, scaleDownDelaySeconds, GetScaleDownDelaySecondsOrDefault(blueGreenNonDefaultValue))
+		assert.Equal(t, time.Duration(scaleDownDelaySeconds)*time.Second, GetScaleDownDelaySecondsOrDefault(blueGreenNonDefaultValue))
 	}
 	{
 		rolloutNoStrategyDefaultValue := &v1alpha1.Rollout{}
-		assert.Equal(t, int32(0), GetScaleDownDelaySecondsOrDefault(rolloutNoStrategyDefaultValue))
+		assert.Equal(t, time.Duration(0), GetScaleDownDelaySecondsOrDefault(rolloutNoStrategyDefaultValue))
 	}
 	{
 		rolloutNoScaleDownDelaySeconds := &v1alpha1.Rollout{
@@ -150,7 +151,7 @@ func TestGetScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, DefaultScaleDownDelaySeconds, GetScaleDownDelaySecondsOrDefault(rolloutNoScaleDownDelaySeconds))
+		assert.Equal(t, time.Duration(DefaultScaleDownDelaySeconds)*time.Second, GetScaleDownDelaySecondsOrDefault(rolloutNoScaleDownDelaySeconds))
 	}
 	{
 		scaleDownDelaySeconds := int32(60)
@@ -163,7 +164,7 @@ func TestGetScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, int32(0), GetScaleDownDelaySecondsOrDefault(canaryNoTrafficRouting))
+		assert.Equal(t, time.Duration(0), GetScaleDownDelaySecondsOrDefault(canaryNoTrafficRouting))
 	}
 	{
 		scaleDownDelaySeconds := int32(60)
@@ -177,7 +178,7 @@ func TestGetScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, scaleDownDelaySeconds, GetScaleDownDelaySecondsOrDefault(canaryWithTrafficRouting))
+		assert.Equal(t, time.Duration(scaleDownDelaySeconds)*time.Second, GetScaleDownDelaySecondsOrDefault(canaryWithTrafficRouting))
 	}
 }
 
@@ -193,7 +194,21 @@ func TestGetAbortScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, abortScaleDownDelaySeconds, GetAbortScaleDownDelaySecondsOrDefault(blueGreenNonDefaultValue))
+		assert.Equal(t, time.Duration(abortScaleDownDelaySeconds)*time.Second, *GetAbortScaleDownDelaySecondsOrDefault(blueGreenNonDefaultValue))
+	}
+	{
+		// dont scale down preview
+		abortScaleDownDelaySeconds := int32(0)
+		blueGreenZeroValue := &v1alpha1.Rollout{
+			Spec: v1alpha1.RolloutSpec{
+				Strategy: v1alpha1.RolloutStrategy{
+					BlueGreen: &v1alpha1.BlueGreenStrategy{
+						AbortScaleDownDelaySeconds: &abortScaleDownDelaySeconds,
+					},
+				},
+			},
+		}
+		assert.Nil(t, GetAbortScaleDownDelaySecondsOrDefault(blueGreenZeroValue))
 	}
 	{
 		blueGreenDefaultValue := &v1alpha1.Rollout{
@@ -203,7 +218,7 @@ func TestGetAbortScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, DefaultAbortScaleDownDelaySeconds, GetAbortScaleDownDelaySecondsOrDefault(blueGreenDefaultValue))
+		assert.Equal(t, time.Duration(DefaultAbortScaleDownDelaySeconds)*time.Second, *GetAbortScaleDownDelaySecondsOrDefault(blueGreenDefaultValue))
 	}
 	{
 		abortScaleDownDelaySeconds := int32(60)
@@ -217,11 +232,26 @@ func TestGetAbortScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, abortScaleDownDelaySeconds, GetAbortScaleDownDelaySecondsOrDefault(canaryNonDefaultValue))
+		assert.Equal(t, time.Duration(abortScaleDownDelaySeconds)*time.Second, *GetAbortScaleDownDelaySecondsOrDefault(canaryNonDefaultValue))
+	}
+	{
+		// dont scale down canary
+		abortScaleDownDelaySeconds := int32(0)
+		canaryZeroValue := &v1alpha1.Rollout{
+			Spec: v1alpha1.RolloutSpec{
+				Strategy: v1alpha1.RolloutStrategy{
+					Canary: &v1alpha1.CanaryStrategy{
+						AbortScaleDownDelaySeconds: &abortScaleDownDelaySeconds,
+						TrafficRouting:             &v1alpha1.RolloutTrafficRouting{},
+					},
+				},
+			},
+		}
+		assert.Nil(t, GetAbortScaleDownDelaySecondsOrDefault(canaryZeroValue))
 	}
 	{
 		rolloutNoStrategyDefaultValue := &v1alpha1.Rollout{}
-		assert.Equal(t, int32(0), GetAbortScaleDownDelaySecondsOrDefault(rolloutNoStrategyDefaultValue))
+		assert.Equal(t, time.Duration(0), *GetAbortScaleDownDelaySecondsOrDefault(rolloutNoStrategyDefaultValue))
 	}
 	{
 		canaryDefaultValue := &v1alpha1.Rollout{
@@ -233,8 +263,20 @@ func TestGetAbortScaleDownDelaySecondsOrDefault(t *testing.T) {
 				},
 			},
 		}
-		assert.Equal(t, DefaultAbortScaleDownDelaySeconds, GetAbortScaleDownDelaySecondsOrDefault(canaryDefaultValue))
+		assert.Equal(t, time.Duration(DefaultAbortScaleDownDelaySeconds)*time.Second, *GetAbortScaleDownDelaySecondsOrDefault(canaryDefaultValue))
 	}
+	{
+		// basic canary should not have scaledown delay seconds
+		canaryDefaultValue := &v1alpha1.Rollout{
+			Spec: v1alpha1.RolloutSpec{
+				Strategy: v1alpha1.RolloutStrategy{
+					Canary: &v1alpha1.CanaryStrategy{},
+				},
+			},
+		}
+		assert.Equal(t, time.Duration(0), *GetAbortScaleDownDelaySecondsOrDefault(canaryDefaultValue))
+	}
+
 }
 
 func TestGetAutoPromotionEnabledOrDefault(t *testing.T) {

utils/replicaset/canary.go

@@ -331,19 +331,20 @@ 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.Spec.Strategy.Canary == nil || rollout.Spec.Strategy.Canary.TrafficRouting == nil {
+		// SetCanaryScale only works with TrafficRouting
 		return nil
 	}
-	currentStep, currentStepIndex := GetCurrentCanaryStep(rollout)
-	if currentStep == nil {
+	if rollout.Status.Abort && defaults.GetAbortScaleDownDelaySecondsOrDefault(rollout) != nil {
+		// If rollout is aborted do not use the set canary scale, *unless* the user explicitly
+		// indicated to leave the canary scaled up (abortScaleDownDelaySeconds: 0).
 		return nil
 	}
-	// SetCanaryScale only works with TrafficRouting
-	if rollout.Spec.Strategy.Canary == nil || rollout.Spec.Strategy.Canary.TrafficRouting == nil {
+	currentStep, currentStepIndex := GetCurrentCanaryStep(rollout)
+	if currentStep == nil {
+		// setCanaryScale feature is unused
 		return nil
 	}
-
 	for i := *currentStepIndex; i >= 0; i-- {
 		step := rollout.Spec.Strategy.Canary.Steps[i]
 		if step.SetCanaryScale == nil {