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.

Sign up for GitHub

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

Jump to bottom

feat(ecs-pattens): pass healthy percent & deregistration delay params #28627

Closed
wants to merge 3 commits into from
Closed

feat(ecs-pattens): pass healthy percent & deregistration delay params #28627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
383ead0
feat(ecs): pass healthy percent & deregistraiton delay params in L3 c…
cheruvian Mar 26, 2024
7216c15
feat(ecs-patterns): add stickinessCookieDuration and minHealth naming…
cheruvian Mar 26, 2024
064754c
chore(ecs-patterns): minHealthyPercent naming for consistency
cheruvian Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions ...pplication-load-balanced-ecs-service.js.snapshot/aws-ecs-integ-multiple-alb.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,8 +969,19 @@
"Protocol": "HTTP",
"TargetGroupAttributes": [
{
"Key": "deregistration_delay.timeout_seconds",
"Value": "10"
},{
"Key": "stickiness.enabled",
"Value": "false"
"Value": "true"
},
{
"Key": "stickiness.type",
"Value": "lb_cookie"
},
{
"Key": "stickiness.lb_cookie.duration_seconds",
"Value": "240"
}
],
"TargetType": "instance",
Expand Down Expand Up @@ -1188,7 +1199,7 @@
"Rollback": false
},
"MaximumPercent": 200,
"MinimumHealthyPercent": 50
"MinimumHealthyPercent": 33
},
"EnableECSManagedTags": false,
"EnableExecuteCommand": true,
Expand Down
5 changes: 4 additions & 1 deletion ...eg/test/aws-ecs-patterns/test/ec2/integ.multiple-application-load-balanced-ecs-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { InstanceType, Vpc } from 'aws-cdk-lib/aws-ec2';
import { Cluster, ContainerImage } from 'aws-cdk-lib/aws-ecs';
import { App, Stack } from 'aws-cdk-lib';
import { App, Duration, Stack } from 'aws-cdk-lib';
import * as integ from '@aws-cdk/integ-tests-alpha';
import { ApplicationMultipleTargetGroupsEc2Service } from 'aws-cdk-lib/aws-ecs-patterns';
import { AUTOSCALING_GENERATE_LAUNCH_TEMPLATE } from 'aws-cdk-lib/cx-api';
Expand All @@ -19,9 +19,12 @@ new ApplicationMultipleTargetGroupsEc2Service(stack, 'myService', {
image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
},
enableExecuteCommand: true,
minHealthyPercent: 33,
targetGroups: [
{
containerPort: 80,
deregistrationDelay: Duration.seconds(10),
stickinessCookieDuration: Duration.minutes(4),
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an integration test for NLB as well?

},
{
containerPort: 90,
Expand Down
4 changes: 4 additions & 0 deletions packages/aws-cdk-lib/aws-ecs-patterns/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,11 @@ const loadBalancedEc2Service = new ecsPatterns.ApplicationMultipleTargetGroupsEc
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
},
minHealthyPercent: 33,
targetGroups: [
{
containerPort: 80,
deregistrationDelay: Duration.seconds(300),
},
{
containerPort: 90,
Expand All @@ -110,9 +112,11 @@ const loadBalancedFargateService = new ecsPatterns.ApplicationMultipleTargetGrou
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
},
minHealthyPercent: 33,
targetGroups: [
{
containerPort: 80,
deregistrationDelay: Duration.seconds(300),
},
{
containerPort: 90,
Expand Down
32 changes: 32 additions & 0 deletions .../aws-cdk-lib/aws-ecs-patterns/lib/base/application-multiple-target-groups-service-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,16 @@ export interface ApplicationMultipleTargetGroupsServiceBaseProps {
* @default - false
*/
readonly enableExecuteCommand?: boolean;

/**
* The minimum number of tasks, specified as a percentage of
* the Amazon ECS service's DesiredCount value, that must
* continue to run and remain healthy during a deployment.
*
* @see https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DeploymentConfiguration.html
* @default - 0 if daemon, otherwise 50
*/
readonly minHealthyPercent?: number;
}

/**
Expand Down Expand Up @@ -267,6 +277,26 @@ export interface ApplicationTargetProps {
* @default No path condition
*/
readonly pathPattern?: string;

/**
* The amount of time for Elastic Load Balancing to wait before deregistering a target.
*
* The range is 0-3600 seconds.
*
* @default 300
*/
readonly deregistrationDelay?: Duration;
/**
* The stickiness cookie expiration period.
*
* Setting this value enables load balancer stickiness.
*
* After this period, the cookie is considered stale. The minimum value is
* 1 second and the maximum value is 7 days (604800 seconds).
*
* @default Duration.days(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be Stickiness is disabled. See #29726

*/
readonly stickinessCookieDuration?: Duration;
}

/**
Expand Down Expand Up @@ -515,6 +545,8 @@ export abstract class ApplicationMultipleTargetGroupsServiceBase extends Constru
}),
],
conditions,
deregistrationDelay: targetProps.deregistrationDelay,
stickinessCookieDuration: targetProps.stickinessCookieDuration,
priority: targetProps.priority,
});
this.targetGroups.push(targetGroup);
Expand Down
20 changes: 20 additions & 0 deletions ...ages/aws-cdk-lib/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ export interface NetworkMultipleTargetGroupsServiceBaseProps {
* @default - false
*/
readonly enableExecuteCommand?: boolean;

/**
* The minimum number of tasks, specified as a percentage of
* the Amazon ECS service's DesiredCount value, that must
* continue to run and remain healthy during a deployment.
*
* @see https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DeploymentConfiguration.html
* @default - 0 if daemon, otherwise 50
*/
readonly minHealthyPercent?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add an @see here with a link to the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, I think I'd prefer this to be minimumHealthyPercent to match CloudFormation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not clear which docs link you'd prefer (CDK, CF or ECS), chose this: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DeploymentConfiguration.html

I also changed the naming per your recommendation to get this approved, however I don't love this since it diverges from the usage/naming used throughout the CDK (already been established in BaseServiceOptions)

https://github.com/aws/aws-cdk/blob/v2.133.0/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L298
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs.FargateService.html#minhealthypercent

If CDK should match the CF naming then it should be updated everywhere rather than piecemeal IMO. This also makes it more of a challenge to re-use BaseServiceOptions similar to the above recommendation to just use ApplciationTargetProps. Are there any thoughts on this from the CDK team?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cheruvian I didn't see it in BaseServiceOptions. Apologies, you can change it to make that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rest LGTM, if you want to change this back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thoughts on whether this should actually just extend Omit<BaseServiceOptions, ...>?

}

/**
Expand Down Expand Up @@ -267,6 +277,15 @@ export interface NetworkTargetProps {
* @default - default listener (first added listener)
*/
readonly listener?: string;

/**
* The amount of time for Elastic Load Balancing to wait before deregistering a target.
*
* The range is 0-3600 seconds.
*
* @default 300
*/
readonly deregistrationDelay?: Duration;
}

/**
Expand Down Expand Up @@ -393,6 +412,7 @@ export abstract class NetworkMultipleTargetGroupsServiceBase extends Construct {
containerPort: targetProps.containerPort,
}),
],
deregistrationDelay: targetProps.deregistrationDelay,
});
this.targetGroups.push(targetGroup);
}
Expand Down
1 change: 1 addition & 0 deletions ...es/aws-cdk-lib/aws-ecs-patterns/lib/ecs/application-multiple-target-groups-ecs-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ export class ApplicationMultipleTargetGroupsEc2Service extends ApplicationMultip
enableExecuteCommand: props.enableExecuteCommand,
placementConstraints: props.placementConstraints,
placementStrategies: props.placementStrategies,
minHealthyPercent: props.minHealthyPercent,
});
}
}
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-ecs-patterns/lib/ecs/network-multiple-target-groups-ecs-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ export class NetworkMultipleTargetGroupsEc2Service extends NetworkMultipleTarget
enableExecuteCommand: props.enableExecuteCommand,
placementConstraints: props.placementConstraints,
placementStrategies: props.placementStrategies,
minHealthyPercent: props.minHealthyPercent,
});
}
}
4 changes: 4 additions & 0 deletions packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s-v2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ describe('ApplicationMultipleTargetGroupsEc2Service', () => {
],
placementStrategies: [PlacementStrategy.spreadAcrossInstances(), PlacementStrategy.packedByCpu(), PlacementStrategy.randomly()],
placementConstraints: [PlacementConstraint.memberOf('attribute:ecs.instance-type =~ m5a.*')],
minHealthyPercent: 50,
});

// THEN
Expand Down Expand Up @@ -197,6 +198,7 @@ describe('ApplicationMultipleTargetGroupsEc2Service', () => {
ServiceName: 'myService',
PlacementConstraints: [{ Type: 'memberOf', Expression: 'attribute:ecs.instance-type =~ m5a.*' }],
PlacementStrategies: [{ Field: 'instanceId', Type: 'spread' }, { Field: 'CPU', Type: 'binpack' }, { Type: 'random' }],
DeploymentConfiguration: { MinimumHealthyPercent: 50 },
});

Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', {
Expand Down Expand Up @@ -1148,6 +1150,7 @@ describe('NetworkMultipleTargetGroupsEc2Service', () => {
],
placementStrategies: [PlacementStrategy.spreadAcrossInstances(), PlacementStrategy.packedByCpu(), PlacementStrategy.randomly()],
placementConstraints: [PlacementConstraint.memberOf('attribute:ecs.instance-type =~ m5a.*')],
minHealthyPercent: 50,
});

// THEN
Expand Down Expand Up @@ -1178,6 +1181,7 @@ describe('NetworkMultipleTargetGroupsEc2Service', () => {
ServiceName: 'myService',
PlacementConstraints: [{ Type: 'memberOf', Expression: 'attribute:ecs.instance-type =~ m5a.*' }],
PlacementStrategies: [{ Field: 'instanceId', Type: 'spread' }, { Field: 'CPU', Type: 'binpack' }, { Type: 'random' }],
DeploymentConfiguration: { MinimumHealthyPercent: 50 },
});

Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', {
Expand Down
Loading