feat(autoscaling): Auto Scaling Group with Launch Template by Martin1994 · Pull Request #19066 · aws/aws-cdk

Martin1994 · 2022-02-21T13:12:19Z

Add support to launch an Auto Scaling Group from a Launch Template rather than a launch configuration.

Closes #6734.

High level designs:

The current AutoScalingGroup L2 construct, unfortunately, is deeply coupled with LaunchConfiguration. Therefore adding LaunchTemplate to the existing construct is not trivial.

There are two different approaches to support LaunchTemplate:

Implement another brand new L2 construct with IAutoScalingGroup interface. The shared logic between the old and the new one such as adding scaling policies can be extracted to a common parent class.
Add LaunchTemplate related interface to the existing L2 construct with minimum breaking changes.

Adding a new construct is always guaranteed to be a clean solution code-wise, but it can also cause confusion from the end user about scattered CDK implementation on the same CFN resource, and on the other hand breaks the existing libraries consuming the existing AutoScalingGroup construct rather than the IAutoScalingGroup interface.

Adding LT to the existing construct, as what this PR does, however may change the API behaviour when LT is used. For example, the implementation in this PR turns AutoScalingGroup.connections from a public field to a public getter, and will throw an error when the ASG is created from a LT reference, which means existing libraries taking an AutoScalingGroup instance as an IConnectable will get runtime error in this case. Existing code (i.e. ASG created from the L2 construct with a LC rather than a LT) won't break with this change.

This PR picks the latter approach since I believe it provides a better experience overall, mainly because of its continuity.

BREAKING CHANGE: Properties relying on a launch configuration are now either optional or turned into throwable getters

autoscaling: AutoScalingGroupProps.instanceType is now optional
autoscaling: AutoScalingGroupProps.machineImage is now optional
autoscaling: AutoScalingGroup.connections is now a throwable getter
autoscaling: AutoScalingGroup.role is now a throwable getter
autoscaling: AutoScalingGroup.userData is now a throwable getter

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

gitpod-io · 2022-02-21T13:12:22Z

Add support to launch an Auto Scaling Group from a Launch Template rather than a launch configuration. Closes aws#6734.

fiftysquid · 2022-03-22T14:09:03Z

Actually being able to use Launch Templates would be neat.

jungseoklee · 2022-03-23T20:28:12Z

The approach looks reasonable. As mentioned, this won't break existing LC users, which is what I like.

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

jungseoklee · 2022-03-23T20:26:04Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+ *
+ * https://docs.aws.amazon.com/autoscaling/ec2/userguide/asg-purchase-options.html
+ */
+export interface MixedInstancesPolicy {


I am wondering if it is better to have a separate file for mixed instance policy because this file is getting bigger.

jungseoklee · 2022-03-23T20:26:55Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

    }));
  }
+
+  private getLaunchSettings(launchConfig?: CfnLaunchConfiguration, launchTemplate?: ec2.ILaunchTemplate, mixedInstancesPolicy?: MixedInstancesPolicy)


I am wondering if it is better to have a separate file for launch template and launch configuration because this file is getting bigger.

I am not sure how practical it is since this design puts LC and LT under one single class, and I don't think we can break a class definition into multiple files like partial class in C#.

jungseoklee · 2022-03-23T20:33:50Z

Can we get attention from maintainers? I see more than 100 thumbs up for this feature, so it would be great if we can start reviewing this PR.

comcalvi

Good first attempt! We're a few iterations from merging this in, but this is looking great so far.

packages/@aws-cdk/aws-autoscaling/README.md

comcalvi · 2022-03-29T11:47:58Z

packages/@aws-cdk/aws-autoscaling/README.md

+declare const lt1: ec2.LaunchTemplate;
+declare const lt2: ec2.LaunchTemplate;


can we call these launchTemplate1 and launchTemplate2?

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

comcalvi · 2022-03-29T20:32:25Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

  /**
   * Type of instance to launch
+   *
+   * Launch template must not be specified when this property is specified.


should be launchTemplate must not be specified when this property is specified.

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

comcalvi · 2022-03-30T02:10:40Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+  /**
+   * UserData for the instances
+   */


Can we improve this comment to explain what the UserData does and how it is used?

I can borrow some words from the public doc. However more importantly I think it's better to document the fact that this getter can throw error. Same as the role getter.

aws-cdk/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

Lines 928 to 931 in 77a5fa1

/**

* UserData for the instances

*/

public readonly userData: ec2.UserData;

