Skip to content
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

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

Closed
cohalz opened this issue Dec 19, 2018 · 10 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs feature-request A feature should be added or improved. p1

Comments

@cohalz
Copy link
Contributor

cohalz commented Dec 19, 2018

I want to use custom LaunchConfiguration in AutoScalingGroup library.
For example, when to use a spot instance.

@cohalz cohalz changed the title AutoScaling: Support custom LaunchConfiguration and LaunchTemplate to AutoScalingGroup AutoScaling: Support a external LaunchConfiguration and LaunchTemplate to AutoScalingGroup Dec 19, 2018
@eladb eladb added @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud enhancement labels Dec 19, 2018
@srchase srchase added feature-request A feature should be added or improved. and removed enhancement labels Jan 3, 2019
@rix0rrr rix0rrr added gap and removed gap labels Jan 4, 2019
@SomayaB
Copy link
Contributor

SomayaB commented Sep 26, 2019

Closing this issue since there hasn't been activity on it or on the related PR in a while. Feel free to reopen.

@SomayaB SomayaB closed this as completed Sep 26, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 18, 2020

I think the FR is still valid.

@rix0rrr rix0rrr reopened this Feb 18, 2020
@eschulma
Copy link

I would like to use a launch template rather than machine image.

@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Feb 21, 2020
@SomayaB SomayaB removed the gap label Feb 25, 2020
@ccfife
Copy link
Contributor

ccfife commented Feb 25, 2020

The current AutoScalingGroup L2 construct only supports Spot Fleet. We have a customer who needs a generic EC2 Fleet (i.e. Launch Template) that is managed by an ASG; this will allow them to run certain components primarily on Spot, but fall-back to on-demand when there is no Spot capacity available.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 25, 2020

Previous comments of mine on this topic:


So, this PR brings to the forefront an interesting dilemma: how should we do the LaunchTemplate/Configuration going forward?

We are currently in the situation that the launch properties are inlined into the props of the AutoScalingGroup. This is great for usability, but not so much for future extension, and it also doesn't allow using an existing or a custom LaunchTemplate.

An alternative is to ONLY take a launchTemplate: ILaunchTemplate as an input and let the user sort it out. Bad for usability (imo) but more true to the natural model that EC2/ASG forces on us.

