Add missing configuration options for Managed Node Groups #1339
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
flostadler
force-pushed
the
flostadler/mng-better-configs
branch
2 times, most recently
from
4be30e4
to
e85b6e5
Compare
…de groups going forward
flostadler
force-pushed
the
flostadler/bottlerocket-disk-size
branch
from
de0d23c
to
50b96a5
Compare
|
|
flostadler
force-pushed
the
flostadler/mng-better-configs
branch
from
e85b6e5
to
c23a7a3
Compare
…/mng-better-configs
|
Does this cover #1224 GPU support? |
| "See for more details: https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html#launch-template-user-data.", | ||
| }, | ||
| "gpu": { | ||
| TypeSpec: schema.TypeSpec{Type: "boolean"}, |
There was a problem hiding this comment.
Would this be any better with defining the explicit default in the schema?
There was a problem hiding this comment.
We need to know whether it's false or not defined. There's certain combinations that do not work (e.g. you cannot choose an AMI with GPU and then set GPU to false).
Is my understanding wrong that we'd receive false in the provider in that case?
| }, | ||
| "amiId": { | ||
| TypeSpec: schema.TypeSpec{Type: "string"}, | ||
| Description: "The AMI ID to use for the worker nodes.\nDefaults to the latest recommended " + |
There was a problem hiding this comment.
Likewise, should schema define Default: "latest" or some such?
There was a problem hiding this comment.
The default value here is not "latest". The provider performs actions in the background to fetch the latest recommended AMI ID.
There was a problem hiding this comment.
So it does so when amiId is empty/undefined, the provider does the search. OK, I guess the current form is the best.
| @@ -2105,9 +2163,6 @@ function getRecommendedAMI( | |||
| k8sVersion: pulumi.Output<string>, | |||
| parent: pulumi.Resource | undefined, | |||
| ): pulumi.Input<string> { | |||
| // TODO[pulumi/pulumi-eks#1195]: Add gpu argument to ManagedNodeGroupOptions | |||
Yes it does! I folded that into this PR because it's relevant here as well. (added this to the PR description) |
nodejs/eks/nodegroup.ts
Outdated
| amiIdMutuallyExclusive.forEach((key) => { | ||
| if (args.amiId && args[key]) { | ||
| throw new pulumi.ResourceError( | ||
| `amiId and ${key} are mutually exclusive to create a managed node group`, |
There was a problem hiding this comment.
The grammar here seems a little funny (but also I'm not a native speaker). Feed it to LLM?
There was a problem hiding this comment.
I actually copied the grammar from a similar error message from above. But now that you're saying this it also sounds weird to me (also not native speaker).
Let me reword those.
| operatingSystem: eks.OperatingSystem.Bottlerocket, | ||
| instanceTypes: ["g5g.xlarge"], | ||
| nodeRole: role, | ||
| gpu: true, |
flostadler
merged commit 46f69fe
into
flostadler/al2023-bottlerocket-support
The `ManagedNodeGroup` component was missing configuration the other node groups had. In detail, that's `amiId`, `gpu` and `userData`. Those will allow booting specific/custom AMIs, nodes with GPUs or setting custom user data. The added E2E tests ensure this works as expected. Relates to #1224
The `ManagedNodeGroup` component was missing configuration the other node groups had. In detail, that's `amiId`, `gpu` and `userData`. Those will allow booting specific/custom AMIs, nodes with GPUs or setting custom user data. The added E2E tests ensure this works as expected. Relates to #1224
The `ManagedNodeGroup` component was missing configuration the other node groups had. In detail, that's `amiId`, `gpu` and `userData`. Those will allow booting specific/custom AMIs, nodes with GPUs or setting custom user data. The added E2E tests ensure this works as expected. Relates to #1224
The
ManagedNodeGroupcomponent was missing configuration the other node groups had.In detail, that's
amiId,gpuanduserData.Those will allow booting specific/custom AMIs, nodes with GPUs or setting custom user data.
The added E2E tests ensure this works as expected.
Relates to #1224