Support weighted target group canary deployments - #191
Conversation
The canary path now reuses an existing ALB/NLB listener and forwards by target group weight instead of creating a separate canary load balancer. The manifest can reference the listener by load balancer name and port, keeps listener_arn for compatibility, and supports a simple bake_time wait after canary traffic is enabled. Constraint: Current requirement targets default listener actions, not host/path listener rules Constraint: Bake time is only a time gate and does not imply health/alarm based promotion Rejected: Reintroduce separate canary load balancers | existing listener weighted target groups are the requested deployment shape Rejected: Add a promotion policy engine | CloudWatch/API gates need separate operational criteria Confidence: high Scope-risk: moderate Directive: Do not remove the weight=0 pre-health-check listener attach without verifying ALB/NLB target groups leave unused state Tested: go test -count=1 ./pkg/... ./cmd/... ./hack/... Tested: make linters Tested: live AWS stable/canary/complete flow on demoapp-xyzdapne2-ext Not-tested: NLB live deployment path Related: #189 Related: #190
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds weighted canary deployment support across configuration, docs, examples, AWS ELBv2 helpers, and canary deployment flow. The canary path now uses listener weighted forward actions, validates listener and weight settings, and reattaches target groups during completion. ChangesWeighted Canary Deployment
Sequence Diagram(s)sequenceDiagram
participant Runner
participant Canary
participant ELBV2Client
Runner->>Canary: health checking / deploy
Canary->>Canary: ValidateCanaryDeployment
Canary->>ELBV2Client: GetLoadBalancerByName(...)
Canary->>ELBV2Client: ModifyListenerWeightedForward(...)
Canary->>Canary: wait bake_time
Canary->>ELBV2Client: ModifyListenerWeightedForward(... 100/0)
Canary->>Canary: CompleteCanaryDeployment
Canary->>Canary: DetachLatestCanaryResources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR successfully implements weighted target group canary deployments as a replacement for the previous dedicated load balancer approach. The implementation is generally solid with comprehensive changes across deployment logic, AWS service integration, and configuration schemas.
Key Changes
- Replaced dedicated canary load balancer/security group creation with weighted target group forwarding on existing ALB/NLB listeners
- Added manifest fields for specifying listener by load balancer name + port instead of requiring ARN
- Implemented bake time support to allow deployments to wait after applying canary weights
- Fixed recursive call bug in
Canary.RunAPITestby properly delegating toDeployer.RunAPITest - Fixed health check panic from empty
AppliedCapacityin complete canary path - Fixed cleanup loop that incorrectly waited for the latest ASG
Critical Finding
1 issue requiring attention: The weight validation allows values up to 99, which creates an extreme traffic imbalance (1% stable, 99% canary). While technically functional, this may cause operational issues. Consider if a lower maximum (e.g., 90) would provide better safety margins.
Testing
The PR includes comprehensive unit tests and real AWS environment validation with successful end-to-end verification including:
- Stable and canary deployments
- Weighted traffic distribution
- Complete canary promotion
- Resource cleanup
Documentation
Both Korean and English documentation have been updated with the new weighted canary concepts, manifest examples, and NLB-specific constraints.
Overall, this is a well-implemented feature with proper testing and documentation. The single finding noted above should be evaluated but does not necessarily block merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| if regionConfig.Canary.Weight < 0 || regionConfig.Canary.Weight > 99 { | ||
| return errors.New("canary.weight must be between 0 and 99") |
There was a problem hiding this comment.
🛑 Logic Error: The validation logic allows Weight=99 which would cause a crash when calculating stableWeight (100-99=1, canaryWeight=99, both non-zero). When weight is 99, the stable target group receives only 1% traffic. However, AWS ELB weighted target groups require that at least one target group weight must be greater than 0. While this technically works, a weight of 99 creates an extreme imbalance that could cause operational issues. Consider whether 99 should be the maximum allowed value or if a lower maximum (e.g., 90) would be more appropriate for safety.
The canary weight field is a percentage, so allowing 99 leaves only 1 percent stable traffic. That is technically valid for ELB weighted target groups, but too aggressive for the safe canary default this feature is intended to provide. Constraint: Canary weight is modeled as a percentage, not raw ELB 0-999 weight values Rejected: Keep 99 as the upper bound | it permits near-full traffic shift before complete-canary Confidence: high Scope-risk: narrow Directive: Keep percentage validation aligned with docs and schema comments Tested: go test ./pkg/deployer -run 'TestValidateCanaryDeployment|TestCanaryWeights' Tested: go test -count=1 ./pkg/... ./cmd/... ./hack/... Tested: make linters Related: #191
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/aws/elbv2.go (1)
191-203: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unreachable fallback.
DescribeLoadBalancerswithNamesraisesLoadBalancerNotFoundExceptionfor a missing load balancer, so thelen(result.LoadBalancers) == 0branch won’t handle that case. Remove it or map that error here if you wantnil, nilto mean not found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/aws/elbv2.go` around lines 191 - 203, In GetLoadBalancerByName on ELBV2Client, the empty result fallback is unreachable because DescribeLoadBalancers with Names returns an error for a missing load balancer. Remove the len(result.LoadBalancers) == 0 branch, or explicitly handle LoadBalancerNotFoundException in this method if you want not-found to return nil, nil; keep the main error path in e.Client.DescribeLoadBalancers as the source of truth.docs/content/en/docs/Concepts/canary.md (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor hyphenation nits. Use "rule-based" (Line 19) and "API test-based" (Line 50) as compound modifiers.
Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/content/en/docs/Concepts/canary.md` at line 19, The doc text has compound-modifier hyphenation issues in the canary content. Update the phrasing in the canary description to use “rule-based” in the listener routing sentence, and also adjust the later “API test-based” wording plus the “50-50” phrasing as needed for consistent compound-modifier style. Use the canary markdown content as the source and revise the affected sentences directly.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/deployer/canary.go`:
- Around line 311-312: The canary weight handling is ambiguous because
`Validate` allows `regionConfig.Canary.Weight` to be 0 while `CanaryWeights`
later interprets 0 as “unset” and falls back to a 10% canary. Update the
`CanaryWeights`/canary config flow so `weight: 0` is either explicitly preserved
via a nullable/pointer field or clearly rejected/documented as unsupported; make
the validation and weight-selection logic consistent, and apply the same fix in
the other referenced canary-weight paths so zero is never silently converted
into default canary traffic.
---
Nitpick comments:
In `@docs/content/en/docs/Concepts/canary.md`:
- Line 19: The doc text has compound-modifier hyphenation issues in the canary
content. Update the phrasing in the canary description to use “rule-based” in
the listener routing sentence, and also adjust the later “API test-based”
wording plus the “50-50” phrasing as needed for consistent compound-modifier
style. Use the canary markdown content as the source and revise the affected
sentences directly.
In `@pkg/aws/elbv2.go`:
- Around line 191-203: In GetLoadBalancerByName on ELBV2Client, the empty result
fallback is unreachable because DescribeLoadBalancers with Names returns an
error for a missing load balancer. Remove the len(result.LoadBalancers) == 0
branch, or explicitly handle LoadBalancerNotFoundException in this method if you
want not-found to return nil, nil; keep the main error path in
e.Client.DescribeLoadBalancers as the source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d41f700d-99c5-416c-bbc5-5eb78de4bf02
📒 Files selected for processing (13)
docs/content/en/docs/Concepts/canary.mddocs/content/en/schemas/schema.jsondocs/content/ko/docs/Concepts/canary.mdexamples/manifests/canary-example.yamlexamples/manifests/demoapp-xyzd-canary.yamlexamples/manifests/demoapp-xyzd-stable.yamlexamples/scripts/demoapp-userdata-canary.shexamples/scripts/demoapp-userdata.shpkg/aws/elbv2.gopkg/aws/elbv2_test.gopkg/deployer/canary.gopkg/deployer/canary_test.gopkg/schemas/config.go
* Rollback failed canaries before promotion Health check failures previously returned from the canary deployer only as a local error path, while the runner did not stop the deployment after health errors. The canary rollback now restores listener traffic, removes canary-only target group resources, scales the failed canary ASG down, waits for termination, and deletes the ASG and launch template before returning the original health failure. The live rollback test also exposed that the stable ASG is stored in LatestAsg while the newly created canary ASG is stored in AsgNames. Rollback and canary target group detach now select the active canary ASG first to avoid touching the stable group. Constraint: Canary rollback must reuse the existing listener default action and target group cleanup helpers from the weighted target group implementation Rejected: Leave failed canary ASGs scaled to zero | operators would still need manual cleanup after every failed canary Rejected: Roll back using LatestAsg only | live AWS test showed this targets the stable ASG during initial canary deploy Confidence: high Scope-risk: moderate Directive: Do not replace AsgNames with LatestAsg in rollback paths without re-running an unhealthy canary against a real or mocked ASG/TG lifecycle Tested: env -u GOROOT go test -count=1 ./pkg/... ./cmd/... ./hack/... Tested: env -u GOROOT make linters Tested: AWS ap-northeast-2 unhealthy canary deploy returned GOPLOYER_EXIT:1, restored listener to stable TG, left v001 1/1/1 healthy, deleted canary TG, and removed v002 launch templates Not-tested: CloudWatch alarm based post-deploy rollback, which is not implemented by this change * Address canary rollback review comments
1 participant
What changed
Closes #189
Weighted target group canary
stable=100andcanary=0so the canary target group becomes active for load balancer health checks. Without this, AWS reports the target group asunused.canary.weightvalue. The default remains10.--complete-canaryattaches the canary ASG to the original target group, restores the listener to a single original target group forward action, then detaches and deletes the canary target group.complete-canarynil panic caused by missingAppliedCapacityduring health checks.Canary.RunAPITest, which previously called itself recursively instead of delegating toDeployer.RunAPITest.Manifest UX
The manifest no longer needs a long listener ARN. New manifests can use load balancer name plus listener port:
canary.listener_arnis still supported for backward compatibility.listener_arnis omitted, Goployer resolves the listener withDescribeLoadBalancersandDescribeListeners.listener_protocolis optional and only used to make the listener match explicit.Bake time
canary.bake_time.Docs and examples
What was tested
Local checks
go test -count=1 ./pkg/... ./cmd/... ./hack/...make lintersgit diff --checkUnit tests
Added or updated tests for:
Canary.RunAPITestdelegation toDeployer.RunAPITestLive AWS validation
Validated the full flow in a test AWS account using these existing resources:
vpc-021c0886b3de5a28bdemoapp-xyzdapne2-extdemoapp-xyzdapne2-extHTTPS:443demoapp-xyzd_apnortheast2-appValidation results:
demoapp-xyzd_apnortheast2-v000.demoapp-xyzd_apnortheast2-v001.stable=90,canary=10.stable=49,canary=11.--complete-canaryrestored the listener to a single original target group forward action.demoapp-xyzd-canary-v001was deleted.demoapp-xyzd_apnortheast2-v001, desired/min/max1/1/1, target healthhealthy.{"app":"demoapp","version":"canary","path":"/"} {"status":"UP"}Remaining work
Tracked in #190
canary.bake_timeonly waits. It does not decide whether the canary is safe to promote.UnHealthyHostCount == 0HTTPCode_Target_5XX_Count == 0TargetResponseTimebelow a configured threshold--complete-canarypromotion gate.Operational notes
load_balancer+listener_portover directlistener_arn.--complete-canary..gitignorehas an unrelated local.omx/change in my working tree; it is intentionally not included in this PR.Summary by CodeRabbit
New Features
--complete-canarycompletion flow.Documentation
Bug Fixes
Tests