From aa4fd8913e1d383b91c7131739aa7567fdb97ac1 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Mon, 3 Dec 2018 22:02:09 +0000 Subject: [PATCH] openstack: Add flavor selection support. OpenStack flavors (analogous to AWS instance types) are not standardized, so we can't pick a default that works across all environments. This patch adds this as one of the items that must be specified. --- data/data/openstack/bootstrap/variables.tf | 1 - data/data/openstack/variables-openstack.tf | 1 - .../installconfig/openstack/openstack.go | 30 +++++++++++++++++- pkg/asset/machines/master.go | 2 +- pkg/asset/machines/worker.go | 6 ++-- pkg/asset/mock/filefetcher_generated.go | 4 +++ pkg/tfvars/tfvars.go | 1 + pkg/types/openstack/platform.go | 4 +++ .../mock/validvaluesfetcher_generated.go | 23 ++++++++++++++ pkg/types/openstack/validation/platform.go | 6 ++++ .../openstack/validation/platform_test.go | 17 ++++++++++ .../validation/realvalidvaluesfetcher.go | 31 +++++++++++++++++++ .../validation/validvaluesfetcher.go | 2 ++ pkg/types/validation/installconfig_test.go | 2 ++ 14 files changed, 123 insertions(+), 7 deletions(-) diff --git a/data/data/openstack/bootstrap/variables.tf b/data/data/openstack/bootstrap/variables.tf index 6780112caa9..e913a63adf3 100644 --- a/data/data/openstack/bootstrap/variables.tf +++ b/data/data/openstack/bootstrap/variables.tf @@ -24,7 +24,6 @@ variable "ignition" { variable "flavor_name" { type = "string" - default = "m1.medium" description = "The Nova flavor for the bootstrap node." } diff --git a/data/data/openstack/variables-openstack.tf b/data/data/openstack/variables-openstack.tf index b62cafe8bb7..4b02f7b4fa8 100644 --- a/data/data/openstack/variables-openstack.tf +++ b/data/data/openstack/variables-openstack.tf @@ -236,7 +236,6 @@ EOF variable "openstack_master_flavor_name" { type = "string" - default = "m1.medium" description = "Instance size for the master node(s). Example: `m1.medium`." } diff --git a/pkg/asset/installconfig/openstack/openstack.go b/pkg/asset/installconfig/openstack/openstack.go index 21135f553a4..01f4232267b 100644 --- a/pkg/asset/installconfig/openstack/openstack.go +++ b/pkg/asset/installconfig/openstack/openstack.go @@ -129,7 +129,34 @@ func Platform() (*openstack.Platform, error) { }, }, &extNet) if err != nil { - return nil, errors.Wrapf(err, "failed to Marshal %s platform", openstack.Name) + return nil, err + } + + flavorNames, err := validValuesFetcher.GetFlavorNames(cloud) + if err != nil { + return nil, err + } + sort.Strings(flavorNames) + var flavor string + err = survey.Ask([]*survey.Question{ + { + Prompt: &survey.Select{ + Message: "FlavorName", + Help: "The OpenStack compute flavor to use for servers. A flavor with at least 4 GB RAM is recommended.", + Options: flavorNames, + }, + Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { + value := ans.(string) + i := sort.SearchStrings(flavorNames, value) + if i == len(flavorNames) || flavorNames[i] != value { + return errors.Errorf("invalid flavor name %q, should be one of %+v", value, strings.Join(flavorNames, ", ")) + } + return nil + }), + }, + }, &flavor) + if err != nil { + return nil, err } return &openstack.Platform{ @@ -138,5 +165,6 @@ func Platform() (*openstack.Platform, error) { BaseImage: image, Cloud: cloud, ExternalNetwork: extNet, + FlavorName: flavor, }, nil } diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go index 100eb08806e..a409569a174 100644 --- a/pkg/asset/machines/master.go +++ b/pkg/asset/machines/master.go @@ -118,7 +118,7 @@ func (m *Master) Generate(dependencies asset.Parents) error { Instances: instances, Image: ic.Platform.OpenStack.BaseImage, Region: ic.Platform.OpenStack.Region, - Machine: defaultOpenStackMachinePoolPlatform(), + Machine: defaultOpenStackMachinePoolPlatform(ic.Platform.OpenStack.FlavorName), } tags := map[string]string{ diff --git a/pkg/asset/machines/worker.go b/pkg/asset/machines/worker.go index 69e7faa3b81..c413a115b14 100644 --- a/pkg/asset/machines/worker.go +++ b/pkg/asset/machines/worker.go @@ -31,9 +31,9 @@ func defaultAWSMachinePoolPlatform() awstypes.MachinePool { } } -func defaultOpenStackMachinePoolPlatform() openstacktypes.MachinePool { +func defaultOpenStackMachinePoolPlatform(flavor string) openstacktypes.MachinePool { return openstacktypes.MachinePool{ - FlavorName: "m1.medium", + FlavorName: flavor, } } @@ -130,7 +130,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error { Replicas: numOfWorkers, Image: ic.Platform.OpenStack.BaseImage, Region: ic.Platform.OpenStack.Region, - Machine: defaultOpenStackMachinePoolPlatform(), + Machine: defaultOpenStackMachinePoolPlatform(ic.Platform.OpenStack.FlavorName), } tags := map[string]string{ diff --git a/pkg/asset/mock/filefetcher_generated.go b/pkg/asset/mock/filefetcher_generated.go index 21b0c0dad8e..369c45c2a77 100644 --- a/pkg/asset/mock/filefetcher_generated.go +++ b/pkg/asset/mock/filefetcher_generated.go @@ -35,6 +35,7 @@ func (m *MockFileFetcher) EXPECT() *MockFileFetcherMockRecorder { // FetchByName mocks base method func (m *MockFileFetcher) FetchByName(arg0 string) (*asset.File, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchByName", arg0) ret0, _ := ret[0].(*asset.File) ret1, _ := ret[1].(error) @@ -43,11 +44,13 @@ func (m *MockFileFetcher) FetchByName(arg0 string) (*asset.File, error) { // FetchByName indicates an expected call of FetchByName func (mr *MockFileFetcherMockRecorder) FetchByName(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchByName", reflect.TypeOf((*MockFileFetcher)(nil).FetchByName), arg0) } // FetchByPattern mocks base method func (m *MockFileFetcher) FetchByPattern(pattern string) ([]*asset.File, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchByPattern", pattern) ret0, _ := ret[0].([]*asset.File) ret1, _ := ret[1].(error) @@ -56,5 +59,6 @@ func (m *MockFileFetcher) FetchByPattern(pattern string) ([]*asset.File, error) // FetchByPattern indicates an expected call of FetchByPattern func (mr *MockFileFetcherMockRecorder) FetchByPattern(pattern interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchByPattern", reflect.TypeOf((*MockFileFetcher)(nil).FetchByPattern), pattern) } diff --git a/pkg/tfvars/tfvars.go b/pkg/tfvars/tfvars.go index 4a0122f1cd7..0f159d5ee87 100644 --- a/pkg/tfvars/tfvars.go +++ b/pkg/tfvars/tfvars.go @@ -120,6 +120,7 @@ func TFVars(cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, e } config.OpenStack.Credentials.Cloud = cfg.Platform.OpenStack.Cloud config.OpenStack.ExternalNetwork = cfg.Platform.OpenStack.ExternalNetwork + config.OpenStack.Master.FlavorName = cfg.Platform.OpenStack.FlavorName } return json.MarshalIndent(config, "", " ") diff --git a/pkg/types/openstack/platform.go b/pkg/types/openstack/platform.go index f8dcb670c55..10a5e8a8499 100644 --- a/pkg/types/openstack/platform.go +++ b/pkg/types/openstack/platform.go @@ -29,4 +29,8 @@ type Platform struct { // ExternalNetwork // The OpenStack external network to be used for installation. ExternalNetwork string `json:"externalNetwork"` + + // FlavorName + // The OpenStack compute flavor to use for servers. + FlavorName string `json:"computeFlavor"` } diff --git a/pkg/types/openstack/validation/mock/validvaluesfetcher_generated.go b/pkg/types/openstack/validation/mock/validvaluesfetcher_generated.go index 57f5f3d252e..420ecf0f4d7 100644 --- a/pkg/types/openstack/validation/mock/validvaluesfetcher_generated.go +++ b/pkg/types/openstack/validation/mock/validvaluesfetcher_generated.go @@ -34,6 +34,7 @@ func (m *MockValidValuesFetcher) EXPECT() *MockValidValuesFetcherMockRecorder { // GetCloudNames mocks base method func (m *MockValidValuesFetcher) GetCloudNames() ([]string, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetCloudNames") ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) @@ -42,11 +43,13 @@ func (m *MockValidValuesFetcher) GetCloudNames() ([]string, error) { // GetCloudNames indicates an expected call of GetCloudNames func (mr *MockValidValuesFetcherMockRecorder) GetCloudNames() *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCloudNames", reflect.TypeOf((*MockValidValuesFetcher)(nil).GetCloudNames)) } // GetRegionNames mocks base method func (m *MockValidValuesFetcher) GetRegionNames(cloud string) ([]string, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetRegionNames", cloud) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) @@ -55,11 +58,13 @@ func (m *MockValidValuesFetcher) GetRegionNames(cloud string) ([]string, error) // GetRegionNames indicates an expected call of GetRegionNames func (mr *MockValidValuesFetcherMockRecorder) GetRegionNames(cloud interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRegionNames", reflect.TypeOf((*MockValidValuesFetcher)(nil).GetRegionNames), cloud) } // GetImageNames mocks base method func (m *MockValidValuesFetcher) GetImageNames(cloud string) ([]string, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetImageNames", cloud) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) @@ -68,11 +73,13 @@ func (m *MockValidValuesFetcher) GetImageNames(cloud string) ([]string, error) { // GetImageNames indicates an expected call of GetImageNames func (mr *MockValidValuesFetcherMockRecorder) GetImageNames(cloud interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetImageNames", reflect.TypeOf((*MockValidValuesFetcher)(nil).GetImageNames), cloud) } // GetNetworkNames mocks base method func (m *MockValidValuesFetcher) GetNetworkNames(cloud string) ([]string, error) { + m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetNetworkNames", cloud) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) @@ -81,5 +88,21 @@ func (m *MockValidValuesFetcher) GetNetworkNames(cloud string) ([]string, error) // GetNetworkNames indicates an expected call of GetNetworkNames func (mr *MockValidValuesFetcherMockRecorder) GetNetworkNames(cloud interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkNames", reflect.TypeOf((*MockValidValuesFetcher)(nil).GetNetworkNames), cloud) } + +// GetFlavorNames mocks base method +func (m *MockValidValuesFetcher) GetFlavorNames(cloud string) ([]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetFlavorNames", cloud) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetFlavorNames indicates an expected call of GetFlavorNames +func (mr *MockValidValuesFetcherMockRecorder) GetFlavorNames(cloud interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetFlavorNames", reflect.TypeOf((*MockValidValuesFetcher)(nil).GetFlavorNames), cloud) +} diff --git a/pkg/types/openstack/validation/platform.go b/pkg/types/openstack/validation/platform.go index 77f17ab6a9b..30a6923c7a9 100644 --- a/pkg/types/openstack/validation/platform.go +++ b/pkg/types/openstack/validation/platform.go @@ -36,6 +36,12 @@ func ValidatePlatform(p *openstack.Platform, fldPath *field.Path, fetcher ValidV } else if !isValidValue(p.ExternalNetwork, validNetworks) { allErrs = append(allErrs, field.NotSupported(fldPath.Child("externalNetwork"), p.ExternalNetwork, validNetworks)) } + validFlavors, err := fetcher.GetFlavorNames(p.Cloud) + if err != nil { + allErrs = append(allErrs, field.InternalError(fldPath.Child("computeFlavor"), errors.New("could not retrieve valid flavors"))) + } else if !isValidValue(p.FlavorName, validFlavors) { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("computeFlavor"), p.FlavorName, validFlavors)) + } } if p.DefaultMachinePlatform != nil { allErrs = append(allErrs, ValidateMachinePool(p.DefaultMachinePlatform, fldPath.Child("defaultMachinePlatform"))...) diff --git a/pkg/types/openstack/validation/platform_test.go b/pkg/types/openstack/validation/platform_test.go index 10137882410..650904b1a8d 100644 --- a/pkg/types/openstack/validation/platform_test.go +++ b/pkg/types/openstack/validation/platform_test.go @@ -20,6 +20,7 @@ func validPlatform() *openstack.Platform { BaseImage: "test-image", Cloud: "test-cloud", ExternalNetwork: "test-network", + FlavorName: "test-flavor", } } @@ -31,6 +32,7 @@ func TestValidatePlatform(t *testing.T) { noRegions bool noImages bool noNetworks bool + noFlavors bool valid bool }{ { @@ -107,6 +109,12 @@ func TestValidatePlatform(t *testing.T) { noNetworks: true, valid: false, }, + { + name: "flavors fetch failure", + platform: validPlatform(), + noFlavors: true, + valid: false, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -148,6 +156,15 @@ func TestValidatePlatform(t *testing.T) { Return([]string{"test-network"}, nil). MaxTimes(1) } + if tc.noFlavors { + fetcher.EXPECT().GetFlavorNames(tc.platform.Cloud). + Return(nil, errors.New("no flavors")). + MaxTimes(1) + } else { + fetcher.EXPECT().GetFlavorNames(tc.platform.Cloud). + Return([]string{"test-flavor"}, nil). + MaxTimes(1) + } err := ValidatePlatform(tc.platform, field.NewPath("test-path"), fetcher).ToAggregate() if tc.valid { assert.NoError(t, err) diff --git a/pkg/types/openstack/validation/realvalidvaluesfetcher.go b/pkg/types/openstack/validation/realvalidvaluesfetcher.go index b0f67c6d5e9..9084dac1589 100644 --- a/pkg/types/openstack/validation/realvalidvaluesfetcher.go +++ b/pkg/types/openstack/validation/realvalidvaluesfetcher.go @@ -1,6 +1,7 @@ package validation import ( + "github.com/gophercloud/gophercloud/openstack/compute/v2/flavors" "github.com/gophercloud/gophercloud/openstack/identity/v3/regions" "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" @@ -118,3 +119,33 @@ func (f realValidValuesFetcher) GetNetworkNames(cloud string) ([]string, error) return networkNames, nil } + +// GetFlavorNames gets a list of valid flavor names. +func (f realValidValuesFetcher) GetFlavorNames(cloud string) ([]string, error) { + opts := &clientconfig.ClientOpts{ + Cloud: cloud, + } + + conn, err := clientconfig.NewServiceClient("compute", opts) + if err != nil { + return nil, err + } + + listOpts := flavors.ListOpts{} + allPages, err := flavors.ListDetail(conn, listOpts).AllPages() + if err != nil { + return nil, err + } + + allFlavors, err := flavors.ExtractFlavors(allPages) + if err != nil { + return nil, err + } + + flavorNames := make([]string, len(allFlavors)) + for i, flavor := range allFlavors { + flavorNames[i] = flavor.Name + } + + return flavorNames, nil +} diff --git a/pkg/types/openstack/validation/validvaluesfetcher.go b/pkg/types/openstack/validation/validvaluesfetcher.go index e4bf689b525..9d4bcf69165 100644 --- a/pkg/types/openstack/validation/validvaluesfetcher.go +++ b/pkg/types/openstack/validation/validvaluesfetcher.go @@ -12,4 +12,6 @@ type ValidValuesFetcher interface { GetImageNames(cloud string) ([]string, error) // GetNetworkNames gets the valid network names. GetNetworkNames(cloud string) ([]string, error) + // GetFlavorNames gets the valid flavor names. + GetFlavorNames(cloud string) ([]string, error) } diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 52264f172b7..cb29cb49489 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -259,6 +259,7 @@ func TestValidateInstallConfig(t *testing.T) { BaseImage: "test-image", Cloud: "test-cloud", ExternalNetwork: "test-network", + FlavorName: "test-flavor", }, } return c @@ -287,6 +288,7 @@ func TestValidateInstallConfig(t *testing.T) { fetcher.EXPECT().GetRegionNames(gomock.Any()).Return([]string{"test-region"}, nil).AnyTimes() fetcher.EXPECT().GetImageNames(gomock.Any()).Return([]string{"test-image"}, nil).AnyTimes() fetcher.EXPECT().GetNetworkNames(gomock.Any()).Return([]string{"test-network"}, nil).AnyTimes() + fetcher.EXPECT().GetFlavorNames(gomock.Any()).Return([]string{"test-flavor"}, nil).AnyTimes() err := ValidateInstallConfig(tc.installConfig, fetcher).ToAggregate() if tc.valid {