Hey @neilkuan I'll need your initial review. Let's discuss this offline.

for testing I know about if I not define amitype default will be AL2_x86_64.

so if I define instanceTypes to ['t4g.medium'],I will get error message from cloudformation.

[t4g.medium] is not a valid instance type for requested amiType AL2_x86_64 (Service: AmazonEKS; Status Code: 400; Error Code: InvalidParameterException; Request ID: 51f86ddf-0218-42c4-86d7-b02ffbe18f76; Proxy: null)

Some NITS. Other than that, looks good. Just need to be cautious that we need to calculate the recommended amiType corresponding to the instanceTypes when the user doesn't specify amiType or ami in launchTemplate. Also need to care the backward compatibility when calculating the recommended amiType for user.

Maybe add something like this in the if (instanceTypes && instanceTypes.length > 0) block:

if (!props.amiType && !props.launchTemplateSpec) {
  // calculate the recommended `amiType` corresponding to the `instanceTypes`
}

or when constructing the CfnNodegroup, use:

const resource = new CfnNodegroup(this, 'Resource', {
  ...
  amiType: props.launchTemplateSpec ? props.amiType : (props.amiType ?? possibleAmiTypes[0]),
  ...
});

Thanks for the suggested changes.

Let's dive a little bit deeper here. We have some optional properties in this construct:

1. props.instanceTypes is optional
2. props.launchTemplateSpec is optional
3. props.amiType is optional

When props.instanceTypes is specified, there might be some cases:

1. amiType undefined with launchTemplateSpec undefined
   Check:
   • ensure the instanceTypes provided are compatible with the default amiType, i.e. AL2_X86_64.
   • to avoid BC, we use possibleAmiTypes[0] as the default amiType, which should be AL2.
2. amiType undefined with launchTemplateSpec defined
   Yes it's possible to have launchTemplateSpec defined while the instance types are also defined in props.instanceTypes property.
   Check:
   • Same as above but we don't propose a default amiType. We leave props.amiType as-is.
3. amiType defined with launchTemplateSpec undefined

• we check if props.amiType is included in possibleAmiTypes we generate from props.instanceTypes.

4. amiType defined with launchTemplateSpec defined

• we still have to check if props.amiType is included in possibleAmiTypes we generate from props.instanceTypes. As we can't see if AMI is also defined in the launch template, it's user responsibility to leave props.amiType undefined if AMI is also specified in the launch template otherwise the deploy will fail.

That being said, it doesn't make difference where props.launchTemplateSpec is defined or not. The validation check is literally the same.

Now let's check the following cases:

Case 1:

instanceTypes: ['t4g.medium']
amiType: `undefined`
launchTemplateSpec: `undefined`

Previously users would have AL2_ARM_64 as the expectedAmiType. Now the possibleAmiTypes[0] would still be AL2_ARM_64. No breaking change.

Case 2:

instanceTypes: ['t4g.medium']
amiType: undefined
launchTemplateSpec: something (with custom AMI)

We just use props.amiType. It's user responsibility to leave props.amiType undefined if custom AMI is defined in the launch template.

Hi @neilkuan @kirintwn I am ready. Can you take another look?

1. Can you also add testing for got this error message instanceTypes of different CPU architectures is not allowed ?!
   ex:

 this.cluster.addNodegroupCapacity('BottlerocketNG1', {
      instanceTypes: [new ec2.InstanceType('t4g.large'), new ec2.InstanceType('t2.large')],
    });

The test is already there.
https://github.com/pahud/aws-cdk/blob/d3acc17b3fa9e9c928d05ff9e85f4f1fb508e053/packages/%40aws-cdk/aws-eks/test/nodegroup.test.ts#L728-L745

2. Can you add the c7g in to instance-types.ts for graviton3?!

c7g is still in preview so I am not adding it in this PR.

https://aws.amazon.com/tw/ec2/instance-types/c7g/

other LGTM.

@neilkuan @kirintwn all addressed. Please take another look. Thanks.

I have revised the implementation based on your suggested changes. I tend to not modify aws-ec2 in this PR so I have a tiny isGpuInstanceType() helper function.

Yeah, I see how changing InstanceType would break existing code. But still, there is an opportunity for simplification here. Something like:

type MyArchitecture = InstanceArchitecture | 'GPU';

function getPossibleAmiTypes(instanceTypes: InstanceType[]): NodegroupAmiType[] {
  function typeToArch(instanceType: InstanceType): MyArchitecture {
    return isGpuInstanceType(instanceType) ? 'GPU' : instanceType.architecture;
  }

  const archAmiMap = new Map<MyArchitecture, NodegroupAmiType[]>([
    [InstanceArchitecture.ARM_64, arm64AmiTypes],
    [InstanceArchitecture.X86_64, x8664AmiTypes],
    ['GPU', gpuAmiTypes],
  ]);

  const architectures: Set<MyArchitecture> = new Set(instanceTypes.flatMap(typeToArch));

  if (architectures.size === 0) {
    throw new Error(`Cannot determine any ami type comptaible with instance types: ${instanceTypes.map(i => i.toString).join(',')}`);
  }

  if (architectures.size > 1) {
    throw new Error('instanceTypes of different architectures is not allowed');
  }

  return archAmiMap.get(Array.from(architectures)[0])!;
}

I have revised the implementation based on your suggested changes. I tend to not modify aws-ec2 in this PR so I have a tiny isGpuInstanceType() helper function.

Yeah, I see how changing InstanceType would break existing code. But still, there is an opportunity for simplification here. Something like:

type MyArchitecture = InstanceArchitecture | 'GPU';

function getPossibleAmiTypes(instanceTypes: InstanceType[]): NodegroupAmiType[] {
  function typeToArch(instanceType: InstanceType): MyArchitecture {
    return isGpuInstanceType(instanceType) ? 'GPU' : instanceType.architecture;
  }

  const archAmiMap = new Map<MyArchitecture, NodegroupAmiType[]>([
    [InstanceArchitecture.ARM_64, arm64AmiTypes],
    [InstanceArchitecture.X86_64, x8664AmiTypes],
    ['GPU', gpuAmiTypes],
  ]);

  const architectures: Set<MyArchitecture> = new Set(instanceTypes.flatMap(typeToArch));

  if (architectures.size === 0) {
    throw new Error(`Cannot determine any ami type comptaible with instance types: ${instanceTypes.map(i => i.toString).join(',')}`);
  }

  if (architectures.size > 1) {
    throw new Error('instanceTypes of different architectures is not allowed');
  }

  return archAmiMap.get(Array.from(architectures)[0])!;
}

@otaviomacedo This is interesting! Let me try it.

@otaviomacedo all fixed. Please take a look.

@kirintwn all resolved :-)

@pahud:

And is it okay to let the docstring separated from the function with this type declaration?
Will it breaks auto-generation mechanism of documentation?

can you confirm this? thanks!

can you confirm this? thanks!

@kirintwn nice catch! all fixed!

Maybe another look? copy @otaviomacedo

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 5bda204
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).