comcalvi · 2022-03-30T02:13:38Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+  /**
+   * Role for the instances
+   */


Can we update this comment to explain what this Role is used for? eg if it's an execution role, that should be mentioned here (also, this variable probably needs a better name than role; perhaps executionRole or instanceExecutionRole, if that's what it represents).

I cannot change the getter name here (same as userData) because it was a class property with the same name. role and userData are turned into getters so that they can throw exceptions when a Launch Template reference is given.

aws-cdk/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

Lines 908 to 911 in 77a5fa1

/**

* The IAM role assumed by instances of this fleet.

*/

public readonly role: iam.IRole;

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

Pull request has been modified.

Martin1994 · 2022-04-08T07:21:11Z

Any update?

comcalvi

This is looking better! In general, when a PR is ready for review, please re-request the review on github

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

comcalvi · 2022-04-11T19:19:13Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+      if (props.mixedInstancesPolicy && props.mixedInstancesPolicy.launchTemplate instanceof ec2.LaunchTemplate) {
+        this.launchTemplate = props.mixedInstancesPolicy.launchTemplate;
+      }


Can we extract this to a local variable?

comcalvi · 2022-04-11T20:17:56Z

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

+      if (props.launchTemplate && props.launchTemplate instanceof ec2.LaunchTemplate) {
+        this.launchTemplate = props.launchTemplate;
+      }

-    launchConfig.node.addDependency(this.role);
+      if (props.mixedInstancesPolicy && props.mixedInstancesPolicy.launchTemplate instanceof ec2.LaunchTemplate) {
+        this.launchTemplate = props.mixedInstancesPolicy.launchTemplate;
+      }


this code:

vpc: new ec2.Vpc(this, 'template-vpc'), mixedInstancesPolicy: { launchTemplate: new ec2.LaunchTemplate(this, 'template-mixed', { instanceType: new ec2.InstanceType('a1.2xlarge') }) } });

Results in a deploy time error:

