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.

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

Conversation

khhirani
Copy link
Contributor

@khhirani khhirani commented Feb 5, 2021

Closes #938

Signed-off-by: khhirani [email protected]

@khhirani khhirani force-pushed the canary-maxunavailable-default branch from adbd69e to 0d0ee20 Compare February 5, 2021 22:49
@codecov-io
Copy link

codecov-io commented Feb 5, 2021

Codecov Report

Merging #981 (0d0ee20) into master (7384684) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #981   +/-   ##
=======================================
  Coverage   81.36%   81.36%           
=======================================
  Files         100      100           
  Lines        8844     8844           
=======================================
  Hits         7196     7196           
  Misses       1180     1180           
  Partials      468      468           
Impacted Files Coverage Δ
utils/defaults/defaults.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7384684...0d0ee20. Read the comment docs.

@khhirani khhirani requested a review from jessesuen February 5, 2021 22:59
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Looks good. Can you update both the documentation and the types.go to reflect this?

Comment on lines 15 to +17
DefaultMaxSurge = "25"
// DefaultMaxUnavailable default number for the max number of unavailable pods during a rollout
DefaultMaxUnavailable = 0
DefaultMaxUnavailable = "25"
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")
}

Signed-off-by: khhirani <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@khhirani khhirani merged commit a87bf87 into argoproj:master Feb 10, 2021
@khhirani khhirani deleted the canary-maxunavailable-default branch February 10, 2021 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canary strategy is not defaulting maxUnavailable to 25%
3 participants