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

CA: Update AWS instance type list #2550

Merged
merged 2 commits into from
Dec 30, 2019
Merged

CA: Update AWS instance type list #2550

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bceab7
CA: Update AWS instance type list
gjtempleton Nov 20, 2019
9211366
Further EC2 instance type generation
gjtempleton Dec 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions cluster-autoscaler/cloudprovider/aws/ec2_instance_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ limitations under the License.

package aws

// InstanceType is sepc of EC2 instance
// InstanceType is spec of EC2 instance
type InstanceType struct {
InstanceType string
VCPU int64
Expand Down Expand Up @@ -223,7 +223,7 @@ var InstanceTypes = map[string]*InstanceType{
"c5d.12xlarge": {
InstanceType: "c5d.12xlarge",
VCPU: 48,
MemoryMb: 131072,
Copy link
Contributor

Choose a reason for hiding this comment

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

Em..Interesting. I do use the generator to populate the specs. Do you change manually or use the gen.go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was confused as well when I noticed it, even weirder I changed it by running make generate again, so I can only assume the API was returning different results when you last generated it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. The new values you give match with values in aws docs. Great!

MemoryMb: 98304,
GPU: 0,
},
"c5d.18xlarge": {
Expand All @@ -235,7 +235,7 @@ var InstanceTypes = map[string]*InstanceType{
"c5d.24xlarge": {
InstanceType: "c5d.24xlarge",
VCPU: 96,
MemoryMb: 262144,
MemoryMb: 196608,
GPU: 0,
},
"c5d.2xlarge": {
Expand Down Expand Up @@ -265,7 +265,7 @@ var InstanceTypes = map[string]*InstanceType{
"c5d.metal": {
InstanceType: "c5d.metal",
VCPU: 96,
MemoryMb: 262144,
MemoryMb: 196608,
GPU: 0,
},
"c5d.xlarge": {
Expand Down Expand Up @@ -436,6 +436,12 @@ var InstanceTypes = map[string]*InstanceType{
MemoryMb: 31232,
GPU: 1,
},
"g4dn": {
InstanceType: "g4dn",
VCPU: 96,
MemoryMb: 393216,
GPU: 8,
},
"g4dn.12xlarge": {
InstanceType: "g4dn.12xlarge",
VCPU: 48,
Expand Down Expand Up @@ -466,6 +472,12 @@ var InstanceTypes = map[string]*InstanceType{
MemoryMb: 131072,
GPU: 1,
},
"g4dn.metal": {
InstanceType: "g4dn.metal",
VCPU: 96,
MemoryMb: 393216,
GPU: 8,
},
"g4dn.xlarge": {
InstanceType: "g4dn.xlarge",
VCPU: 4,
Expand Down Expand Up @@ -1702,6 +1714,30 @@ var InstanceTypes = map[string]*InstanceType{
MemoryMb: 12582912,
GPU: 0,
},
"u-18tb1": {
InstanceType: "u-18tb1",
VCPU: 448,
MemoryMb: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason it get 0 MemoryMb?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I failed to notice that! I think I'll need to dig into this and figure out why that's happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally got around to spending more time on digging into this, and it looks like the Pricing API we're currently using can't be fully trusted. For one thing the correct API designation of these instances is u-18tb1.metal, not u-18tb1 as returned by the pricing API.

I'm going to raise a separate issue to discuss whether/how we move away from the Pricing API as it seems it can't be fully trusted to build a proper picture of these nodes, in the meantime any problem with merging this as is? (I've updated it with a couple more changes generated when running today and double checked them against the info returned by instance descriptions using a boto3 client.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Em.. u-18tb1.metal is the right name according to release note. I don't have concern merging this since this instances are available from dedicated hosts with 3 yrs reservation. Most users may not even have access to it. Feel free to open a new issue and let's resolve the pricing api issue there. Some other projects uses nodes template as well like CNI, I will also have a check on reliable API.

GPU: 0,
},
"u-18tb1.metal": {
InstanceType: "u-18tb1.metal",
VCPU: 448,
MemoryMb: 18874368,
GPU: 0,
},
"u-24tb1": {
InstanceType: "u-24tb1",
VCPU: 448,
MemoryMb: 0,
GPU: 0,
},
"u-24tb1.metal": {
InstanceType: "u-24tb1.metal",
VCPU: 448,
MemoryMb: 25165824,
GPU: 0,
},
"u-6tb1": {
InstanceType: "u-6tb1",
VCPU: 448,
Expand Down