Managed Node Group Launch Template Improvements #1225
Conversation
… support setting disk size when a MNG uses a custom launch template
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
nodejs/eks/nodegroup.ts
Outdated
| * | ||
| * Note: `amiType` and `gpu` are mutually exclusive. | ||
| */ | ||
| amiType?: pulumi.Input<string>; |
There was a problem hiding this comment.
These looks like something copied over from NodeGroupBaseOptions, I wonder if we could factor this out in TypeScript somehow.
There was a problem hiding this comment.
I had a look, this inherits directly from the AWS config so I figured we shouldn't mess with it too much.
|
Thanks for contributing here! We indeed rely heavily on integration-level tests so an AWS testing account is needed to test the changes. |
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
|
Yep @t0yv0 - Is testing something you're prepared to help with? I do have access to my company's AWS account, but spinning up cluster(s) is a pretty heavy operation so would love to avoid if possible. |
|
@JustASquid I can help with testing this. Do you want to have a first stab at creating a test? |
|
Cheers @flostadler - I just went on leave, should be able to take a stab at writing some tests in a couple weeks. Appreciate the help and guidance. |
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
|
@flostadler - I added the SDK generation and took a crack at adding some tests - surely they are missing something but hopefully it's a jumping off point. |
| eks.createManagedNodeGroup(`${projectName}-managed-ng`, { | ||
| cluster: cluster, | ||
| nodeRole: role, | ||
| gpu: true |
There was a problem hiding this comment.
Does this have any special instance type requirements or can we launch a GPU optimized AMI on a basic t3 instance for example?
| @@ -735,6 +735,21 @@ func generateSchema() schema.PackageSpec { | |||
| "(https://docs.aws.amazon.com/eks/latest/APIReference/API_Nodegroup.html#AmazonEKS-Type-Nodegroup-amiType) " + | |||
| "for valid AMI Types. This provider will only perform drift detection if a configuration value is provided.", | |||
| }, | |||
| "amiId": { | |||
There was a problem hiding this comment.
You'll need to run make build and check in the changes to the schema and sdk folder. Otherwise those don't take effect.
Because of the disparity between the nodejs SDK and the rest of the SDKs it would be good to add a test in another language as well (e.g. Golang). Let me know if you need help with converting the test you've created into another language
| // Find the recommended AMI for the EKS node group. | ||
| // See https://docs.aws.amazon.com/eks/latest/userguide/retrieve-ami-id.html | ||
| async function getEksAmiId(k8sVersion: string): Promise<string> { | ||
| const client = new SSMClient(); |
There was a problem hiding this comment.
You can do that in pulumi natively. See here for an example:
pulumi-eks/examples/custom-managed-nodegroup/index.ts
Lines 38 to 40 in 9d80b5d
| const amiId = cluster.eksCluster.version.apply(version => pulumi.output(getEksAmiId(version))); | ||
|
|
||
| // Create a managed node group using a cluster as input. | ||
| eks.createManagedNodeGroup(`${projectName}-managed-ng`, { |
There was a problem hiding this comment.
What do you think about combining the ami and gpu tests into one?
One test with two nodegroups should be fine.
We need to balance the number of eks clusters we're creating and here we're interested in the node group.
| @@ -1740,7 +1775,7 @@ function createManagedNodeGroupInternal( | |||
| } | |||
|
|
|||
| let launchTemplate: aws.ec2.LaunchTemplate | undefined; | |||
| if (args.kubeletExtraArgs || args.bootstrapExtraArgs || args.enableIMDSv2) { | |||
| if (args.kubeletExtraArgs || args.bootstrapExtraArgs || args.enableIMDSv2 || args.gpu || args.amiId || args.amiType) { | |||
There was a problem hiding this comment.
Why do we need to create a custom launch template if amiType is set? That's a valid input for aws.eks.NodeGroup on its own.
Aside from that, the list here gets a bit long now. What do you think about creating a method for deciding whether to create a custom launch template?
|
Thanks a lot @JustASquid! I left some comments, let me know if you need some help with any of the proposed changes |
|
@JustASquid I'm starting to touch the same parts of the code base as part of refactoring needed for #1195. I'd propose that I fold your changes into that to prevent duplication/conflicts. |
|
Closing this in favor of #1195. |
Proposed changes
Add the ability to specify
gpu/amiId/amiTypeon a managed node group, and specify disk size when a launch template is created.This PR is a draft because I haven't been able to test the changes (yet) - any guidance on this is helpful, I don't have easy access to a spare AWS account where I can spin up test clusters.
Related issues (optional)
Resolves #1224