diff --git a/rollout/replicaset.go b/rollout/replicaset.go index 6adad2cf47..32e63a589e 100644 --- a/rollout/replicaset.go +++ b/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. diff --git a/rollout/replicaset_test.go b/rollout/replicaset_test.go index 83c7370c83..5bf7ac86ed 100644 --- a/rollout/replicaset_test.go +++ b/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{ diff --git a/rollout/sync.go b/rollout/sync.go index f8edddcd51..d541ece498 100644 --- a/rollout/sync.go +++ b/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 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/defaults/defaults.go b/utils/defaults/defaults.go index 3499927532..99e6520400 100644 --- a/utils/defaults/defaults.go +++ b/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 { diff --git a/utils/defaults/defaults_test.go b/utils/defaults/defaults_test.go index 4eef0eaf76..f76c3b097b 100644 --- a/utils/defaults/defaults_test.go +++ b/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) { diff --git a/utils/replicaset/canary.go b/utils/replicaset/canary.go index 1956a11152..e03b9bdeab 100644 --- a/utils/replicaset/canary.go +++ b/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 { diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go index ac4a548865..65df64f2a6 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") @@ -718,15 +759,15 @@ func TestGetCurrentSetWeight(t *testing.T) { rollout.Status.CurrentStepIndex = &stepIndex setWeight := GetCurrentSetWeight(rollout) - assert.Equal(t, setWeight, int32(100)) + assert.Equal(t, int32(100), setWeight) stepIndex = 0 setWeight = GetCurrentSetWeight(rollout) - assert.Equal(t, setWeight, int32(10)) + assert.Equal(t, int32(10), setWeight) rollout.Status.Abort = true setWeight = GetCurrentSetWeight(rollout) - assert.Equal(t, setWeight, int32(0)) + assert.Equal(t, int32(0), setWeight) }