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

fix: Set Canary Strategy default maxUnavailable to 25% #981

Merged
merged 3 commits into from
Feb 10, 2021
Merged

fix: Set Canary Strategy default maxUnavailable to 25% #981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e42c0e
fix: Set Canary Strategy default maxUnavailable to 25%
khhirani Feb 5, 2021
0d0ee20
Undo error
khhirani Feb 5, 2021
cd77e18
Update docs
khhirani Feb 9, 2021
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
2 changes: 1 addition & 1 deletion docs/features/canary.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ Defaults to "25%".
### maxUnavailable
The maximum number of pods that can be unavailable during the update. Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). This can not be 0 if MaxSurge is 0.

Defaults to 0
Defaults to 25%

### trafficRouting
The [traffic management](traffic-management/index.md) rules to apply to control the flow of traffic between the active and canary versions. If not set, the default weighted pod replica based routing will be used.
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/rollouts/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ type CanaryStrategy struct {
// Value can be an absolute number (ex: 5) or a percentage of total pods at the start of update (ex: 10%).
// Absolute number is calculated from percentage by rounding down.
// This can not be 0 if MaxSurge is 0.
// By default, a fixed value of 1 is used.
// By default, a fixed value of 25% is used.
// Example: when this is set to 30%, the old RC can be scaled down by 30%
// immediately when the rolling update starts. Once new pods are ready, old RC
// can be scaled down further, followed by scaling up the new RC, ensuring
Expand All @@ -190,7 +190,7 @@ type CanaryStrategy struct {
// Value can be an absolute number (ex: 5) or a percentage of total pods at
// the start of the update (ex: 10%). This can not be 0 if MaxUnavailable is 0.
// Absolute number is calculated from percentage by rounding up.
// By default, a value of 1 is used.
// By default, a value of 25% is used.
// Example: when this is set to 30%, the new RC can be scaled up by 30%
// immediately when the rolling update starts. Once old pods have been killed,
// new RC can be scaled up further, ensuring that total number of pods running
Expand Down
4 changes: 2 additions & 2 deletions utils/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const (
// DefaultMaxSurge default number for the max number of additional pods that can be brought up during a rollout
DefaultMaxSurge = "25"
// DefaultMaxUnavailable default number for the max number of unavailable pods during a rollout
DefaultMaxUnavailable = 0
DefaultMaxUnavailable = "25"
Comment on lines 15 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this is 25 and not 25%. Can you verify that we aren't defaulting max unavailable to 25 pods instead of 25% of pods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code which resolves both MaxSurge and MaxUnavailable uses the method intstrutil.GetValueFromIntOrPercent. I confirmed that it works with some manual tests.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for confirming. I see that GetValueFromIntOrPercent interprets all string values as percentages and not integer values, even if the value is missing a % character:

func GetValueFromIntOrPercent(intOrPercent *IntOrString, total int, roundUp bool) (int, error) {
	if intOrPercent == nil {
		return 0, errors.New("nil value for IntOrString")
	}
	value, isPercent, err := getIntOrPercentValue(intOrPercent)
	if err != nil {
		return 0, fmt.Errorf("invalid value for IntOrString: %v", err)
	}
	if isPercent {
		if roundUp {
			value = int(math.Ceil(float64(value) * (float64(total)) / 100))
		} else {
			value = int(math.Floor(float64(value) * (float64(total)) / 100))
		}
	}
	return value, nil
}

func getIntOrPercentValue(intOrStr *IntOrString) (int, bool, error) {
	switch intOrStr.Type {
	case Int:
		return intOrStr.IntValue(), false, nil
	case String:
		s := strings.Replace(intOrStr.StrVal, "%", "", -1)
		v, err := strconv.Atoi(s)
		if err != nil {
			return 0, false, fmt.Errorf("invalid value %q: %v", intOrStr.StrVal, err)
		}
		return int(v), true, nil
	}
	return 0, false, fmt.Errorf("invalid type: neither int nor percentage")
}

// DefaultProgressDeadlineSeconds default number of seconds for the rollout to be making progress
DefaultProgressDeadlineSeconds = int32(600)
// DefaultScaleDownDelaySeconds default seconds before scaling down old replicaset after switching services
Expand Down Expand Up @@ -57,7 +57,7 @@ func GetMaxUnavailableOrDefault(rollout *v1alpha1.Rollout) *intstr.IntOrString {
if rollout.Spec.Strategy.Canary != nil && rollout.Spec.Strategy.Canary.MaxUnavailable != nil {
return rollout.Spec.Strategy.Canary.MaxUnavailable
}
defaultValue := intstr.FromInt(DefaultMaxUnavailable)
defaultValue := intstr.FromString(DefaultMaxUnavailable)
return &defaultValue
}

Expand Down
2 changes: 1 addition & 1 deletion utils/defaults/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestGetMaxUnavailableOrDefault(t *testing.T) {
assert.Equal(t, maxUnavailable, *GetMaxUnavailableOrDefault(rolloutBlueGreenNonDefaultValue))

rolloutDefaultValue := &v1alpha1.Rollout{}
assert.Equal(t, intstr.FromInt(DefaultMaxUnavailable), *GetMaxUnavailableOrDefault(rolloutDefaultValue))
assert.Equal(t, intstr.FromString(DefaultMaxUnavailable), *GetMaxUnavailableOrDefault(rolloutDefaultValue))
}

func TestGetCanaryIngressAnnotationPrefixOrDefault(t *testing.T) {
Expand Down