Skip to content

Commit

Permalink
pkg/types/aws/defaults/platform: Per-region instance-class defaults
Browse files Browse the repository at this point in the history
From Trevor:

For regions like eu-west-3 (Paris) that don't support m4.  The only
instance type available in all AWS regions today is i3.  We prefer m4
because of the higher EBS attach limit.  But for regions that don't
support m4 we use m5.

This also adds a CI job which checks the AWS pricing API to keep our
region map up to date.

The eu-north-1 region is new as of 2018-12-12 [1], and it show up in:

  $ aws ec2 describe-regions --query 'Regions[].RegionName' --output json | jq -r 'sort[]'
  ...
  eu-central-1
  eu-north-1
  eu-west-1
  ...

The GovCloud entries didn't show up in that query (maybe they are only
listed for accounts that have access?), but they are listed on [2].

The new test is in platformtests/aws to keep it out of our default
unit tests.  Maintainers can run it manually to verify that the
default fallbacks we hard-code are still appropriate for the current
AWS offerings.  It's not in tests/aws, because 'dep ensure' seems to
ignore tests.  I think it's fine to have both platformtests and tests
in parallel, with the former holding tests who share the project
vendor/ and the latter holding tests which have a distinct vendor/.
bdd-smoke is in tests with its own vendor/ intentionally to keep
gomega and other test-specific libraries out of the project vendor/
[3].  But for these tests we're using Go's built-in test framework, so
it's less maintenance to layer our minimal additional dependencies
into the project vendor/.

[1]: https://aws.amazon.com/blogs/aws/now-open-aws-europe-stockholm-region/
[2]: https://aws.amazon.com/about-aws/global-infrastructure/regional-product-services/
[3]: #552 (comment)
  • Loading branch information
