feat(ec2): add CapacityReservationSpecification support to Launch…#35453
feat(ec2): add CapacityReservationSpecification support to Launch…#35453
Conversation
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
pahud
left a comment
There was a problem hiding this comment.
@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
pahud
left a comment
There was a problem hiding this comment.
LGTM. Requesting the team for another review.
kaizencc
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 referenceCapacityReservationwith static factory methods:fromId(capacityReservationId)- reference by Capacity Reservation IDfromResourceGroupArn(resourceGroupArn)- reference by resource group ARNCapacityReservationPreferencewithOPEN,NONE, andCAPACITY_RESERVATIONS_ONLYvaluesFlat Property Structure (following CDK Design Guidelines):
capacityReservation?: ICapacityReservation- direct property on LaunchTemplatePropscapacityReservationPreference?: CapacityReservationPreference- direct property on LaunchTemplatePropsKey Features:
AWS::EC2::LaunchTemplate.CapacityReservationSpecificationExample 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