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.

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

Support regions without m4 machines #1163

Merged
merged 2 commits into from
Feb 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 3 additions & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe RHEL has signed off on the M5 series. We will need to figure out which machine class we want to use as a fallback.

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe RHEL has signed off on the M5 series. We will need to figure out which machine class we want to use as a fallback.

I thought someone tested m5 somewhere :p. But whatever, it's easy to slot in a different alternative if you know what it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eric and Clayton think we should go with M5 for now. Ignore me!

"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)",
Copy link
Member Author

Choose a reason for hiding this comment

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

You sure about this? The pricing API calls this AWS GovCloud (US) I think...

Copy link
Member

Choose a reason for hiding this comment

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

The pricing API calls this AWS GovCloud (US) I think...

It does, but I have an entry for decoding that here. They use US-West in other places, and now that US-East exists, I think the qualified name makes more sense here.

"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)
})
}
}
Loading