You must use a valid fully-formed launch template. The request must contain the parameter ImageId (Service: AmazonAutoScaling; Status Code: 400; Error Code: ValidationError;

This error only happens if the launch template is specified here as part of an autoscaling group, and does not happen when a launch template is specified on its own; thus, we should ensure the user has passed an imageId here, and throw if they have not.

packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

comcalvi · 2022-04-11T20:29:29Z

packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts

+
+    expect(() => {
+      asg.connections;
+    }).toThrow();


can we validate that the error thrown here at least partially matches the error message?

packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts

Pull request has been modified.

comcalvi

This is pretty much perfect, just one small comment.

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

Pull request has been modified.

robertd · 2022-04-13T00:09:09Z

Thanks for all the hard work you’ve put in on this PR @Martin1994 🙌

Martin1994 · 2022-04-13T00:33:58Z

Just to confirm, GitHub can rebase, squash and merge this PR right?

robertd · 2022-04-13T00:39:25Z

@Martin1994 Once @comcalvi approves this PR GitHub Actions will do all the grunt work.

comcalvi

Awesome work @Martin1994!

mergify · 2022-04-13T16:56:38Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

robertd · 2022-04-13T16:57:19Z

Very excited for this PR!!! Thanks everyone. 🙏

jungseoklee · 2022-04-13T17:07:12Z

Very excited. Great thanks, everyone!

Pull request has been modified.

robertd · 2022-04-13T20:23:59Z

Thanks for stepping in @comcalvi 🙌

aws-cdk-automation · 2022-04-13T21:05:48Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: ee7860c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-04-13T21:06:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

badfun · 2022-04-14T16:06:49Z

@Martin1994 hurray! Thanks for this. Looking forward to refactoring. 🙌

Add support to launch an Auto Scaling Group from a Launch Template rather than a launch configuration. Closes aws#6734. ---- High level designs: The current AutoScalingGroup L2 construct, unfortunately, is deeply coupled with LaunchConfiguration. Therefore adding LaunchTemplate to the existing construct is not trivial. There are two different approaches to support LaunchTemplate: 1. Implement another brand new L2 construct with `IAutoScalingGroup` interface. The shared logic between the old and the new one such as adding scaling policies can be extracted to a common parent class. 2. Add LaunchTemplate related interface to the existing L2 construct with minimum breaking changes. Adding a new construct is always guaranteed to be a clean solution code-wise, but it can also cause confusion from the end user about scattered CDK implementation on the same CFN resource, and on the other hand breaks the existing libraries consuming the existing `AutoScalingGroup` construct rather than the `IAutoScalingGroup` interface. Adding LT to the existing construct, as what this PR does, however may change the API behaviour when LT is used. For example, the implementation in this PR turns `AutoScalingGroup.connections` from a public field to a public getter, and will throw an error when the ASG is created from a LT reference, which means existing libraries taking an `AutoScalingGroup` instance as an `IConnectable` will get runtime error in this case. Existing code (i.e. ASG created from the L2 construct with a LC rather than a LT) won't break with this change. This PR picks the latter approach since I believe it provides a better experience overall, mainly because of its continuity. ---- BREAKING CHANGE: Properties relying on a launch configuration are now either optional or turned into throwable getters * **autoscaling:** `AutoScalingGroupProps.instanceType` is now optional * **autoscaling:** `AutoScalingGroupProps.machineImage` is now optional * **autoscaling:** `AutoScalingGroup.connections` is now a throwable getter * **autoscaling:** `AutoScalingGroup.role` is now a throwable getter * **autoscaling:** `AutoScalingGroup.userData` is now a throwable getter ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling label Feb 21, 2022

github-actions bot assigned comcalvi Feb 21, 2022

Martin1994 mentioned this pull request Feb 23, 2022

AutoScaling: Support a external LaunchConfiguration and LaunchTemplate to AutoScalingGroup #1403

Closed

rix0rrr added feature-request A feature should be added or improved. p1 and removed p1 feature-request A feature should be added or improved. @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling labels Mar 4, 2022

Martin1994 added 5 commits March 8, 2022 08:57

feat(autoscaling): Auto Scaling Group with Launch Template

5b0b033

Add support to launch an Auto Scaling Group from a Launch Template rather than a launch configuration. Closes aws#6734.

Fixes self reference of userData getter

cb51876

Fix unit test failures

2f5b51a

Remove @example from a pure text example

9f80bc2

Add instruction to reate an ASG from an LT in README

428f557

Martin1994 force-pushed the asg-lt-pr branch from 33533a8 to 428f557 Compare March 8, 2022 16:57

jungseoklee reviewed Mar 23, 2022

View reviewed changes

comcalvi previously requested changes Mar 30, 2022

View reviewed changes

jungseoklee reviewed Mar 30, 2022

View reviewed changes

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts Outdated Show resolved Hide resolved

Address PR comments

cdad736

Handle empty instancesDistribution

a9f21ae

comcalvi previously requested changes Apr 11, 2022

View reviewed changes

Address PR comments

192aa3c

Martin1994 requested a review from comcalvi April 12, 2022 03:42

comcalvi previously requested changes Apr 12, 2022

View reviewed changes

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts Outdated Show resolved Hide resolved

minor style improvement

d98b27c

Martin1994 requested a review from comcalvi April 13, 2022 00:54

comcalvi previously approved these changes Apr 13, 2022

View reviewed changes

Merge branch 'master' into asg-lt-pr

763fcad

added snapshot directory to match new integ test behavior

ee7860c

comcalvi approved these changes Apr 13, 2022

View reviewed changes

mergify bot merged commit 1581af0 into aws:master Apr 13, 2022

tomoki10 mentioned this pull request Aug 7, 2022

EC2 Launch configurations is deprecated aws-samples/baseline-environment-on-aws#60

Closed

		declare const lt1: ec2.LaunchTemplate;
		declare const lt2: ec2.LaunchTemplate;

	/**
	* UserData for the instances
	*/
	public readonly userData: ec2.UserData;

	/**
	* The IAM role assumed by instances of this fleet.
	*/
	public readonly role: iam.IRole;

Conversation

Martin1994 commented Feb 21, 2022

Uh oh!

gitpod-io bot commented Feb 21, 2022

Uh oh!

fiftysquid commented Mar 22, 2022

Uh oh!

jungseoklee commented Mar 23, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jungseoklee commented Mar 23, 2022

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Martin1994 Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Martin1994 Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Martin1994 commented Apr 8, 2022

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertd commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Martin1994 commented Apr 13, 2022

Uh oh!

robertd commented Apr 13, 2022

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Apr 13, 2022

Uh oh!

robertd commented Apr 13, 2022

Martin1994 Mar 31, 2022 •

edited

Loading

Martin1994 Mar 31, 2022 •

edited

Loading

robertd commented Apr 13, 2022 •

edited

Loading