Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: zero-value abortScaleDownDelay was not honored with setCanaryScale #1375

Merged
merged 1 commit into from
Jul 29, 2021
Merged

fix: zero-value abortScaleDownDelay was not honored with setCanaryScale #1375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions rollout/replicaset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed confusing method behavior where duration was expected to be supplied in nanoseconds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All callers of this method were supplying scaleDownDelaySeconds in nanosecond duration. I also changed the callers to simply supply proper durations.

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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this condition check required as it is verified in shouldDelayScaleDownOnAbort

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that it's not required. But I was trying to be defensive here in case there was a bug or change in behavior to shouldDelayScaleDownOnAbort().

err = c.addScaleDownDelay(c.newRS, *abortScaleDownDelaySeconds)
if err != nil {
return false, err
}
Expand All @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions rollout/replicaset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions rollout/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions test/e2e/istio_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
41 changes: 27 additions & 14 deletions utils/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"io/ioutil"
"os"
"strings"
"time"

"k8s.io/apimachinery/pkg/util/intstr"

Expand Down Expand Up @@ -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 {
Expand Down
62 changes: 52 additions & 10 deletions utils/defaults/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package defaults

import (
"testing"
"time"

"k8s.io/utils/pointer"

Expand Down Expand Up @@ -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{
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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))
}
}

Expand All @@ -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{
Expand All @@ -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)
Expand All @@ -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{
Expand All @@ -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) {
Expand Down
15 changes: 8 additions & 7 deletions utils/replicaset/canary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading