Skip to content

Bug 1710981: Default AWS instance type to 'm5' #3853

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

Merged
openshift-merge-robot merged 1 commit into from
Jul 8, 2020
Merged

Bug 1710981: Default AWS instance type to 'm5' #3853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions docs/user/aws/limits.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ to be created. The security groups which exist after the default install are:
By default, a cluster will create:

* One m4.large bootstrap machine (2 vCPUs but removed after install)
* Three m4.xlarge master nodes (4 vCPUs each).
* Three m4.large worker nodes (2 vCPUs each).
* Three m5.xlarge master nodes (4 vCPUs each).
* Three m5.large worker nodes (2 vCPUs each).

Currently, these vCPU counts are not within a new account's default limit. The default limit is 1 but for all these instances you will need 20. To increase the limit you have to [contact the AWS support](https://console.aws.amazon.com/support/cases?#/create?issueType=service-limit-increase&limitType=ec2-instances).
If you intend to start with a higher number of workers, enable autoscaling and large workloads
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/machines/master_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ spec:
Platform: types.MachinePoolPlatform{
AWS: &awstypes.MachinePool{
Zones: []string{"us-east-1a"},
InstanceType: "m4.xlarge",
InstanceType: "m5.xlarge",
},
},
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/machines/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ spec:
Platform: types.MachinePoolPlatform{
AWS: &awstypes.MachinePool{
Zones: []string{"us-east-1a"},
InstanceType: "m4.large",
InstanceType: "m5.large",
},
},
},
Expand Down
19 changes: 6 additions & 13 deletions pkg/types/aws/defaults/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,8 @@ import (

var (
defaultMachineClass = map[string][]string{
"ap-east-1": {"m5", "m4"},
"ap-northeast-2": {"m5", "m4"},
"eu-north-1": {"m5", "m4"},
"eu-west-3": {"m5", "m4"},
"me-south-1": {"m5", "m4"},
"us-gov-east-1": {"m5", "m4"},
"us-west-2": {"m5", "m4"},
// Example region default machine class override:
// "ap-east-1": {"m5", "m4"},
Copy link
Member Author

@mtnbikenc mtnbikenc Jul 7, 2020

Choose a reason for hiding this comment

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

@abhinavdahiya Should we just rip out all the logic for having different defaults for different regions or keep the logic and structure in case we need to implement something similar in the future?

Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 7, 2020

Choose a reason for hiding this comment

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

i think we should remove the overrides, but keep the mechanism in place.

Copy link
Member

@wking wking Jul 7, 2020

Choose a reason for hiding this comment

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

we need to keep some overrides for regions/zones that lack m5 support, right?

Copy link
Member

@wking wking Jul 7, 2020

Choose a reason for hiding this comment

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

never mind, I'm stale after the removal of platformtests/ (which I've just filed #3856 to wrap up). I'm not clear on why we would need/want the ability to set overrides if we trust the dynamic APIs we've used since #3051.

}
)

Expand All @@ -21,21 +16,19 @@ func SetPlatformDefaults(p *aws.Platform) {
}

// InstanceClass returns the instance "class" we should use for a given
// region. We prefer m4 if available (more EBS volumes per node) but will use
// m5 in regions that don't have m4.
// region. Default is m5 unless a region override is defined in defaultMachineClass.
func InstanceClass(region string) string {
if classes, ok := defaultMachineClass[region]; ok {
return classes[0]
}
return "m4"
return "m5"
}

// InstanceClasses returns a list of instance "class", in decreasing priority order, which we should use for a given
// region. We prefer m4 if available (more EBS volumes per node) but will use
// m5 in regions that don't have m4.
// region. Default is m5 then m4 unless a region override is defined in defaultMachineClass.
func InstanceClasses(region string) []string {
if classes, ok := defaultMachineClass[region]; ok {
return classes
}
return []string{"m4", "m5"}
return []string{"m5", "m4"}
}
2 changes: 1 addition & 1 deletion upi/aws/cloudformation/05_cluster_master_nodes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Parameters:
Description: IAM profile to associate with master nodes.
Type: String
MasterInstanceType:
Default: m4.xlarge
Default: m5.xlarge
Type: String
AllowedValues:
- "m4.xlarge"
Expand Down
2 changes: 1 addition & 1 deletion upi/aws/cloudformation/06_cluster_worker_node.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Parameters:
Description: IAM profile to associate with master nodes.
Type: String
WorkerInstanceType:
Default: m4.large
Default: m5.large
Type: String
AllowedValues:
- "m4.large"
Expand Down