Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(eks): update nodegroup gpu check #31445

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

AlexKaracaoglu
Copy link

@AlexKaracaoglu AlexKaracaoglu commented Sep 13, 2024

Issue # (if applicable)

Closes #31347.

Reason for this change

The motivating bug is that you cannot add a combo of g5 and a g6 as instance classes onto the same node group or else the following error will be thrown: instanceTypes of different architectures is not allowed.

Description of changes

  • (eks) Fixes the isGpuInstanceType check
    • G6/G6E instance classes will now be recognized as GPU instance types so those different types can be used together for a multi instance type managed node group

Description of how you validated changes

Wrote unit tests for the isGpuInstanceType function

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team September 13, 2024 19:43
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Sep 13, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 13, 2024
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

tools/@aws-cdk/cdk-build-tools/package.json Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Sep 14, 2024
Copy link
Contributor

@sumupitchayan sumupitchayan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR @AlexKaracaoglu - I think adding an Integ test or modifying and existing one to use this new Instance Type would be nice to confirm that it successfully deploys.

Also, can you add a unit test for the isGpuInstanceType function?

@pahud
Copy link
Contributor

pahud commented Oct 6, 2024

Hi @AlexKaracaoglu

Not sure if you are still on it but you can refer to this commit I made
pahud@8fd3db8

@AlexKaracaoglu
Copy link
Author

@pahud @sumupitchayan - Appreciate the follow ups, I'll get some changes in later today

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7f82fd5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@AlexKaracaoglu AlexKaracaoglu changed the title chore(eks): update nodegroup gpu check and add g6e instance class chore(eks): update nodegroup gpu check Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-eks: manage nodegroups GPU instance types not up to date
5 participants