eparis authored and wking committed Feb 19, 2019
1 parent 820ff4c commit de982f1
Show file tree
Hide file tree
Showing 14 changed files with 202 additions and 25 deletions.
3 changes: 1 addition & 2 deletions data/data/aws/bootstrap/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ variable "ignition" {

variable "instance_type" {
type = "string"
default = "m4.large"
description = "The EC2 instance type for the bootstrap node."
description = "The instance type of the bootstrap node."
}

variable "subnet_id" {
Expand Down
7 changes: 4 additions & 3 deletions data/data/aws/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ module "bootstrap" {
source = "./bootstrap"

ami = "${var.aws_ec2_ami_override}"
instance_type = "${var.aws_bootstrap_instance_type}"
cluster_name = "${var.cluster_name}"
ignition = "${var.ignition_bootstrap}"
subnet_id = "${module.vpc.public_subnet_ids[0]}"
Expand All @@ -32,9 +33,9 @@ module "bootstrap" {
module "masters" {
source = "./master"

cluster_id = "${var.cluster_id}"
cluster_name = "${var.cluster_name}"
ec2_type = "${var.aws_master_ec2_type}"
cluster_id = "${var.cluster_id}"
cluster_name = "${var.cluster_name}"
instance_type = "${var.aws_master_instance_type}"

tags = "${merge(map(
"kubernetes.io/cluster/${var.cluster_name}", "owned",
Expand Down
2 changes: 1 addition & 1 deletion data/data/aws/master/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ resource "aws_instance" "master" {
ami = "${var.ec2_ami}"

iam_instance_profile = "${aws_iam_instance_profile.master.name}"
instance_type = "${var.ec2_type}"
instance_type = "${var.instance_type}"
subnet_id = "${element(var.subnet_ids, count.index)}"
user_data = "${var.user_data_ign}"

Expand Down
2 changes: 1 addition & 1 deletion data/data/aws/master/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ variable "dns_server_ip" {
default = ""
}

variable "ec2_type" {
variable "instance_type" {
type = "string"
}

Expand Down
9 changes: 7 additions & 2 deletions data/data/aws/variables-aws.tf
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@ EOF
default = "1.0"
}

variable "aws_master_ec2_type" {
variable "aws_bootstrap_instance_type" {
type = "string"
description = "Instance size for the master node(s). Example: `m4.large`."
description = "Instance type for the bootstrap node. Example: `m4.large`."
}

variable "aws_master_instance_type" {
type = "string"
description = "Instance type for the master node(s). Example: `m4.large`."
}

variable "aws_ec2_ami_override" {
Expand Down
9 changes: 8 additions & 1 deletion pkg/asset/machines/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/openshift/installer/pkg/asset/machines/openstack"
"github.com/openshift/installer/pkg/asset/rhcos"
awstypes "github.com/openshift/installer/pkg/types/aws"
awsdefaults "github.com/openshift/installer/pkg/types/aws/defaults"
libvirttypes "github.com/openshift/installer/pkg/types/libvirt"
nonetypes "github.com/openshift/installer/pkg/types/none"
openstacktypes "github.com/openshift/installer/pkg/types/openstack"
Expand Down Expand Up @@ -66,6 +67,12 @@ func (m *Master) Dependencies() []asset.Asset {
}
}

func awsDefaultMasterMachineType(installconfig *installconfig.InstallConfig) string {
region := installconfig.Config.Platform.AWS.Region
instanceClass := awsdefaults.InstanceClass(region)
return fmt.Sprintf("%s.xlarge", instanceClass)
}

// Generate generates the Master asset.
func (m *Master) Generate(dependencies asset.Parents) error {
clusterID := &installconfig.ClusterID{}
Expand All @@ -81,7 +88,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
switch ic.Platform.Name() {
case awstypes.Name:
mpool := defaultAWSMachinePoolPlatform()
mpool.InstanceType = "m4.xlarge"
mpool.InstanceType = awsDefaultMasterMachineType(installconfig)
mpool.Set(ic.Platform.AWS.DefaultMachinePlatform)
mpool.Set(pool.Platform.AWS)
if len(mpool.Zones) == 0 {
Expand Down
9 changes: 8 additions & 1 deletion pkg/asset/machines/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/openshift/installer/pkg/asset/machines/openstack"
"github.com/openshift/installer/pkg/asset/rhcos"
awstypes "github.com/openshift/installer/pkg/types/aws"
awsdefaults "github.com/openshift/installer/pkg/types/aws/defaults"
libvirttypes "github.com/openshift/installer/pkg/types/libvirt"
nonetypes "github.com/openshift/installer/pkg/types/none"
openstacktypes "github.com/openshift/installer/pkg/types/openstack"
Expand Down Expand Up @@ -70,6 +71,12 @@ func (w *Worker) Dependencies() []asset.Asset {
}
}

func awsDefaultWorkerMachineType(installconfig *installconfig.InstallConfig) string {
region := installconfig.Config.Platform.AWS.Region
instanceClass := awsdefaults.InstanceClass(region)
return fmt.Sprintf("%s.large", instanceClass)
}

// Generate generates the Worker asset.
func (w *Worker) Generate(dependencies asset.Parents) error {
clusterID := &installconfig.ClusterID{}
Expand All @@ -92,7 +99,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
switch ic.Platform.Name() {
case awstypes.Name:
mpool := defaultAWSMachinePoolPlatform()
mpool.InstanceType = "m4.large"
mpool.InstanceType = awsDefaultWorkerMachineType(installconfig)
mpool.Set(ic.Platform.AWS.DefaultMachinePlatform)
mpool.Set(pool.Platform.AWS)
if len(mpool.Zones) == 0 {
Expand Down
32 changes: 19 additions & 13 deletions pkg/tfvars/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,22 @@ package aws

import (
"encoding/json"
"fmt"

"github.com/openshift/installer/pkg/types/aws/defaults"
"github.com/pkg/errors"
"sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1beta1"
)

type config struct {
EC2AMIOverride string `json:"aws_ec2_ami_override,omitempty"`
ExtraTags map[string]string `json:"aws_extra_tags,omitempty"`
EC2Type string `json:"aws_master_ec2_type,omitempty"`
IOPS int64 `json:"aws_master_root_volume_iops"`
Size int64 `json:"aws_master_root_volume_size,omitempty"`
Type string `json:"aws_master_root_volume_type,omitempty"`
Region string `json:"aws_region,omitempty"`
EC2AMIOverride string `json:"aws_ec2_ami_override,omitempty"`
ExtraTags map[string]string `json:"aws_extra_tags,omitempty"`
BootstrapInstanceType string `json:"aws_bootstrap_instance_type,omitempty"`
MasterInstanceType string `json:"aws_master_instance_type,omitempty"`
IOPS int64 `json:"aws_master_root_volume_iops"`
Size int64 `json:"aws_master_root_volume_size,omitempty"`
Type string `json:"aws_master_root_volume_type,omitempty"`
Region string `json:"aws_region,omitempty"`
}

// TFVars generates AWS-specific Terraform variables launching the cluster.
Expand Down Expand Up @@ -46,13 +49,16 @@ func TFVars(masterConfig *v1beta1.AWSMachineProviderConfig) ([]byte, error) {
return nil, errors.New("EBS IOPS must be configured for the io1 root volume")
}

instanceClass := defaults.InstanceClass(masterConfig.Placement.Region)

cfg := &config{
Region: masterConfig.Placement.Region,
ExtraTags: tags,
EC2AMIOverride: *masterConfig.AMI.ID,
EC2Type: masterConfig.InstanceType,
Size: *rootVolume.EBS.VolumeSize,
Type: *rootVolume.EBS.VolumeType,
Region: masterConfig.Placement.Region,
ExtraTags: tags,
EC2AMIOverride: *masterConfig.AMI.ID,
BootstrapInstanceType: fmt.Sprintf("%s.large", instanceClass),
MasterInstanceType: masterConfig.InstanceType,
Size: *rootVolume.EBS.VolumeSize,
Type: *rootVolume.EBS.VolumeType,
}

if rootVolume.EBS.Iops != nil {
Expand Down
18 changes: 18 additions & 0 deletions pkg/types/aws/defaults/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@ import (
"github.com/openshift/installer/pkg/types/aws"
)

var (
defaultMachineClass = map[string]string{
"eu-north-1": "m5",
"eu-west-3": "m5",
"us-gov-east-1": "m5",
}
)

// SetPlatformDefaults sets the defaults for the platform.
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.
func InstanceClass(region string) string {
if class, ok := defaultMachineClass[region]; ok {
return class
}
return "m4"
}
3 changes: 3 additions & 0 deletions pkg/types/aws/validation/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,15 @@ var (
"cn-north-1": "Beijing",
"cn-northwest-1": "Ningxia",
"eu-central-1": "Frankfurt",
"eu-north-1": "Stockholm",
"eu-west-1": "Ireland",
"eu-west-2": "London",
"eu-west-3": "Paris",
"sa-east-1": "São Paulo",
"us-east-1": "N. Virginia",
"us-east-2": "Ohio",
"us-gov-east-1": "AWS GovCloud (US-East)",
"us-gov-west-1": "AWS GovCloud (US-West)",
"us-west-1": "N. California",
"us-west-2": "Oregon",
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/types/validation/installconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func TestValidateInstallConfig(t *testing.T) {
}
return c
}(),
expectedError: `^platform\.aws\.region: Unsupported value: "": supported values: "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2", "ca-central-1", "cn-north-1", "cn-northwest-1", "eu-central-1", "eu-west-1", "eu-west-2", "eu-west-3", "sa-east-1", "us-east-1", "us-east-2", "us-west-1", "us-west-2"$`,
expectedError: `^platform\.aws\.region: Unsupported value: "": supported values: "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2", "ca-central-1", "cn-north-1", "cn-northwest-1", "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3", "sa-east-1", "us-east-1", "us-east-2", "us-gov-east-1", "us-gov-west-1", "us-west-1", "us-west-2"$`,
},
{
name: "valid libvirt platform",
Expand Down
9 changes: 9 additions & 0 deletions platformtests/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Platform Tests

This directory contains test suites checking per-platform assumptions.
These tests require platform access that is not appropriate for platform-agnostic unit tests.
The `t` directory name (unlike `tests`) allows us to share [the project `vendor` directory managed by `dep`](../docs/dev/dependencies.md#go).

Platforms:

* [aws](aws)
12 changes: 12 additions & 0 deletions platformtests/aws/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# AWS Tests

This directory contains test suites checking AWS-specific assumptions.
Run with:

```console
$ AWS_PROFILE=your-profile go test .
```

or similar (it needs access to [your AWS credentials][credentials]).

[credentials]: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html
110 changes: 110 additions & 0 deletions platformtests/aws/default_instance_class_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package aws

import (
"strings"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/pricing"
awsutil "github.com/openshift/installer/pkg/asset/installconfig/aws"
"github.com/openshift/installer/pkg/types/aws/defaults"
"github.com/openshift/installer/pkg/types/aws/validation"
"github.com/stretchr/testify/assert"
)

func TestGetDefaultInstanceClass(t *testing.T) {
ssn, err := awsutil.GetSession()
if err != nil {
t.Fatal(err)
}

exists := struct{}{}
instanceClasses := map[string]map[string]struct{}{}

client := pricing.New(ssn, aws.NewConfig().WithRegion("us-east-1"))

err = client.GetProductsPages(
&pricing.GetProductsInput{
ServiceCode: aws.String("AmazonEC2"),
Filters: []*pricing.Filter{
{
Field: aws.String("tenancy"),
Type: aws.String("TERM_MATCH"),
Value: aws.String("Shared"),
},
{
Field: aws.String("productFamily"),
Type: aws.String("TERM_MATCH"),
Value: aws.String("Compute Instance"),
},
{
Field: aws.String("operatingSystem"),
Type: aws.String("TERM_MATCH"),
Value: aws.String("Linux"),
},
{
Field: aws.String("instanceFamily"),
Type: aws.String("TERM_MATCH"),
Value: aws.String("General purpose"),
},
},
},
func(result *pricing.GetProductsOutput, lastPage bool) bool {
for _, priceList := range result.PriceList {
product := priceList["product"].(map[string]interface{})
attr := product["attributes"].(map[string]interface{})
location := attr["location"].(string)
instanceType := attr["instanceType"].(string)
instanceClassSlice := strings.Split(instanceType, ".")
instanceClass := instanceClassSlice[0]
_, ok := instanceClasses[location]
if ok {
instanceClasses[location][instanceClass] = exists
} else {
instanceClasses[location] = map[string]struct{}{instanceClass: exists}
}
}
return !lastPage
},
)
if err != nil {
t.Fatal(err)
}

regions := map[string]string{ // seed with locations that don't match AWS's usual names
"South America (Sao Paulo)": "sa-east-1",
"AWS GovCloud (US)": "us-gov-west-1",
}

for location, classes := range instanceClasses {
t.Run(location, func(t *testing.T) {
region, ok := regions[location]
if !ok {
for slug, name := range validation.Regions {
if strings.Contains(location, name) {
regions[location] = slug
region = slug
break
}
}
if region == "" {
t.Fatal("not a recognized region")
}
}

class := ""
// ordered list of prefered instance classes
for _, preferredClass := range []string{"m4", "m5"} {
if _, ok := classes[preferredClass]; ok {
class = preferredClass
break
}
}
if class == "" {
t.Fatalf("does not support any preferred classes: %v", classes)
}
defaultClass := defaults.InstanceClass(region)
assert.Equal(t, defaultClass, class)
})
}
}

0 comments on commit de982f1

Please sign in to comment.