-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(ec2): add CapacityReservationSpecification support to Launch… #35453
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
base: main
Are you sure you want to change the base?
feat(ec2): add CapacityReservationSpecification support to Launch… #35453
Conversation
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 review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
@aws-cdk-testing/framework-integ: UNCHANGED triggers/test/integ.triggers 1.349s
@aws-cdk-testing/framework-integ: Snapshot Results:
@aws-cdk-testing/framework-integ: Tests: 1 failed, 1265 total
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3595677729/src/actions-runner/_work/aws-cdk/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.launch-template-capacity-reservation.js
@aws-cdk-testing/framework-integ: Error: Some tests failed!
@aws-cdk-testing/framework-integ: To re-run failed tests run: integ-runner --update-on-failed
@aws-cdk-testing/framework-integ: at main (/codebuild/output/src3595677729/src/actions-runner/_work/aws-cdk/aws-cdk/node_modules/@aws-cdk/integ-runner/lib/index.js:10626:13)
@aws-cdk-testing/framework-integ: Error: integ-runner exited with error code 1
@aws-cdk-testing/framework-integ: Tests failed. Total time (2m59.8s) | integ-runner (2m13.9s) | /codebuild/output/src3595677729/src/actions-runner/_work/aws-cdk/aws-cdk/node_modules/jest/bin/jest.js (44.7s)
@aws-cdk-testing/framework-integ: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
@aws-cdk-testing/framework-integ:
@aws-cdk-testing/framework-integ: error Command failed with exit code 1.
let's re-run the integ tests
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.
LGTM. Requesting the team for another review.
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.
Thanks for the PR @snese. I think we're skirting around actually modeling new L2s and I'm confused why. That seems like the better approach to me that is more maintainable and will follow CDK conventions better.
One more thing: the title is cutoff and should not have the word add
.
|
||
You can specify [EC2 Capacity Reservations](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-capacity-reservations.html) for your launch template to ensure that EC2 capacity is reserved for your instances: | ||
|
||
```ts fixture=with-vpc |
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.
nit: why do we need fixture=with-vpc?
* The ARN of the Capacity Reservation resource group in which to run the instance | ||
* @default - No Capacity Reservation resource group | ||
*/ | ||
readonly capacityReservationResourceGroupArn?: string; |
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.
we rarely model things as Arn
in CDK. This represents a place where we can uplift the existing CFN spec and modle capacityReservationResourceGroup
directly.
The issue is that if we model capacityReservationResourceGroup
in the future, then it wouldn't match the normal CDK pattern of supplying a IInterface
to connect constructs
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.
after doing a ton of research it looks like CFN does not even have a capacityReservationResourceGroup
. But even then, we need to model this as an L2 with the static function fromResourceGroupArn
so that we still don't model this as a raw arn.
// Fields not yet implemented: | ||
// ========================== | ||
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-launchtemplatedata-capacityreservationspecification.html | ||
// Will require creating an L2 for AWS::EC2::CapacityReservation |
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 comment sums it up: to support this we need to create an L2 for AWS::ECR::CapacityReservation
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.
you've modeled all the individual components of AWS::ECR::CapacityReservation
why not just create it as an L2 also?
then the property becomes
readonly capacityReservation?: ICapacityReservation;
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.
why .lit.ts
? this is a legacy way of providing examples in the readme.
@@ -0,0 +1,63 @@ | |||
const cdk = require('aws-cdk-lib'); |
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 a TypeScript file, not a .js
file.
capacityReservationSpecification: { | ||
capacityReservationPreference: ec2.CapacityReservationPreference.OPEN, | ||
capacityReservationTarget: { | ||
capacityReservationId: 'cr-1234567890abcdef0', |
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'm going code blind from all the times that the words capacityReservation
shows up here. Why is this not:
capacityReservation: CapacityReservation.fromId(this, 'Reservation', 'cr-1234567890abcdef0'),
capacityReservationPreference: ec2.CapacityReservationPreference.OPEN,
* Capacity Reservation specification | ||
* @default - No capacity reservation specification | ||
*/ | ||
readonly capacityReservationSpecification?: LaunchTemplateCapacityReservationSpecification; |
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 goes against our design guidelines:
Do not introduce artificial nesting for props. It hinders discoverability and makes it cumbersome to use in some languages (like Java) [awslint:props-flat].
https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#flat
* Information about the target Capacity Reservation or Capacity Reservation group | ||
* @default - No target specified | ||
*/ | ||
readonly capacityReservationTarget?: LaunchTemplateCapacityReservationTarget; |
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.
Shouldn't target
be required if you specify a capacity reservation?
I can now specify:
capacityReservationSpecification: { }
What does that mean?
export interface LaunchTemplateCapacityReservationSpecification { | ||
/** | ||
* Indicates the instance's Capacity Reservation preferences | ||
* @default - No preference 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.
Surely there is an implied default here. Tell us what the default is.
Describe the default value or default behavior, even if it's not CDK that controls the default. For example, if an absent value does not get rendered into the template and it's ultimately the AWS service that determines the default behavior, we still describe it in our documentation.
https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#defaults
"No XXX specified" is not useful to say. Here's what you're telling a user:
"If you don't specify a capacityReservationPreference, then no capacityReservationPreference is specified."
I think we can trust our customers to figure that out for themselves. What they want to know is what happens when they don't specify a certain thing.
12659f7
to
8132748
Compare
Pull request has been modified.
…Template construct - Add LaunchTemplateCapacityReservationSpecification and LaunchTemplateCapacityReservationTarget interfaces - Add CapacityReservationPreference enum with OPEN and NONE values - Add optional capacityReservationSpecification property to LaunchTemplateProps - Map property to existing CloudFormation L1 constructs - Add 7 comprehensive unit tests covering all scenarios - Maintain backward compatibility with optional property Closes aws#34921
…ReservationSpecification - Add capacity reservation documentation to aws-ec2 README with usage examples - Create integration test with snapshot for capacity reservation functionality - Fix PR title scope from aws-ec2 to ec2 per CDK automation requirements
…fication - Add integ.launch-template-capacity-reservation.lit.ts in main package - Follows existing pattern of .lit.ts integration tests - Tests both capacity reservation ID and resource group ARN scenarios
…late construct - Add LaunchTemplateCapacityReservationSpecification interface - Add LaunchTemplateCapacityReservationTarget interface - Add CapacityReservationPreference enum with OPEN, NONE, CAPACITY_RESERVATIONS_ONLY values - Add optional capacityReservationSpecification property to LaunchTemplateProps - Implement CloudFormation mapping to AWS::EC2::LaunchTemplate.CapacityReservationSpecification - Add comprehensive unit tests covering all property combinations - Add README documentation with usage examples - Maintain backward compatibility - property omitted when not specified Closes aws#34921
…ecification - Add comprehensive integration test for CapacityReservationSpecification - Test covers all capacity reservation preference options (open, none, capacity-reservations-only) - Test includes capacity reservation target with both ID and resource group ARN - Validates CloudFormation template generation for capacity reservation properties - Ensures backward compatibility when capacity reservation is not specified Addresses aws#34921
…at properties - Created ICapacityReservation interface and CapacityReservation class with static factory methods - Flattened property structure: capacityReservation and capacityReservationPreference as direct properties - Removed artificial nesting (capacityReservationSpecification) - Changed integration test from .js to .ts format - Improved documentation with actual default behavior descriptions - Removed unnecessary fixture=with-vpc from README examples Addresses feedback from @kaizencc and @rix0rrr: - Proper L2 construct modeling instead of raw interfaces - Follows CDK Design Guidelines (no artificial nesting) - Factory methods: fromId() and fromResourceGroupArn() - TypeScript integration test instead of JavaScript - Clear documentation of default behaviors
8132748
to
63a82f8
Compare
Reviewer Feedback AddressedI've updated the implementation to address all feedback from @kaizencc and @rix0rrr. The changes implement proper L2 constructs with flat properties following CDK Design Guidelines. Key Changes Made:
Example Usage (New API):// Reference by Capacity Reservation ID
new ec2.LaunchTemplate(this, 'Template', {
machineImage: ec2.MachineImage.latestAmazonLinux2023(),
capacityReservation: ec2.CapacityReservation.fromId('cr-1234567890abcdef0'),
capacityReservationPreference: ec2.CapacityReservationPreference.OPEN,
}); This is much cleaner than the previous nested structure and follows the exact pattern suggested by @rix0rrr. Test Results:
Ready for re-review! |
Issue #34921
Closes #34921.
Reason for this change
The CDK LaunchTemplate construct was missing the CapacityReservationSpecification parameter that exists in the CloudFormation specification. This prevented users from utilizing EC2 capacity reservations through CDK, causing job failures for instance types with poor on-demand availability (like p4d.24xlarge for GPU workloads). Users were forced to use workarounds with
addPropertyOverride()
that bypass CDK's type safety.Description of changes
Added comprehensive Capacity Reservation support to the LaunchTemplate construct following CDK L2 construct patterns and design guidelines:
L2 Construct Implementation:
ICapacityReservation
- represents a Capacity Reservation referenceCapacityReservation
with static factory methods:fromId(capacityReservationId)
- reference by Capacity Reservation IDfromResourceGroupArn(resourceGroupArn)
- reference by resource group ARNCapacityReservationPreference
withOPEN
,NONE
, andCAPACITY_RESERVATIONS_ONLY
valuesFlat Property Structure (following CDK Design Guidelines):
capacityReservation?: ICapacityReservation
- direct property on LaunchTemplatePropscapacityReservationPreference?: CapacityReservationPreference
- direct property on LaunchTemplatePropsKey Features:
AWS::EC2::LaunchTemplate.CapacityReservationSpecification
Example Usage:
Describe any new or updated permissions being added
N/A - This change only adds configuration properties that map to existing CloudFormation functionality. No new IAM permissions are required.
Description of how you validated changes
Reviewer Feedback Addressed
This implementation addresses all feedback from @kaizencc and @rix0rrr:
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license