-
Notifications
You must be signed in to change notification settings - Fork 74
feat(cli): configurable timeout for ECS hotswap operation #417
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
Changes from all commits
9ac2f0c
86f0edf
12b5385
0e54dba
5e41ea3
91c5661
759f511
4ed46f6
cc14cb5
b6a7500
19e3c37
b6c7741
218d9c7
35adbd4
67b80e1
5210b6c
6ac4929
c1ecb4b
c425575
22113eb
79f8cb8
eb94795
376b3c6
ea80af1
4fc3348
bc6e336
69e53ef
a01e45c
bd4c439
488ef80
5778e4a
9516a49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -724,5 +724,97 @@ describe.each([ | |
| }, | ||
| forceNewDeployment: true, | ||
| }); | ||
| expect(mockECSClient).toHaveReceivedCommandWith(DescribeServicesCommand, { | ||
| cluster: 'arn:aws:ecs:region:account:service/my-cluster', | ||
| services: ['arn:aws:ecs:region:account:service/my-cluster/my-service'], | ||
| }); | ||
|
Comment on lines
+727
to
+730
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed this assertion was missing, to make sure we actually call the waiter. |
||
| }); | ||
| }); | ||
|
|
||
| test.each([ | ||
| // default case | ||
| [101, undefined], | ||
| [2, 10], | ||
| [11, 60], | ||
| ])('DesribeService is called %p times when timeout is %p', async (describeAttempts: number, timeoutSeconds?: number) => { | ||
| setup.setCurrentCfnStackTemplate({ | ||
| Resources: { | ||
| TaskDef: { | ||
| Type: 'AWS::ECS::TaskDefinition', | ||
| Properties: { | ||
| Family: 'my-task-def', | ||
| ContainerDefinitions: [ | ||
| { Image: 'image1' }, | ||
| ], | ||
| }, | ||
| }, | ||
| Service: { | ||
| Type: 'AWS::ECS::Service', | ||
| Properties: { | ||
| TaskDefinition: { Ref: 'TaskDef' }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| setup.pushStackResourceSummaries( | ||
| setup.stackSummaryOf('Service', 'AWS::ECS::Service', | ||
| 'arn:aws:ecs:region:account:service/my-cluster/my-service'), | ||
| ); | ||
| mockECSClient.on(RegisterTaskDefinitionCommand).resolves({ | ||
| taskDefinition: { | ||
| taskDefinitionArn: 'arn:aws:ecs:region:account:task-definition/my-task-def:3', | ||
| }, | ||
| }); | ||
| const cdkStackArtifact = setup.cdkStackArtifactOf({ | ||
| template: { | ||
| Resources: { | ||
| TaskDef: { | ||
| Type: 'AWS::ECS::TaskDefinition', | ||
| Properties: { | ||
| Family: 'my-task-def', | ||
| ContainerDefinitions: [ | ||
| { Image: 'image2' }, | ||
| ], | ||
| }, | ||
| }, | ||
| Service: { | ||
| Type: 'AWS::ECS::Service', | ||
| Properties: { | ||
| TaskDefinition: { Ref: 'TaskDef' }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // WHEN | ||
| let ecsHotswapProperties = new EcsHotswapProperties(undefined, undefined, timeoutSeconds); | ||
| // mock the client such that the service never becomes stable using desiredCount > runningCount | ||
| mockECSClient.on(DescribeServicesCommand).resolves({ | ||
| services: [ | ||
| { | ||
| serviceArn: 'arn:aws:ecs:region:account:service/my-cluster/my-service', | ||
| taskDefinition: 'arn:aws:ecs:region:account:task-definition/my-task-def:3', | ||
| desiredCount: 1, | ||
| runningCount: 0, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| jest.useFakeTimers(); | ||
| jest.spyOn(global, 'setTimeout').mockImplementation((callback, ms) => { | ||
| callback(); | ||
| jest.advanceTimersByTime(ms ?? 0); | ||
| return {} as NodeJS.Timeout; | ||
| }); | ||
|
|
||
| await expect(hotswapMockSdkProvider.tryHotswapDeployment( | ||
| HotswapMode.HOTSWAP_ONLY, | ||
| cdkStackArtifact, | ||
| {}, | ||
| new HotswapPropertyOverrides(ecsHotswapProperties), | ||
| )).rejects.toThrow('Resource is not in the expected state due to waiter status'); | ||
|
|
||
| // THEN | ||
| expect(mockECSClient).toHaveReceivedCommandTimes(DescribeServicesCommand, describeAttempts); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -158,6 +158,18 @@ export async function makeConfig(): Promise<CliConfig> { | |||||||||||
| 'and falls back to a full deployment if that is not possible. ' + | ||||||||||||
| 'Do not use this in production environments', | ||||||||||||
| }, | ||||||||||||
| 'hotswap-ecs-minimum-healthy-percent': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Lower limit on the number of your service\'s tasks that must remain in the RUNNING state during a deployment, as a percentage of the desiredCount', | ||||||||||||
| }, | ||||||||||||
| 'hotswap-ecs-maximum-healthy-percent': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Upper limit on the number of your service\'s tasks that are allowed in the RUNNING or PENDING state during a deployment, as a percentage of the desiredCount', | ||||||||||||
| }, | ||||||||||||
|
Comment on lines
+161
to
+168
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were just flat out missing from the yargs definition, even though we did (try to) handle them in: aws-cdk-cli/packages/aws-cdk/lib/cli/user-configuration.ts Lines 306 to 310 in 3823937
|
||||||||||||
| 'hotswap-ecs-stabilization-timeout-seconds': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Number of seconds to wait for a single service to reach stable state, where the desiredCount is equal to the runningCount', | ||||||||||||
| }, | ||||||||||||
| 'watch': { | ||||||||||||
| type: 'boolean', | ||||||||||||
| desc: 'Continuously observe the project files, ' + | ||||||||||||
|
|
@@ -275,6 +287,18 @@ export async function makeConfig(): Promise<CliConfig> { | |||||||||||
| 'which skips CloudFormation and updates the resources directly, ' + | ||||||||||||
| 'and falls back to a full deployment if that is not possible.', | ||||||||||||
| }, | ||||||||||||
| 'hotswap-ecs-minimum-healthy-percent': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Lower limit on the number of your service\'s tasks that must remain in the RUNNING state during a deployment, as a percentage of the desiredCount', | ||||||||||||
| }, | ||||||||||||
| 'hotswap-ecs-maximum-healthy-percent': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Upper limit on the number of your service\'s tasks that are allowed in the RUNNING or PENDING state during a deployment, as a percentage of the desiredCount', | ||||||||||||
| }, | ||||||||||||
| 'hotswap-ecs-stabilization-timeout-seconds': { | ||||||||||||
| type: 'string', | ||||||||||||
| desc: 'Number of seconds to wait for a single service to reach stable state, where the desiredCount is equal to the runningCount', | ||||||||||||
| }, | ||||||||||||
| 'logs': { | ||||||||||||
| type: 'boolean', | ||||||||||||
| default: true, | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -305,8 +305,9 @@ export function commandLineArgumentsToSettings(argv: Arguments): Settings { | |||||||||||||||
| ignoreNoStacks: argv['ignore-no-stacks'], | ||||||||||||||||
| hotswap: { | ||||||||||||||||
| ecs: { | ||||||||||||||||
| minimumEcsHealthyPercent: argv.minimumEcsHealthyPercent, | ||||||||||||||||
| maximumEcsHealthyPercent: argv.maximumEcsHealthyPercent, | ||||||||||||||||
|
Comment on lines
-308
to
-309
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows for passing the hotswap override properties via the command line. However, it never actually worked because the keys it sets are not the keys we read: aws-cdk-cli/packages/aws-cdk/lib/cli/cdk-toolkit.ts Lines 387 to 393 in 3823937
|
||||||||||||||||
| minimumHealthyPercent: argv.hotswapEcsMinimumHealthyPercent, | ||||||||||||||||
| maximumHealthyPercent: argv.hotswapEcsMaximumHealthyPercent, | ||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So actually a user could never override this behavior from the CLI. Which means, we can change the argument names to be more descriptive:
These names are derived and match the argument names that I think we initially wanted, based on: aws-cdk-cli/packages/@aws-cdk/toolkit-lib/lib/api/hotswap/common.ts Lines 91 to 96 in 3823937
|
||||||||||||||||
| stabilizationTimeoutSeconds: argv.hotswapEcsStabilizationTimeoutSeconds, | ||||||||||||||||
| }, | ||||||||||||||||
| }, | ||||||||||||||||
| unstable: argv.unstable, | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided not to do the
hotswap-ecs-stabilization-timeout=10soption now because:-secondssuffix.cdk.json- so no excessive typing needed.