fix(ecs): suppress shouldUseCircuitBreaker warning for DAEMON scheduling strategy services - #38140
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an exclusion so ECS “should use circuit breaker” warnings aren’t emitted for DAEMON services, and introduces a regression test covering the DAEMON case.
Changes:
- Add a DAEMON scheduling-strategy guard to the circuit breaker recommendation warning.
- Add a new unit test ensuring no circuit breaker suggestion is emitted for daemon EC2 services.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-service.test.ts | Adds coverage to ensure DAEMON services don’t get circuit breaker recommendation warnings. |
| packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts | Skips the circuit-breaker recommendation warning when scheduling strategy is DAEMON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
21adc24 to
ef50dd8
Compare
The ECS deployment circuit breaker is desiredCount-based (threshold = 0.5 × desiredCount) and is not applicable to DAEMON scheduling strategy services, which have no desiredCount. BaseService was emitting a false-positive shouldUseCircuitBreaker warning for daemon services because the guard only checked !circuitBreaker && isEcsDeploymentController without accounting for the scheduling strategy. Fix: skip the warning when schedulingStrategy is DAEMON, consistent with how CDK already special-cases daemon mode for desiredCount validation, minHealthyPercent, and placement strategy checks. Fixes aws#38102 Signed-off-by: Radhakrishnan Pachyappan <radhakrishnan.p@op.tech>
ef50dd8 to
4236026
Compare
|
Exemption Request This PR only suppresses a false-positive advisory annotation () for DAEMON scheduling-strategy services. It does not change any synthesized CloudFormation template, resource property, or deployment behavior — the only observable effect is that no longer emits a misleading warning for daemon services. Integration tests verify infrastructure behavior via snapshot diffs; because no snapshot would change as a result of this fix, an integration test would add no signal beyond what the existing unit tests already cover. |
Summary
Fixes #38102
The
@aws-cdk/aws-ecs:shouldUseCircuitBreakerwarning fires forEc2Servicewithdaemon: true, even though the ECS deployment circuit breaker does not apply to DAEMON services. The circuit breaker threshold is0.5 × desiredCount(min 3), but DAEMON services have nodesiredCount— they run one task per container instance. Following the warning's advice and settingcircuitBreakeron a daemon service is meaningless, and the only way to silence the false positive today isacknowledgeWarning().Root cause:
BaseServiceguards the warning with!props.circuitBreaker && this.isEcsDeploymentController, with no check for the scheduling strategy.Ec2ServicepassesschedulingStrategyonly inadditionalProps, soBaseServicenever accounts for it.Fix: Add
additionalProps?.schedulingStrategy !== 'DAEMON'to the warning guard. This is consistent with how CDK already special-cases daemon mode in multiple other places (desiredCountvalidation,minHealthyPercentdefault/warning,availabilityZoneRebalancingcheck, placement strategy checks).Changes
shouldUseCircuitBreakerwarning whenschedulingStrategyisDAEMONTest plan
does not suggest circuitBreaker for DAEMON scheduling strategy services— passessuggests using circuitBreaker if false setandsuggests using circuitBreaker if true set— still pass (non-daemon replica services are unaffected)