Another alternative is to BOTH accept all configuration options (or at least the most common ones) as well as a launchTemplate?: ILaunchTemplate (and you can't specify config options when supplying an existing launch template).

Another another alternative is to use an enum-like pattern, where we can do something like:

launchInfo: LaunchInfo.simple({ 
  instanceType: new InstanceType(...)
}),
launchInfo: LaunchInfo.withMixedInstancePolicy({ ... }),
launchInfo: LaunchInfo.fromLaunchTemplate(myExistingLaunchTemplate),

(Name LaunchInfo tbd, I'd prefer for it to be called LaunchConfig but I'm 99% sure LaunchConfig.fromLaunchTemplate() is going to confuse people who know too much about the underlying details).

Unsure if the "enum-like" pattern isn't going to end up biting itself too. We could also use the enum-like pattern for different subsets of launch properties, one enum-like for instance types, another enum-like for userdata, etc.

Q: Can existing AutoScalingGroups using a LaunchConfig be transparently upgraded to using a LaunchTemplate? Or will this result in an error? (I suspect the latter)

Don't really know how to proceed, and soliciting other people's opinions as well. I'd like it if someone could make a table of LaunchConfig/LaunchTemplate features, show which ones apply to which, and which ones we already support in our current AutoScalingGroupProps, so we can make an informed decision on the deltas we'll introduce with each kind of change. If no one volunteers I will do one.


Feature LC LT AsgProps
Associate public address x (via ENI config) x
Block device mappings x x x
IAM Instance Profile (Role) x x x
Image ID x x x
Single Instance Type x x x
Multiple Instance Types (ASG overrides)
Key Name x x x
Security Groups x x x
Spot (single price) x (via singleton bid) x
Spot (multiple bids) x
UserData x x x
EBS Optimized x x
Classic Link VPC x
Instance Monitoring x x (default is yes)
Kernel ID x x
Placement tenancy lots of options
RAM disk id x x
Capacity Reservations x
CPU Options (reduce cores, disable HT) x
T2/T3 Credit Specification x
Termination/Hibernation Options x
GPU/TPU x
License Specification x
Configure metadata service x
Advanced ENI config x

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 25, 2020

Here's what I think the right approach is:

We'd still want to keep (some of) the parameters that we have on the LaunchTemplate on the AutoScalingGroup class directly, because it is very convenient in its use. We can add more features to the basic AutoScalingGroupProps, but we need to allow a launchTemplate?: ILaunchTemplate parameter as well because we won't be able to keep up with all the LT features.

We now have the option to combine them into the same props. That would look like this:

class AutoScalingGroupProps {
  /**
   * Mutually exclusive with launchTemplate
   */
  readonly instanceType?: ec2.InstanceType;
  readonly userData: ...;
  readonly associatePublicIpAddress: ...;
  readonly blockDeviceMappings: ...;
  readonly ...;

  /**
   * Mutually exclusive with instanceType, userData, associatePublicIpAddress, blockDeviceMappings, ...
   */
  readonly launchTemplate?: ec2.ILaunchTemplate;
}

In a just world, we would have had

  • AutoScalingGroup taking a LaunchTemplate argument.
  • SimpleAutoScalingGroup taking some important constructor parameters and impliclty creating the LaunchTemplate/LaunchConfig for you.

In the world we have, why don't we do the following:

  • AutoScalingGroup -- can't really change this without breaking changes. It may get features added to remove the need for people to reach for the...
  • AdvancedAutoScalingGroup -- will be the one that takes the LaunchTemplate, and allows full freedom.

Thoughts?

@ddneilson
Copy link
Contributor

We'd still want to keep (some of) the parameters that we have on the LaunchTemplate on the AutoScalingGroup class directly, because it is very convenient in its use. We can add more features to the basic AutoScalingGroupProps, but we need to allow a launchTemplate?: ILaunchTemplate parameter as well because we won't be able to keep up with all the LT features.

For what it's worth, I agree. I was looking at this last night. The LaunchTemplate is used in more than just the AutoScalingGroup; it's also used, in the same way, in the EC2::Instance, and EC2::Fleet. In all cases, the reference in those Cfn resources looks like:

{
  "LaunchTemplateId" : string,
  "LaunchTemplateName" : string,
  "Version" : string
}

where the Id and Name are mutually exclusive.

Seems to me that it makes the most sense to create a LaunchTemplate L2, and pass the ILaunchTemplate to each of the ec2.Instance, ec2.Fleet, and asg.AdvancedAutoScalingGroup L2's. Include some sort of helper magic to convert the ILaunchTemplate into the form used in the instance, fleet, and ASG L1's and you're golden.

This lets you:

  1. Keep all of the LaunchTemplate stuff in one place, instead of three, to reduce the maintenance burden;
  2. Create a LaunchTemplate import mechanism as exists for other L2's; and
  3. Simply expose the ILaunchTemplate in the props for the relevant constructs.

It seems like a clear win, to me, over trying to work with a hodge-podge of properties in the Instance, Fleet, and ASG L2's to materialize a LaunchTemplate behind the scenes. The user experience doesn't feel right down that path, especially when you factor in that the three constructs likely will not be kept at the same level of LaunchTemplate support.

@NetaNir NetaNir self-assigned this May 1, 2020
@rix0rrr rix0rrr added the p1 label Aug 12, 2020
@ericzbeard ericzbeard added the feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs label Apr 7, 2021
@Martin1994
Copy link
Contributor

#19066

@madeline-k
Copy link
Contributor

Resolved by #19066

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature/coverage-gap Gaps in CloudFormation coverage by L2 constructs feature-request A feature should be added or improved. p1
Projects
None yet