-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cli): support optimistic stabilization strategy(--exit-on-config-complete) #29536
Changes from 8 commits
a78b102
34afe01
e00c0fa
8dbc5c3
4426221
f721596
d9a4c13
5d488c0
0311661
86abf37
ad261c8
de245f0
30ee9b0
579dff4
caaf9f7
33fd364
6209ce5
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 |
---|---|---|
|
@@ -417,6 +417,36 @@ class LambdaHotswapStack extends cdk.Stack { | |
} | ||
} | ||
|
||
class DetailedStatusStack extends cdk.Stack{ | ||
constructor(parent, id, props) { | ||
super(parent, id, props); | ||
|
||
const vpc = new ec2.Vpc(this, 'Vpc', { | ||
natGateways: 0, | ||
maxAzs: 1, | ||
subnetConfiguration: [ | ||
{ | ||
cidrMask: 24, | ||
name: 'Public', | ||
subnetType: ec2.SubnetType.PUBLIC, | ||
}, | ||
], | ||
}); | ||
|
||
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. A high level question -- what if a resource fails deploying after CONFIGURATION_COMPLETE status is emitted? Do we handle that case? Is that a possible case? If it is a possible case, could we have an integration test for that? 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. Will discuss this with the CFN team. As this PR is to monitor
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. My thinking is, if that happens, we should fail with exit status 1. One idea, without much thinking about how this would work, is to poll the statuses of all the stacks once the final stack has entered CONFIG_COMPLETE. So then it's just that the final stack of the deployment is not getting the speedup... which I think is fine since you then get the speed up on all the intermediate stacks. I am considering if there is a case where a customer kicks off some sort of production thing as soon as the customer get a success signal from CDK deploy? If so, then we'd want to make sure that everything did successfully provision... unless CloudFormation handles that, somehow. I'm not too familiar with this new optimistic stabilization feature from CloudFormation. 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. We probably should figure out, let's say if we have 2 stacks to deploy with Stack2 depends on Stack1: Stack1 enters 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. My preconception (which could be wrong) is that stack 2 would fail if stack 1 eventually failed. But if the scenario you're describing is true, then that biases me in favor of including a warning about this not being for production, and that would be all we'd do for addressing the failure case. So, similar to how we advise users to not use hotswap in production, we'd advise users to not use this flag in production. What do you think of that? I personally feel satisfied with that. 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'll check with the CFN team:
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. Yes! I agree with @bergjaak to warn customer to not use the flag in production. This is what CFN official user guide mentions. Stack 2 will not fail automatically. unless CDK implement a pulling mechanism that read the state an fail the stack based on the stack 1 state. |
||
const instance = new ec2.CfnInstance(this, 'Instance', { | ||
instanceType: 't3.nano', | ||
imageId: new ec2.AmazonLinuxImage().getImage(this).imageId, | ||
subnetId: vpc.publicSubnets[0].subnetId, | ||
}); | ||
new ec2.Volume(this, 'Volume', { | ||
availabilityZone: instance.attrAvailabilityZone, | ||
removalPolicy: cdk.RemovalPolicy.DESTROY, | ||
size: cdk.Size.gibibytes(1), | ||
}); | ||
|
||
} | ||
} | ||
|
||
class EcsHotswapStack extends cdk.Stack { | ||
constructor(parent, id, props) { | ||
super(parent, id, props); | ||
|
@@ -647,6 +677,7 @@ switch (stackSet) { | |
new LambdaStack(app, `${stackPrefix}-lambda`); | ||
new LambdaHotswapStack(app, `${stackPrefix}-lambda-hotswap`); | ||
new EcsHotswapStack(app, `${stackPrefix}-ecs-hotswap`); | ||
new DetailedStatusStack(app, `${stackPrefix}-detailed-status`); | ||
new DockerStack(app, `${stackPrefix}-docker`); | ||
new DockerStackWithCustomFile(app, `${stackPrefix}-docker-with-custom-file`); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,7 +174,8 @@ async function parseCommandLineArguments(args: string[]) { | |
.option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true }) | ||
.option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' }) | ||
.option('asset-prebuild', { type: 'boolean', desc: 'Whether to build all assets before deploying the first stack (useful for failing Docker builds)', default: true }) | ||
.option('ignore-no-stacks', { type: 'boolean', desc: 'Whether to deploy if the app contains no stacks', default: false }), | ||
.option('ignore-no-stacks', { type: 'boolean', desc: 'Whether to deploy if the app contains no stacks', default: false }) | ||
.option('exit-on-config-complete', { type: 'boolean', desc: 'Whether to exit on configuration complete', default: false }), | ||
) | ||
.command('import [STACK]', 'Import existing resource(s) into the given STACK', (yargs: Argv) => yargs | ||
.option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true }) | ||
|
@@ -255,7 +256,8 @@ async function parseCommandLineArguments(args: string[]) { | |
.command('destroy [STACKS..]', 'Destroy the stack(s) named STACKS', (yargs: Argv) => yargs | ||
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. would it make sense to also add an option for 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. Correct me if I am wrong but AFAIK 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. Ah, I thinkk we're both right. It's an options to turn off the --hot-swap for This comment is just a suggestion, though. I would approve this PR with or without watch support, since that could be added in the future, if there was a demand for it |
||
.option('all', { type: 'boolean', default: false, desc: 'Destroy all available stacks' }) | ||
.option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only destroy requested stacks, don\'t include dependees' }) | ||
.option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' })) | ||
.option('force', { type: 'boolean', alias: 'f', desc: 'Do not ask for confirmation before destroying the stacks' }) | ||
.option('exit-on-config-complete', { type: 'boolean', desc: 'Whether to exit on configuration complete', default: false })) | ||
pahud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.command('diff [STACKS..]', 'Compares the specified stack with the deployed stack or a local template file, and returns with status 1 if any difference is found', (yargs: Argv) => yargs | ||
.option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only diff requested stacks, don\'t include dependencies' }) | ||
.option('context-lines', { type: 'number', desc: 'Number of context lines to include in arbitrary JSON diff rendering', default: 3, requiresArg: true }) | ||
|
@@ -604,6 +606,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n | |
assetParallelism: configuration.settings.get(['assetParallelism']), | ||
assetBuildTime: configuration.settings.get(['assetPrebuild']) ? AssetBuildTime.ALL_BEFORE_DEPLOY : AssetBuildTime.JUST_IN_TIME, | ||
ignoreNoStacks: args.ignoreNoStacks, | ||
exitOnConfigComplete: args.exitOnConfigComplete, | ||
pahud marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
case 'import': | ||
|
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.
none of these resources currently cause the stack itself to emit the
CONFIGURATION_COMPLETE
event, so this test needs to be updated. The only resource I know of that does make it emit this event is ECS Services