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.

Sign up for GitHub

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

Jump to bottom

feat(eks): arm64 support #9875

Merged
merged 24 commits into from
Sep 7, 2020
Merged

feat(eks): arm64 support #9875

merged 24 commits into from
Sep 7, 2020

Conversation

pahud
Copy link
Contributor

@pahud pahud commented Aug 20, 2020 •
edited
Loading

feat(eks): support ARM64 for self-managed and managed nodegroup

Closes: #9915

  • self-managed nodegroups with ARM64 support
  • managed nodegroups with ARM64 support
  • cluster default capacity with ARM64 support
  • implicitly determine the amiType from the instanceType to make it just works.
  • update README
  • integ and unit tests

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

@pahud pahud marked this pull request as draft August 21, 2020 01:20
@pahud pahud changed the title feat(eks): support ARM64 for self-managed nodegroup feat(eks): support ARM64 for self-managed and managed nodegroup Aug 21, 2020
@pahud pahud changed the title feat(eks): support ARM64 for self-managed and managed nodegroup feat(eks): ARM64 support for nodegroup Aug 21, 2020
@pahud pahud marked this pull request as ready for review August 21, 2020 09:36
@iliapolo
Copy link
Contributor

iliapolo commented Aug 22, 2020 •
edited
Loading

Hi @pahud - Thanks for this!

Would you mind please creating an issue for this and marking it in-progress? It will help with discoverability and communicate this feature is being actively worked on.

Thanks!

@pahud
Copy link
Contributor Author

pahud commented Aug 23, 2020

Hi @pahud - Thanks for this!

Would you mind please creating an issue for this and marking it in-progress? It will help with discoverability and communicate this feature is being actively worked on.

Thanks!

Absolutely. The issue has been linked to this PR now.

Feel free to have the first round of the review if you have time.

Thanks

eladb
eladb reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

initial pass

@pahud
Copy link
Contributor Author

pahud commented Aug 27, 2020

Hi @eladb , all addressed in the initial round. :-)

@pahud pahud requested a review from eladb August 28, 2020 00:19
@iliapolo iliapolo assigned eladb and unassigned iliapolo Aug 29, 2020
eladb
eladb previously requested changes Sep 1, 2020
View reviewed changes
@pahud
Update packages/@aws-cdk/aws-eks/README.md
8dc10b0 
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
@mergify mergify bot dismissed eladb’s stale review September 1, 2020 15:27

Pull request has been modified.

@pahud
Update packages/@aws-cdk/aws-eks/README.md
2506119 
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
pahud and others added 3 commits September 1, 2020 23:29
@pahud
Update packages/@aws-cdk/aws-eks/README.md
957d68e 
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
@pahud
Update packages/@aws-cdk/aws-eks/lib/cluster.ts
c825392 
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
@pahud
Update packages/@aws-cdk/aws-eks/lib/managed-nodegroup.ts
accb37f 
Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>
@eladb eladb changed the title feat(eks): ARM64 support for nodegroup feat(eks): arm64 support Sep 2, 2020
@eladb
Copy link
Contributor

eladb commented Sep 3, 2020

There are still a few unresolved issues @pahud

@pahud
Copy link
Contributor Author

pahud commented Sep 3, 2020

@eladb WIP 💪

eladb
eladb previously requested changes Sep 4, 2020
View reviewed changes
@mergify mergify bot dismissed eladb’s stale review September 4, 2020 14:20

Pull request has been modified.

@pahud
Copy link
Contributor Author

pahud commented Sep 4, 2020

Hi @eladb

All resolved. Please check it out again. thanks.

eladb
eladb previously approved these changes Sep 7, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Approved with a minor comment.
Remove the do-not-merge label when ready

packages/@aws-cdk/aws-eks/lib/instance-types.ts Outdated
@@ -0,0 +1,4 @@
export const GPU_INSTANCETYPES = ['p2', 'p3', 'g2', 'g3', 'g4'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to a map?

export INSTANCE_TYPES = {
  gpu: [ ... ],
  ...
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eladb

Just fixed :-)

@eladb eladb added the pr/do-not-merge This PR should not be merged at this time. label Sep 7, 2020
@mergify mergify bot dismissed eladb’s stale review September 7, 2020 10:52

Pull request has been modified.

@eladb eladb removed the pr/do-not-merge This PR should not be merged at this time. label Sep 7, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2020

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 081e498
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Sep 7, 2020

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).

@mergify mergify bot merged commit ffb84c6 into aws:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

Assignees

@eladb eladb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[aws-eks] support ARM64 instances
4 participants
@pahud @iliapolo @eladb @aws-cdk-automation