feat(eks): support CUSTOM amiType #30660

Artemigos · 2024-06-25T12:32:50Z

Issue # (if applicable)

Reason for this change

Node groups with custom AMI are supported by CloudFormation, but not by CDK. This adds that support.

Description of changes

added CUSTOM to the NodegroupAmiType enum
always add CUSTOM to possibleAmiTypes when validating instanceTypes CPU architecture against amiType
verify that launchTemplateSpec is defined when using CUSTOM

Description of how you validated changes

Added unit tests checking valid deployment as well as failure when CUSTOM is used, but launchTemplateSpec isn't defined.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks 👍 Left some comments for minor adjustments

lpizzinidev · 2024-07-29T13:08:23Z

packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts

@@ -426,7 +430,7 @@ export class Nodegroup extends Resource implements INodegroup {
       * 1. instance types of different CPU architectures are not mixed(e.g. X86 with ARM).
       * 2. user-specified amiType should be included in `possibleAmiTypes`.
       */
-      possibleAmiTypes = getPossibleAmiTypes(instanceTypes);
+      possibleAmiTypes = getPossibleAmiTypes(instanceTypes).concat(NodegroupAmiType.CUSTOM);


Can you please concatenate inside the method for reusability?

lpizzinidev · 2024-07-29T13:11:40Z

packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts

@@ -442,6 +446,11 @@ export class Nodegroup extends Resource implements INodegroup {
      }
    }

+    // custom AMI type can be used only when there's a launch template that picks an AMI
+    if (props.amiType === NodegroupAmiType.CUSTOM && !props.launchTemplateSpec) {


Can you please update the comments here and here accordingly?

Added a little bit of clarification in both places.

lpizzinidev · 2024-07-29T13:14:17Z

packages/aws-cdk-lib/aws-eks/README.md

@@ -227,6 +227,27 @@ cluster.addNodegroupCapacity('custom-node-group', {
 });
 ```

+To use a custom AMI for the node group, you can set `amiType` to `eks.NodegroupAmiType.CUSTOM` and provide a launch template.


Suggested change

To use a custom AMI for the node group, you can set `amiType` to `eks.NodegroupAmiType.CUSTOM` and provide a launch template.

To use a custom AMI for the node group, you can set `amiType` to `CUSTOM`.

A launch template must be provided.

change applied

…combinations

aws-cdk-automation · 2024-08-12T12:41:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 432ead4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

lpizzinidev

Thanks 👍

GavinZZ

Thank you for making the changes. Left some comments and questions.

GavinZZ · 2024-08-30T00:15:37Z

packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts

@@ -651,5 +662,5 @@ function getPossibleAmiTypes(instanceTypes: InstanceType[]): NodegroupAmiType[]
    throw new Error('instanceTypes of different architectures is not allowed');
  }

-  return archAmiMap.get(Array.from(architectures)[0])!;
+  return archAmiMap.get(Array.from(architectures)[0])!.concat(NodegroupAmiType.CUSTOM);


Instead of concat here, should we just add it directly to the x8664AmiTypes, arm64AmiTypes, etc. Otherwise this line of code would be easily missed and maintainers and other contributors may not realize those amiTypes are not accurate and complete.

GavinZZ · 2024-08-30T00:16:04Z

packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts

+   * The AMI type for your node group. If you explicitly specify the launchTemplate with custom AMI, either set this property to `CUSTOM` or leave
+   * it undefined, otherwise the node group deployment will fail. In other cases, you will need to specify correct amiType for the nodegroup.


Just so for my own understanding, CUSTOM ami type is supported now if we leave it unset?

GavinZZ · 2024-08-30T00:16:40Z

packages/aws-cdk-lib/aws-eks/README.md

+To use a custom AMI for the node group, you can set `amiType` to `CUSTOM`.
+A launch template must be provided.


Related to the comment above, if custom AMI is supported now leaving the amiType property unset, we should call that out in the README too.

shikha372

marking it as requested changes based on previous feedback provided by @GavinZZ

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jun 25, 2024

aws-cdk-automation requested a review from a team June 25, 2024 12:33

aws-cdk-automation previously requested changes Jun 25, 2024

View reviewed changes

feat: add CUSTOM amiType

578ac1c

Artemigos force-pushed the eks-custom-ami branch from b8792a2 to 578ac1c Compare June 28, 2024 07:52

Artemigos and others added 2 commits June 28, 2024 10:08

Merge branch 'main' into eks-custom-ami

f152d5a

fix(docs): declare cluster in README code snippet

57e282f

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 28, 2024

This was referenced Jul 1, 2024

Monthly PR metrics report #30715

Closed

Weekly PR metrics report #30720

Closed

lpizzinidev suggested changes Jul 29, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 29, 2024

Artemigos added 2 commits August 12, 2024 14:07

chore: updated field docs about allowed launch template and AMI type …

37adee8

…combinations

chore: refactor where CUSTOM gets added to possible AMI types

432ead4

lpizzinidev approved these changes Aug 12, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 12, 2024

GavinZZ reviewed Aug 30, 2024

View reviewed changes

shikha372 requested changes Sep 20, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eks): support CUSTOM amiType #30660

feat(eks): support CUSTOM amiType #30660

Artemigos commented Jun 25, 2024

aws-cdk-automation left a comment

lpizzinidev left a comment

lpizzinidev Jul 29, 2024

Artemigos Aug 12, 2024

lpizzinidev Jul 29, 2024

Artemigos Aug 12, 2024

lpizzinidev Jul 29, 2024

Artemigos Aug 12, 2024

aws-cdk-automation commented Aug 12, 2024

lpizzinidev left a comment

GavinZZ left a comment •

edited

Loading

GavinZZ Aug 30, 2024

GavinZZ Aug 30, 2024

GavinZZ Aug 30, 2024

shikha372 left a comment •

edited

Loading

	To use a custom AMI for the node group, you can set `amiType` to `eks.NodegroupAmiType.CUSTOM` and provide a launch template.
	To use a custom AMI for the node group, you can set `amiType` to `CUSTOM`.
	A launch template must be provided.

		* The AMI type for your node group. If you explicitly specify the launchTemplate with custom AMI, either set this property to `CUSTOM` or leave
		* it undefined, otherwise the node group deployment will fail. In other cases, you will need to specify correct amiType for the nodegroup.

feat(eks): support CUSTOM amiType #30660

Are you sure you want to change the base?

feat(eks): support CUSTOM amiType #30660

Conversation

Artemigos commented Jun 25, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 12, 2024

AWS CodeBuild CI Report

lpizzinidev left a comment

Choose a reason for hiding this comment

GavinZZ left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikha372 left a comment • edited Loading

Choose a reason for hiding this comment

GavinZZ left a comment •

edited

Loading

shikha372 left a comment •

edited

Loading