Skip to content

Commit a52af7d

Browse files
authored
fix(ecs): enabling the circuitBreaker is not recommended loudly enough (#37755)
If a simple misconfiguration in an ECS Task Definition causes the task to never start, a Service based off of that Task Definition takes 3 hours to fail unless `circuitBreaker` is configured. This is not obvious: our examples don't show that and the construct itself never tells you about it. Update our top-level README example to include the circuit breaker field, and make the construct warn you if you didn't configure the property (you most likely always want it). Also remove `desiredCapacity` from the initial example, which is not recommended anymore because setting that unscales your service during deployments (it even warns). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent a661c2d commit a52af7d

7 files changed

Lines changed: 58 additions & 8 deletions

File tree

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/fargate-service-base.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { FargatePlatformVersion, FargateTaskDefinition, RuntimePlatform } from '../../../aws-ecs';
1+
import type { DeploymentCircuitBreaker, FargatePlatformVersion, FargateTaskDefinition, RuntimePlatform } from '../../../aws-ecs';
22

33
export interface FargateServiceBaseProps {
44
/**
@@ -89,4 +89,11 @@ export interface FargateServiceBaseProps {
8989
* @default - If the property is undefined, `operatingSystemFamily` is LINUX and `cpuArchitecture` is X86_64
9090
*/
9191
readonly runtimePlatform?: RuntimePlatform;
92+
93+
/**
94+
* Whether to enable the deployment circuit breaker. If this property is defined, circuit breaker will be implicitly
95+
* enabled.
96+
* @default - disabled
97+
*/
98+
readonly circuitBreaker?: DeploymentCircuitBreaker;
9299
}

packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/network-multiple-target-groups-fargate-service.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ export class NetworkMultipleTargetGroupsFargateService extends NetworkMultipleTa
159159
enableExecuteCommand: props.enableExecuteCommand,
160160
minHealthyPercent: props.minHealthyPercent,
161161
maxHealthyPercent: props.maxHealthyPercent,
162+
circuitBreaker: props.circuitBreaker,
162163
});
163164
}
164165
}

packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/network-multiple-target-groups-fargate-service.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ describe('NetworkMultipleTargetGroupsFargateService', () => {
4343
image: ecs.ContainerImage.fromRegistry('/aws/aws-example-app'),
4444
},
4545
minHealthyPercent: 100,
46+
circuitBreaker: {}, // Set to silence an unrelated warning
4647
});
4748

4849
// THEN - no warning about minHealthyPercent

packages/aws-cdk-lib/aws-ecs/README.md

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ const cluster = new ecs.Cluster(this, 'Cluster', { vpc });
2121
// Add capacity to it
2222
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
2323
instanceType: new ec2.InstanceType("t2.xlarge"),
24-
desiredCapacity: 3,
2524
});
2625

2726
const taskDefinition = new ecs.Ec2TaskDefinition(this, 'TaskDef');
@@ -36,6 +35,9 @@ const ecsService = new ecs.Ec2Service(this, 'Service', {
3635
cluster,
3736
taskDefinition,
3837
minHealthyPercent: 100,
38+
circuitBreaker: {
39+
enable: true,
40+
},
3941
});
4042
```
4143

@@ -127,7 +129,6 @@ const cluster = new ecs.Cluster(this, 'Cluster', {
127129
// Either add default capacity
128130
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
129131
instanceType: new ec2.InstanceType("t2.xlarge"),
130-
desiredCapacity: 3,
131132
});
132133

133134
// Or add customized capacity. Be sure to start the Amazon ECS-optimized AMI.
@@ -360,7 +361,7 @@ cluster.addCapacity('ASGEncryptedSNS', {
360361

361362
### Container Insights
362363

363-
On a cluster, CloudWatch Container Insights can be enabled by setting the `containerInsightsV2` property. [Container Insights](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/cloudwatch-container-insights.html)
364+
On a cluster, CloudWatch Container Insights can be enabled by setting the `containerInsightsV2` property. [Container Insights](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/cloudwatch-container-insights.html)
364365
can be disabled, enabled, or enhanced.
365366

366367
```ts
@@ -807,6 +808,9 @@ const service = new ecs.FargateService(this, 'Service', {
807808
taskDefinition,
808809
desiredCount: 5,
809810
minHealthyPercent: 100,
811+
circuitBreaker: {
812+
enable: true,
813+
},
810814
});
811815
```
812816

@@ -1683,7 +1687,7 @@ Capacity Option Type provides the purchasing option for the EC2 instances used i
16831687
See [ECS documentation for Managed Instances Capacity Provider](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/managed-instances-capacity-providers-concept.html) for more documentation.
16841688

16851689
#### IAM Roles Setup
1686-
Managed instances require an infrastructure and an EC2 instance profile. You can either provide your own infrastructure role and/or instance profile, or let the construct create them automatically.
1690+
Managed instances require an infrastructure and an EC2 instance profile. You can either provide your own infrastructure role and/or instance profile, or let the construct create them automatically.
16871691

16881692
Option 1: Let CDK create the role and instance profile automatically
16891693
```ts
@@ -1850,16 +1854,16 @@ const miCapacityProvider = new ecs.ManagedInstancesCapacityProvider(this, 'MICap
18501854
acceleratorManufacturers: [ec2.AcceleratorManufacturer.NVIDIA],
18511855
acceleratorNames: [ec2.AcceleratorName.T4, ec2.AcceleratorName.V100],
18521856
acceleratorCountMin: 1,
1853-
1857+
18541858
// Storage requirements
18551859
localStorage: ec2.LocalStorage.REQUIRED,
18561860
localStorageTypes: [ec2.LocalStorageType.SSD],
18571861
totalLocalStorageGBMin: 100,
1858-
1862+
18591863
// Network requirements
18601864
networkInterfaceCountMin: 2,
18611865
networkBandwidthGbpsMin: 10,
1862-
1866+
18631867
// Cost optimization
18641868
onDemandMaxPricePercentageOverLowestPrice: 10,
18651869
},

packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -949,6 +949,11 @@ export abstract class BaseService extends Resource
949949
Annotations.of(this)._addTrackableError(lit`CircuitBreakerRequiresEcsController`, 'Deployment circuit breaker requires the ECS deployment controller.');
950950
}
951951

952+
if (!props.circuitBreaker && this.isEcsDeploymentController) {
953+
// If we *could* use a circuit breaker, then let's recommend users to do so. It makes detecting errors sooo much faster.
954+
Annotations.of(this).addWarningV2('@aws-cdk/aws-ecs:shouldUseCircuitBreaker', 'Enable the \'circuitBreaker\' property to trigger a quicker deployment failure if tasks are failing to come start (without this setting deployments may take up to 3 hours to fail).');
955+
}
956+
952957
if (props.deploymentAlarms && !this.isEcsDeploymentController) {
953958
throw new ValidationError(lit`RequiresDeploymentAlarmsRequires`, 'Deployment alarms requires the ECS deployment controller.', this);
954959
}

packages/aws-cdk-lib/aws-ecs/test/ec2/ec2-service.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import * as s3 from '../../../aws-s3';
1111
import * as cloudmap from '../../../aws-servicediscovery';
1212
import * as cdk from '../../../core';
1313
import { App } from '../../../core';
14+
import { flattenMeta } from '../../../core/test/util';
1415
import { ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME } from '../../../cx-api';
1516
import * as ecs from '../../lib';
1617
import {
@@ -63,6 +64,36 @@ describe('ec2 service', () => {
6364
expect(service.node.defaultChild).toBeDefined();
6465
});
6566

67+
test.each([false, true])('suggests using circuitBreaker if %p set', (circuitBreakerSet) => {
68+
// GIVEN
69+
const app = new cdk.App();
70+
const stack = new cdk.Stack(app, 'Stack');
71+
const vpc = new ec2.Vpc(stack, 'MyVpc', {});
72+
const cluster = new ecs.Cluster(stack, 'EcsCluster', { vpc });
73+
addDefaultCapacityProvider(cluster, stack, vpc);
74+
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'Ec2TaskDef');
75+
76+
taskDefinition.addContainer('web', {
77+
image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
78+
memoryLimitMiB: 512,
79+
});
80+
81+
new ecs.Ec2Service(stack, 'Ec2Service', {
82+
cluster,
83+
taskDefinition,
84+
circuitBreaker: circuitBreakerSet ? { } : undefined,
85+
});
86+
87+
// THEN
88+
const warnings = flattenMeta(app.synth().getStackByName('Stack').metadata)['/Stack/Ec2Service']['aws:cdk:warning'];
89+
90+
if (circuitBreakerSet) {
91+
expect(warnings).not.toContainEqual(expect.stringContaining('Enable the \'circuitBreaker\' property'));
92+
} else {
93+
expect(warnings).toContainEqual(expect.stringContaining('Enable the \'circuitBreaker\' property'));
94+
}
95+
});
96+
6697
[false, undefined].forEach((value) => {
6798
test('set cloudwatch permissions based on falsy feature flag when no cloudwatch log configured', () => {
6899
// GIVEN

packages/aws-cdk-lib/aws-ecs/test/fargate/fargate-service.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3646,6 +3646,7 @@ describe('fargate service', () => {
36463646
cluster,
36473647
taskDefinition,
36483648
minHealthyPercent: 50, // must be set to avoid warning causing test failure
3649+
circuitBreaker: {}, // Set to silence an unrelated warning
36493650
});
36503651

36513652
// WHEN

0 commit comments

Comments
 (0)