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

feat: add ability to verify canary weights before advancing steps #957

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Jan 20, 2021

Resolves #701

This introduces an optional flag to the controller --alb-verify-weight=true. When enabled, for ALB ingresses, the controller will block a setWeight step until it is verified applied in the ALB target group listener rules. This requires that the controller has sufficient AWS privileges.

Changes the TrafficRoutingReconciler interface from:

type TrafficRoutingReconciler interface {
	Reconcile(desiredWeight int32) error
}

to

type TrafficRoutingReconciler interface {
	// SetWeight sets the canary weight to the desired weight
	SetWeight(desiredWeight int32) error
	// VerifyWeight returns true if the canary is at the desired weight
	VerifyWeight(desiredWeight int32) (bool, error)
	// Type returns the type of the traffic routing reconciler
	Type() string		Type() string
}

to support this.

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

@jessesuen jessesuen force-pushed the verify-weight branch 3 times, most recently from 16ca7e1 to 2d87c4a Compare January 25, 2021 19:21
@jessesuen jessesuen marked this pull request as ready for review January 25, 2021 19:22
@jessesuen jessesuen force-pushed the verify-weight branch 4 times, most recently from 97710a1 to cfc0f8d Compare January 28, 2021 01:44
@codecov-io
Copy link

Codecov Report

Merging #957 (cfc0f8d) into master (1beffd4) will decrease coverage by 0.27%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #957      +/-   ##
==========================================
- Coverage   81.60%   81.33%   -0.28%     
==========================================
  Files          99      100       +1     
  Lines        8732     8839     +107     
==========================================
+ Hits         7126     7189      +63     
- Misses       1151     1181      +30     
- Partials      455      469      +14     
Impacted Files Coverage Δ
rollout/context.go 90.19% <ø> (ø)
rollout/trafficrouting/istio/istio.go 90.22% <0.00%> (-0.69%) ⬇️
rollout/trafficrouting/nginx/nginx.go 87.00% <0.00%> (-0.88%) ⬇️
rollout/trafficrouting/smi/smi.go 94.50% <0.00%> (-0.53%) ⬇️
rollout/trafficrouting/alb/alb.go 82.88% <57.50%> (-14.34%) ⬇️
utils/aws/aws.go 62.00% <62.00%> (ø)
rollout/canary.go 79.40% <81.81%> (-1.66%) ⬇️
ingress/alb.go 96.29% <100.00%> (ø)
rollout/trafficrouting.go 93.93% <100.00%> (+1.34%) ⬆️
pkg/kubectl-argo-rollouts/cmd/create/create.go 66.01% <0.00%> (ø)
... and 1 more

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 1beffd4...cfc0f8d. Read the comment docs.

Copy link
Contributor

@khhirani khhirani left a comment

Choose a reason for hiding this comment

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

LGTM

rollout/canary.go Outdated Show resolved Hide resolved
Signed-off-by: Jesse Suen <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Feb 1, 2021

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
3.6% 3.6% Duplication

@jessesuen jessesuen merged commit b30da39 into argoproj:master Feb 1, 2021
@jessesuen jessesuen deleted the verify-weight branch February 1, 2021 22:13
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.

Ability to verify weight before completing setWeight steps
4 participants