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

Detect Graviton 2 instance as arm64 Fix #1113 #1116

Merged
merged 2 commits into from
Dec 10, 2020
Merged

Detect Graviton 2 instance as arm64 Fix #1113 #1116

merged 2 commits into from
Dec 10, 2020

Conversation

pingleig
Copy link
Member

@pingleig pingleig commented Dec 9, 2020

Detect Graviton 2 arm instances as arm64 when choosing AMI.

Fix #1113

Enter [N/A] in the box, if an item is not applicable to your change.

Testing

  • Unit tests passed
  • Integration tests passed
  • Unit tests added for new functionality
  • Listed manual checks and their outputs in the comments (example)
  • Link to issue or PR for the integration tests:

Documentation

  • [N/A] Contacted our doc writer
  • [N/A] Updated our README

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

@pingleig
Copy link
Member Author

pingleig commented Dec 9, 2020 •
edited
Loading

Only did manual test, but I can run integration test if it is required cc @allisaurus

make bin/darwin-amd64/ecs-cli

./bin/darwin-amd64/ecs-cli up --capability-iam --size 2 --instance-type m6g.medium --cluster ecs-graviton2 --region us-west-2
WARN[0000] You will not be able to SSH into your EC2 instances without a key pair.
INFO[0007] Using Arm ecs-optimized AMI because instance type was m6g.medium
INFO[0008] Using recommended Amazon Linux 2 AMI with ECS Agent 1.48.1 and Docker version 19.03.13-ce
INFO[0008] Created cluster                               cluster=ecs-graviton2 region=us-west-2
INFO[0009] Waiting for your cluster resources to be created...
INFO[0009] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
INFO[0069] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
INFO[0130] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
VPC created: vpc-123
Security Group created: sg-123
Subnet created: subnet-123
Subnet created: subnet-123
Cluster creation succeeded.

And all the arm instances I found from public doc

General Purpose

  • t4g
  • m6g
  • m6gd
  • a1

Compute Optimized

  • c6g
  • c6gd

Memory Optimized

  • r6g
  • r6gd

Accelerated Computing

  • N/A

Storage Optimized

  • N/A

btw: I am not sure if .metal instances are supported by ECS, I can change the regexp so m6g.metal etc. use arm AMI as well.

@allisaurus allisaurus self-assigned this Dec 9, 2020
@pingleig
Detect Graviton 2 instance as arm64 Fix aws#1113
f90a972 
- The original PR aws#695 only handles a1, which is the only arm instance
  type at that time. Now there are more instance types like c6g etc.
- Tweak the unit test a bit so instance types using same ami are merged into
  one test case.
@pingleig
Copy link
Member Author

pingleig commented Dec 9, 2020

Checked a1.metal and it works, mg6.metal asks for 64 VPUs and is beyond my current quota (which is right at 64 .. hmm). Did a force push on my fork.

ecs-cli up --capability-iam --size 1 --instance-type a1.metal --cluster ecs-graviton2-metal --region us-west-2
WARN[0000] You will not be able to SSH into your EC2 instances without a key pair.
INFO[0000] Using Arm ecs-optimized AMI because instance type was a1.metal
INFO[0000] Using recommended Amazon Linux 2 AMI with ECS Agent 1.48.1 and Docker version 19.03.13-ce
INFO[0000] Created cluster                               cluster=ecs-graviton2-metal region=us-west-2
INFO[0001] Waiting for your cluster resources to be created...
INFO[0001] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
INFO[0062] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
INFO[0123] Cloudformation stack status                   stackStatus=CREATE_IN_PROGRESS
VPC created: vpc-0a38aae81317f590d
Security Group created: sg-123
Subnet created: subnet-123
Subnet created: subnet-123
Cluster creation succeeded.

allisaurus
allisaurus reviewed Dec 10, 2020
View reviewed changes
Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

this is awesome @pingleig ! I have one minor comment re: expanding a test case but otherwise LGTM

ecs-cli/modules/clients/aws/amimetadata/client_test.go Outdated
@@ -54,7 +43,7 @@ func TestMetadataClient_GetRecommendedECSLinuxAMI(t *testing.T) {
},
{
// validate that we use the generic AMI for other instances
"t2.micro",
[]string{"t2.micro"},
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're accounting for multiple instance types in the other AMI selection tests, can we add a few more "generic" types here? T3, M5a, C4, etc. (doesn't have to be exhaustive, but at least several popular ones that adhere to diff patterns)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 2d6be87

@SoManyHs
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 10, 2020

Command update: success

Branch already up to date

@allisaurus allisaurus merged commit 4c40431 into aws:mainline Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisaurus allisaurus allisaurus approved these changes

Assignees

@allisaurus allisaurus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Graviton2 instance type are detected as x86 instead of arm64
3 participants
@pingleig @SoManyHs @allisaurus