From de982f13c64fc65671696e3e86f51edd4e9b7532 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Thu, 31 Jan 2019 16:05:54 -0500 Subject: [PATCH] pkg/types/aws/defaults/platform: Per-region instance-class defaults 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]: https://github.com/openshift/installer/pull/552#issuecomment-459892007 --- data/data/aws/bootstrap/variables.tf | 3 +- data/data/aws/main.tf | 7 +- data/data/aws/master/main.tf | 2 +- data/data/aws/master/variables.tf | 2 +- data/data/aws/variables-aws.tf | 9 +- pkg/asset/machines/master.go | 9 +- pkg/asset/machines/worker.go | 9 +- pkg/tfvars/aws/aws.go | 32 ++--- pkg/types/aws/defaults/platform.go | 18 +++ pkg/types/aws/validation/platform.go | 3 + pkg/types/validation/installconfig_test.go | 2 +- platformtests/README.md | 9 ++ platformtests/aws/README.md | 12 ++ .../aws/default_instance_class_test.go | 110 ++++++++++++++++++ 14 files changed, 202 insertions(+), 25 deletions(-) create mode 100644 platformtests/README.md create mode 100644 platformtests/aws/README.md create mode 100644 platformtests/aws/default_instance_class_test.go diff --git a/data/data/aws/bootstrap/variables.tf b/data/data/aws/bootstrap/variables.tf index faed48e1fbe..3e93ae85fce 100644 --- a/data/data/aws/bootstrap/variables.tf +++ b/data/data/aws/bootstrap/variables.tf @@ -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" { diff --git a/data/data/aws/main.tf b/data/data/aws/main.tf index 14161edd4ba..7bed29d893e 100644 --- a/data/data/aws/main.tf +++ b/data/data/aws/main.tf @@ -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]}" @@ -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", diff --git a/data/data/aws/master/main.tf b/data/data/aws/master/main.tf index 232c8594601..54404822ba1 100644 --- a/data/data/aws/master/main.tf +++ b/data/data/aws/master/main.tf @@ -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}" diff --git a/data/data/aws/master/variables.tf b/data/data/aws/master/variables.tf index 6f536718b8b..050f93f4721 100644 --- a/data/data/aws/master/variables.tf +++ b/data/data/aws/master/variables.tf @@ -11,7 +11,7 @@ variable "dns_server_ip" { default = "" } -variable "ec2_type" { +variable "instance_type" { type = "string" } diff --git a/data/data/aws/variables-aws.tf b/data/data/aws/variables-aws.tf index 630e3eb65d2..e7530e54374 100644 --- a/data/data/aws/variables-aws.tf +++ b/data/data/aws/variables-aws.tf @@ -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" { diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go index 4ab7a5aa05f..14914d865c9 100644 --- a/pkg/asset/machines/master.go +++ b/pkg/asset/machines/master.go @@ -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" @@ -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{} @@ -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 { diff --git a/pkg/asset/machines/worker.go b/pkg/asset/machines/worker.go index 44b3e781ef7..fad164cd840 100644 --- a/pkg/asset/machines/worker.go +++ b/pkg/asset/machines/worker.go @@ -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" @@ -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{} @@ -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 { diff --git a/pkg/tfvars/aws/aws.go b/pkg/tfvars/aws/aws.go index 2e932158048..a5fb07c4a88 100644 --- a/pkg/tfvars/aws/aws.go +++ b/pkg/tfvars/aws/aws.go @@ -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. @@ -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 { diff --git a/pkg/types/aws/defaults/platform.go b/pkg/types/aws/defaults/platform.go index 273ae07bdc9..08f986734ee 100644 --- a/pkg/types/aws/defaults/platform.go +++ b/pkg/types/aws/defaults/platform.go @@ -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" +} diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go index a231372b9bd..f2f2c058867 100644 --- a/pkg/types/aws/validation/platform.go +++ b/pkg/types/aws/validation/platform.go @@ -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", } diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 900104a1760..f91b895eff8 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -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", diff --git a/platformtests/README.md b/platformtests/README.md new file mode 100644 index 00000000000..9f05880656f --- /dev/null +++ b/platformtests/README.md @@ -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) diff --git a/platformtests/aws/README.md b/platformtests/aws/README.md new file mode 100644 index 00000000000..e090126a376 --- /dev/null +++ b/platformtests/aws/README.md @@ -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 diff --git a/platformtests/aws/default_instance_class_test.go b/platformtests/aws/default_instance_class_test.go new file mode 100644 index 00000000000..b1b78440144 --- /dev/null +++ b/platformtests/aws/default_instance_class_test.go @@ -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) + }) + } +}