fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout #31446

michelle-wangg · 2024-09-13T22:11:44Z

Issue # (if applicable)

Reason for this change

When specifiying both resourceSignalTimeout and initOptions.timeout in the options for creating an EC2 Instance, only the value from resourceSignalTimeout is used.

Description of changes

If both initOptions.timeout and resourceSignalTimeout are set, timeout consisting of the sum of the values and a warning suggesting that only one field should be specified
Else, timeout is set to field specified, else defaults to 5 min

Description of how you validated changes

Update integration test for instance-init.js with both fields and verify that warning is logged
Add unit tests:
- initOptions.timeout and resourceSignalTimeout are both not set, timeout is set to default of 5 min
- initOptions.timeout set and resourceSignalTimeout not set, timeout is set to initOptions.timeout
- initOptions.timeout not set and resourceSignalTimeout is set, timeout is set to resourceSignalTimeout
- initOptions.timeout and resourceSignalTimeout are both set, timeout is set to sum of timeouts and warning is logged

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. 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.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

GavinZZ

Nice Michelle! Left some comments.

I see your fix would now be summing the two values up. This would mean that users (who haven't changed their template) would have the values updated in their next deployment if they upgrade their CDK version. This is generally a sign of breaking change, however, in this case it's extending the timeout value. I think this can be argued to be a non-breaking change. Can you include @moelasmar in the review and get his though?

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

kaizencc · 2024-09-17T14:59:39Z

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

-          timeout: props.resourceSignalTimeout && props.resourceSignalTimeout.toIsoString(),
+          ...this.instance.cfnOptions.creationPolicy?.resourceSignal,
+          timeout: props.initOptions?.timeout ?
+            props.resourceSignalTimeout.plus(props.initOptions?.timeout).toIsoString() :


can you double check this for me: i don't think you need the ? on this line because typescript should be smart enough to know props.initOptions must exist to be on this side of the ternary operator.

Yes, you are right! I have updated this.

moelasmar · 2024-09-18T01:07:33Z

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

+            props.resourceSignalTimeout.plus(props.initOptions.timeout).toIsoString() :
+            props.resourceSignalTimeout.toIsoString(),


although it looks safe to add the 2 numbers, but this may cause a breaking change for customers if the sum of the 2 numbers is more than 12 hours .. this should be a very rare case, but may be some customers set one of these numbers to the maximum value, and since we ignore the other one, it was working for them, but now it will stop working.

Also, I can see that the original intent was to sum these 2 numbers together based on this code

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/instance.ts

Lines 686 to 694 in 4fa7716

private waitForResourceSignal(timeout: Duration) {

const oldResourceSignal = this.instance.cfnOptions.creationPolicy?.resourceSignal;

this.instance.cfnOptions.creationPolicy = {

...this.instance.cfnOptions.creationPolicy,

resourceSignal: {

count: (oldResourceSignal?.count ?? 0) + 1,

timeout: (oldResourceSignal?.timeout ? Duration.parse(oldResourceSignal?.timeout).plus(timeout) : timeout).toIsoString(),

},

};

That is why I think summing the 2 numbers is still Ok, but we should do it using a feature flag.

one more thing it is not needed to do this change here, it is enough to change this snippet

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/instance.ts

Lines 592 to 596 in 4fa7716

if (props.init) {

this.applyCloudFormationInit(props.init, props.initOptions);

}

this.applyUpdatePolicies(props);

to move this.applyUpdatePolicies(props); before the if condition

I think you're right that it may cause breaking changes when the sum is over the max threshold. Thanks for pointing that out.

For the second point, I disagree that we put it inside if statement because users could choose not to specify props.init and still setup the timeout value. Now if we move it inside, it won't happen anymore.

i did not mean to put it inside the if condition, what I recommend is something like this

this.applyUpdatePolicies(props); if (props.init) { this.applyCloudFormationInit(props.init, props.initOptions); }

and to remove the addition changes that @michelle-wangg added to the applyUpdatePolicies method and depend that the addition logic is already exist in applyCloudFormationInit method some how:

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/instance.ts

Lines 661 to 695 in 4fa7716

public applyCloudFormationInit(init: CloudFormationInit, options: ApplyCloudFormationInitOptions = {}) {

init.attach(this.instance, {

platform: this.osType,

instanceRole: this.role,

userData: this.userData,

configSets: options.configSets,

embedFingerprint: options.embedFingerprint,

printLog: options.printLog,

ignoreFailures: options.ignoreFailures,

includeRole: options.includeRole,

includeUrl: options.includeUrl,

});

this.waitForResourceSignal(options.timeout ?? Duration.minutes(5));

}

/**

* Wait for a single additional resource signal

*

* Add 1 to the current ResourceSignal Count and add the given timeout to the current timeout.

*

* Use this to pause the CloudFormation deployment to wait for the instances

* in the AutoScalingGroup to report successful startup during

* creation and updates. The UserData script needs to invoke `cfn-signal`

* with a success or failure code after it is done setting up the instance.

*/

private waitForResourceSignal(timeout: Duration) {

const oldResourceSignal = this.instance.cfnOptions.creationPolicy?.resourceSignal;

this.instance.cfnOptions.creationPolicy = {

...this.instance.cfnOptions.creationPolicy,

resourceSignal: {

count: (oldResourceSignal?.count ?? 0) + 1,

timeout: (oldResourceSignal?.timeout ? Duration.parse(oldResourceSignal?.timeout).plus(timeout) : timeout).toIsoString(),

},

};

}

See the login in waitForResourceSignal, by this change we do not need to duplicate the addition logic in 2 places.

For the warning message, I also prefer to move it to the constructor after the validations.

Sounds good! I have updated the implementation to use a feature flag. Since I have not changed the logic waitForResourceSignal, even if initOptions.Timeout is not specified and resourceSignalTimeout is specified, the timeout will add the default 5 min to the resourceSignalTimeout.

aws-cdk-automation · 2024-09-19T22:59:37Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6024e98
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

moelasmar · 2024-09-20T00:34:48Z

packages/aws-cdk-lib/aws-ec2/lib/instance.ts

@@ -589,11 +590,23 @@ export class Instance extends Resource implements IInstance {
    this.instancePublicDnsName = this.instance.attrPublicDnsName;
    this.instancePublicIp = this.instance.attrPublicIp;

-    if (props.init) {
-      this.applyCloudFormationInit(props.init, props.initOptions);
+    if (props.initOptions?.timeout && props.resourceSignalTimeout) {


I believe if the feature flag is enabled, so no need for this warning, as this is our recommended behaviour.

moelasmar · 2024-09-20T00:44:08Z

...g/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/integ-init.template.json

@@ -581,8 +581,7 @@
   ],
   "CreationPolicy": {
    "ResourceSignal": {
-     "Count": 1,


I believe Count should stay in the template

moelasmar

left minor comments, and you need to rerun the integration test cases with --update-on-failed flag

aws-cdk-automation requested a review from a team September 13, 2024 22:11

github-actions bot added the p2 label Sep 13, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 13, 2024

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort labels Sep 13, 2024

aws-cdk-automation previously requested changes Sep 13, 2024

View reviewed changes

michelle-wangg changed the title ~~fix(ec2): Instance resourceSignalTimeout overwrites initOptions.timeout~~ fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout Sep 16, 2024

GavinZZ reviewed Sep 16, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

michelle-wangg marked this pull request as ready for review September 16, 2024 21:03

michelle-wangg requested a review from GavinZZ September 16, 2024 21:06

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 16, 2024

GavinZZ requested changes Sep 16, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ec2/lib/instance.ts Outdated Show resolved Hide resolved

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 16, 2024

michelle-wangg requested review from moelasmar and GavinZZ September 16, 2024 22:24

kaizencc reviewed Sep 17, 2024

View reviewed changes

michelle-wangg requested a review from kaizencc September 17, 2024 22:48

moelasmar reviewed Sep 18, 2024

View reviewed changes

michelle-wangg force-pushed the overwriting-timeout branch from 289663d to 1cda59c Compare September 19, 2024 20:40

Michelle Wang added 10 commits September 19, 2024 15:01

fix(ec2): Instance resourceSignalTimeout overwrites initOptions.timeout

fa2935b

update integration test and snapshots

90f462c

update tests

82a485e

fix build

be80c08

update

0345d00

fixes

c87894d

move warning and update docs

a376317

update

60948af

update to use feature flag

26b95e3

feature flags

03b346c

Michelle Wang added 3 commits September 19, 2024 15:03

add integration test

25b1641

integration test

76f4383

update comments

40dbdd2

michelle-wangg force-pushed the overwriting-timeout branch from 563f7fe to 40dbdd2 Compare September 19, 2024 22:03

Michelle Wang added 2 commits September 19, 2024 15:26

update feature flag naming

ba460f2

modify integration test

6024e98

michelle-wangg requested a review from moelasmar September 19, 2024 23:08

moelasmar reviewed Sep 20, 2024

View reviewed changes

moelasmar requested changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout #31446

fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout #31446

michelle-wangg commented Sep 13, 2024 •

edited

Loading

aws-cdk-automation left a comment

GavinZZ left a comment •

edited

Loading

kaizencc Sep 17, 2024

michelle-wangg Sep 17, 2024

moelasmar Sep 18, 2024 •

edited

Loading

moelasmar Sep 18, 2024

GavinZZ Sep 18, 2024

moelasmar Sep 18, 2024

michelle-wangg Sep 19, 2024

aws-cdk-automation commented Sep 19, 2024

moelasmar Sep 20, 2024

moelasmar Sep 20, 2024

moelasmar left a comment

		props.resourceSignalTimeout.plus(props.initOptions.timeout).toIsoString() :
		props.resourceSignalTimeout.toIsoString(),

	private waitForResourceSignal(timeout: Duration) {
	const oldResourceSignal = this.instance.cfnOptions.creationPolicy?.resourceSignal;
	this.instance.cfnOptions.creationPolicy = {
	...this.instance.cfnOptions.creationPolicy,
	resourceSignal: {
	count: (oldResourceSignal?.count ?? 0) + 1,
	timeout: (oldResourceSignal?.timeout ? Duration.parse(oldResourceSignal?.timeout).plus(timeout) : timeout).toIsoString(),
	},
	};

	if (props.init) {
	this.applyCloudFormationInit(props.init, props.initOptions);
	}

	this.applyUpdatePolicies(props);

	public applyCloudFormationInit(init: CloudFormationInit, options: ApplyCloudFormationInitOptions = {}) {
	init.attach(this.instance, {
	platform: this.osType,
	instanceRole: this.role,
	userData: this.userData,
	configSets: options.configSets,
	embedFingerprint: options.embedFingerprint,
	printLog: options.printLog,
	ignoreFailures: options.ignoreFailures,
	includeRole: options.includeRole,
	includeUrl: options.includeUrl,
	});
	this.waitForResourceSignal(options.timeout ?? Duration.minutes(5));
	}

	/**
	* Wait for a single additional resource signal
	*
	* Add 1 to the current ResourceSignal Count and add the given timeout to the current timeout.
	*
	* Use this to pause the CloudFormation deployment to wait for the instances
	* in the AutoScalingGroup to report successful startup during
	* creation and updates. The UserData script needs to invoke `cfn-signal`
	* with a success or failure code after it is done setting up the instance.
	*/
	private waitForResourceSignal(timeout: Duration) {
	const oldResourceSignal = this.instance.cfnOptions.creationPolicy?.resourceSignal;
	this.instance.cfnOptions.creationPolicy = {
	...this.instance.cfnOptions.creationPolicy,
	resourceSignal: {
	count: (oldResourceSignal?.count ?? 0) + 1,
	timeout: (oldResourceSignal?.timeout ? Duration.parse(oldResourceSignal?.timeout).plus(timeout) : timeout).toIsoString(),
	},
	};
	}

fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout #31446

Are you sure you want to change the base?

fix(ec2): instance resourceSignalTimeout overwrites initOptions.timeout #31446

Conversation

michelle-wangg commented Sep 13, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

GavinZZ left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 19, 2024

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar left a comment

Choose a reason for hiding this comment

michelle-wangg commented Sep 13, 2024 •

edited

Loading

GavinZZ left a comment •

edited

Loading

moelasmar Sep 18, 2024 •

edited

Loading