-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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(autoscaling): Auto Scaling Group with Launch Template #19066
Conversation
Add support to launch an Auto Scaling Group from a Launch Template rather than a launch configuration. Closes aws#6734.
Actually being able to use Launch Templates would be neat. |
The approach looks reasonable. As mentioned, this won't break existing LC users, which is what I like. |
* | ||
* https://docs.aws.amazon.com/autoscaling/ec2/userguide/asg-purchase-options.html | ||
*/ | ||
export interface MixedInstancesPolicy { |
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.
I am wondering if it is better to have a separate file for mixed instance policy because this file is getting bigger.
@@ -1315,6 +1625,74 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements | |||
metrics: group._metrics?.size !== 0 ? [...group._metrics].map(m => m.name) : undefined, | |||
})); | |||
} | |||
|
|||
private getLaunchSettings(launchConfig?: CfnLaunchConfiguration, launchTemplate?: ec2.ILaunchTemplate, mixedInstancesPolicy?: MixedInstancesPolicy) |
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.
I am wondering if it is better to have a separate file for launch template and launch configuration because this file is getting bigger.
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.
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#.
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. |
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.
Good first attempt! We're a few iterations from merging this in, but this is looking great so far.
declare const lt1: ec2.LaunchTemplate; | ||
declare const lt2: ec2.LaunchTemplate; |
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.
can we call these launchTemplate1
and launchTemplate2
?
/** | ||
* Type of instance to launch | ||
* | ||
* Launch template must not be specified when this property is specified. |
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.
should be launchTemplate
must not be specified when this property is specified.
/** | ||
* UserData for the instances | ||
*/ |
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.
Can we improve this comment to explain what the UserData
does and how it is used?
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.
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; |
/** | ||
* Role for the instances | ||
*/ |
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.
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).
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.
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; |
Any update? |
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.
This is looking better! In general, when a PR is ready for review, please re-request the review on github
if (props.mixedInstancesPolicy && props.mixedInstancesPolicy.launchTemplate instanceof ec2.LaunchTemplate) { | ||
this.launchTemplate = props.mixedInstancesPolicy.launchTemplate; | ||
} |
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.
Can we extract this to a local variable?
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; | ||
} |
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.
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
Outdated
Show resolved
Hide resolved
|
||
expect(() => { | ||
asg.connections; | ||
}).toThrow(); |
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.
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
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
Outdated
Show resolved
Hide resolved
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.
This is pretty much perfect, just one small comment.
Thanks for all the hard work you’ve put in on this PR @Martin1994 🙌 |
Just to confirm, GitHub can rebase, squash and merge this PR right? |
@Martin1994 Once @comcalvi approves this PR GitHub Actions will do all the grunt work. |
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.
Awesome work @Martin1994!
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). |
Very excited for this PR!!! Thanks everyone. 🙏 |
Very excited. Great thanks, everyone! |
Thanks for stepping in @comcalvi 🙌 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
@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*
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:
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.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 theIAutoScalingGroup
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 anAutoScalingGroup
instance as anIConnectable
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
AutoScalingGroupProps.instanceType
is now optionalAutoScalingGroupProps.machineImage
is now optionalAutoScalingGroup.connections
is now a throwable getterAutoScalingGroup.role
is now a throwable getterAutoScalingGroup.userData
is now a throwable getterBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license