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) + }) + } +}