From 95c9d3840e552b353bbb7f565344782168278411 Mon Sep 17 00:00:00 2001 From: staebler Date: Tue, 26 Feb 2019 13:26:43 -0500 Subject: [PATCH] Add the Hyperthreading field to MachinePool which allows the user to enable or disable hyperthreading for machines. The default is for hyperthreading to be enabled. RHCOS ships with pivot.service that uses the `/etc/pivot/kernel-args` to override the kernel arguments for hosts. Adding `nosmt` kernel argument switches hyperthreading off. Add MachineConfig to disable hyperthreading for control plane and compute that have the hyperthreading option disabled. --- pkg/asset/installconfig/installconfig_test.go | 30 +++-- .../machines/machineconfig/hyperthreading.go | 40 +++++++ pkg/asset/machines/master.go | 22 ++-- pkg/asset/machines/master_test.go | 108 +++++++++++++++++- pkg/asset/machines/worker.go | 7 +- pkg/asset/machines/worker_test.go | 108 +++++++++++++++++- pkg/types/defaults/installconfig.go | 22 +--- pkg/types/defaults/installconfig_test.go | 26 ++--- pkg/types/defaults/machinepools.go | 20 ++++ pkg/types/defaults/machinepools_test.go | 80 +++++++++++++ pkg/types/machinepools.go | 18 ++- pkg/types/validation/installconfig_test.go | 43 +++---- pkg/types/validation/machinepools.go | 18 +++ pkg/types/validation/machinepools_test.go | 25 ++-- 14 files changed, 464 insertions(+), 103 deletions(-) create mode 100644 pkg/asset/machines/machineconfig/hyperthreading.go create mode 100644 pkg/types/defaults/machinepools.go create mode 100644 pkg/types/defaults/machinepools_test.go diff --git a/pkg/asset/installconfig/installconfig_test.go b/pkg/asset/installconfig/installconfig_test.go index 5c02065c5bd..d58728e23ad 100644 --- a/pkg/asset/installconfig/installconfig_test.go +++ b/pkg/asset/installconfig/installconfig_test.go @@ -76,13 +76,15 @@ func TestInstallConfigGenerate_FillsInDefaults(t *testing.T) { }, }, ControlPlane: &types.MachinePool{ - Name: "master", - Replicas: pointer.Int64Ptr(3), + Name: "master", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, Compute: []types.MachinePool{ { - Name: "worker", - Replicas: pointer.Int64Ptr(3), + Name: "worker", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, }, Platform: types.Platform{ @@ -135,13 +137,15 @@ pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}" }, }, ControlPlane: &types.MachinePool{ - Name: "master", - Replicas: pointer.Int64Ptr(3), + Name: "master", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, Compute: []types.MachinePool{ { - Name: "worker", - Replicas: pointer.Int64Ptr(3), + Name: "worker", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, }, Platform: types.Platform{ @@ -214,13 +218,15 @@ network: }, }, ControlPlane: &types.MachinePool{ - Name: "master", - Replicas: pointer.Int64Ptr(3), + Name: "master", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, Compute: []types.MachinePool{ { - Name: "worker", - Replicas: pointer.Int64Ptr(3), + Name: "worker", + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, }, }, Platform: types.Platform{ diff --git a/pkg/asset/machines/machineconfig/hyperthreading.go b/pkg/asset/machines/machineconfig/hyperthreading.go new file mode 100644 index 00000000000..ef3662e88ef --- /dev/null +++ b/pkg/asset/machines/machineconfig/hyperthreading.go @@ -0,0 +1,40 @@ +package machineconfig + +import ( + "fmt" + + igntypes "github.com/coreos/ignition/config/v2_2/types" + mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/openshift/installer/pkg/asset/ignition" +) + +// ForHyperthreadingDisabled creates the MachineConfig to enable hyperthreading. +// RHCOS ships with pivot.service that uses the `/etc/pivot/kernel-args` to override the kernel arguments for hosts. +func ForHyperthreadingDisabled(role string) *mcfgv1.MachineConfig { + return &mcfgv1.MachineConfig{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "machineconfiguration.openshift.io/v1", + Kind: "MachineConfig", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("99-%s-hyperthreading", role), + Labels: map[string]string{ + "machineconfiguration.openshift.io/role": role, + }, + }, + Spec: mcfgv1.MachineConfigSpec{ + Config: igntypes.Config{ + Ignition: igntypes.Ignition{ + Version: igntypes.MaxVersion.String(), + }, + Storage: igntypes.Storage{ + Files: []igntypes.File{ + ignition.FileFromString("/etc/pivot/kernel-args", "root", 0600, "ADD nosmt"), + }, + }, + }, + }, + } +} diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go index 372dbe46e9f..c3f5ad5f5f2 100644 --- a/pkg/asset/machines/master.go +++ b/pkg/asset/machines/master.go @@ -9,6 +9,15 @@ import ( libvirtapi "github.com/openshift/cluster-api-provider-libvirt/pkg/apis" libvirtprovider "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1" machineapi "github.com/openshift/cluster-api/pkg/apis/machine/v1beta1" + mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + awsapi "sigs.k8s.io/cluster-api-provider-aws/pkg/apis" + awsprovider "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1beta1" + openstackapi "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis" + openstackprovider "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis/openstackproviderconfig/v1alpha1" + "github.com/openshift/installer/pkg/asset" "github.com/openshift/installer/pkg/asset/ignition/machine" "github.com/openshift/installer/pkg/asset/installconfig" @@ -17,20 +26,13 @@ import ( "github.com/openshift/installer/pkg/asset/machines/machineconfig" "github.com/openshift/installer/pkg/asset/machines/openstack" "github.com/openshift/installer/pkg/asset/rhcos" + "github.com/openshift/installer/pkg/types" 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" vspheretypes "github.com/openshift/installer/pkg/types/vsphere" - mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" - "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - awsapi "sigs.k8s.io/cluster-api-provider-aws/pkg/apis" - awsprovider "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1beta1" - openstackapi "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis" - openstackprovider "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis/openstackproviderconfig/v1alpha1" ) // Master generates the machines for the `master` machine pool. @@ -91,6 +93,7 @@ func (m *Master) Generate(dependencies asset.Parents) error { dependencies.Get(clusterID, installconfig, rhcosImage, mign) ic := installconfig.Config + pool := ic.ControlPlane var err error machines := []machineapi.Machine{} @@ -150,6 +153,9 @@ func (m *Master) Generate(dependencies asset.Parents) error { } machineConfigs := []*mcfgv1.MachineConfig{} + if pool.Hyperthreading == types.HyperthreadingDisabled { + machineConfigs = append(machineConfigs, machineconfig.ForHyperthreadingDisabled("master")) + } if ic.SSHKey != "" { machineConfigs = append(machineConfigs, machineconfig.ForAuthorizedKeys(ic.SSHKey, "master")) } diff --git a/pkg/asset/machines/master_test.go b/pkg/asset/machines/master_test.go index 99232f98fb2..19c02e52efa 100644 --- a/pkg/asset/machines/master_test.go +++ b/pkg/asset/machines/master_test.go @@ -19,17 +19,116 @@ func TestMasterGenerateMachineConfigs(t *testing.T) { cases := []struct { name string key string + hyperthreading types.HyperthreadingMode expectedMachineConfig string }{ { - name: "no key", + name: "no key hyperthreading enabled", + hyperthreading: types.HyperthreadingEnabled, }, { - name: "key present", - key: "ssh-rsa: dummy-key", + name: "key present hyperthreading enabled", + key: "ssh-rsa: dummy-key", + hyperthreading: types.HyperthreadingEnabled, expectedMachineConfig: `--- apiVersion: machineconfiguration.openshift.io/v1 kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: master + name: 99-master-ssh +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: + users: + - name: core + sshAuthorizedKeys: + - 'ssh-rsa: dummy-key' + storage: {} + systemd: {} + osImageURL: "" +`, + }, + { + name: "no key hyperthreading disabled", + hyperthreading: types.HyperthreadingDisabled, + expectedMachineConfig: `--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: master + name: 99-master-hyperthreading +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: {} + storage: + files: + - contents: + source: data:text/plain;charset=utf-8;base64,QUREIG5vc210 + verification: {} + filesystem: root + mode: 384 + path: /etc/pivot/kernel-args + user: + name: root + systemd: {} + osImageURL: "" +`, + }, + { + name: "key present hyperthreading disabled", + key: "ssh-rsa: dummy-key", + hyperthreading: types.HyperthreadingDisabled, + expectedMachineConfig: `--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: master + name: 99-master-hyperthreading +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: {} + storage: + files: + - contents: + source: data:text/plain;charset=utf-8;base64,QUREIG5vc210 + verification: {} + filesystem: root + mode: 384 + path: /etc/pivot/kernel-args + user: + name: root + systemd: {} + osImageURL: "" +--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig metadata: creationTimestamp: null labels: @@ -76,7 +175,8 @@ spec: }, }, ControlPlane: &types.MachinePool{ - Replicas: pointer.Int64Ptr(1), + Hyperthreading: tc.hyperthreading, + Replicas: pointer.Int64Ptr(1), Platform: types.MachinePoolPlatform{ AWS: &awstypes.MachinePool{ Zones: []string{"us-east-1a"}, diff --git a/pkg/asset/machines/worker.go b/pkg/asset/machines/worker.go index 968174a5cfd..7384da49605 100644 --- a/pkg/asset/machines/worker.go +++ b/pkg/asset/machines/worker.go @@ -26,6 +26,7 @@ import ( "github.com/openshift/installer/pkg/asset/machines/machineconfig" "github.com/openshift/installer/pkg/asset/machines/openstack" "github.com/openshift/installer/pkg/asset/rhcos" + "github.com/openshift/installer/pkg/types" 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" @@ -110,11 +111,16 @@ func (w *Worker) Generate(dependencies asset.Parents) error { machineConfigs := []*mcfgv1.MachineConfig{} machineSets := []runtime.Object{} + var err error ic := installconfig.Config for _, pool := range ic.Compute { + if pool.Hyperthreading == types.HyperthreadingDisabled { + machineConfigs = append(machineConfigs, machineconfig.ForHyperthreadingDisabled("worker")) + } if ic.SSHKey != "" { machineConfigs = append(machineConfigs, machineconfig.ForAuthorizedKeys(ic.SSHKey, "worker")) } + switch ic.Platform.Name() { case awstypes.Name: mpool := defaultAWSMachinePoolPlatform() @@ -196,7 +202,6 @@ func (w *Worker) Generate(dependencies asset.Parents) error { Data: data, } } - return nil } diff --git a/pkg/asset/machines/worker_test.go b/pkg/asset/machines/worker_test.go index 17e03f30e09..6eee5bde722 100644 --- a/pkg/asset/machines/worker_test.go +++ b/pkg/asset/machines/worker_test.go @@ -19,17 +19,116 @@ func TestWorkerGenerate(t *testing.T) { cases := []struct { name string key string + hyperthreading types.HyperthreadingMode expectedMachineConfig string }{ { - name: "no key", + name: "no key hyperthreading enabled", + hyperthreading: types.HyperthreadingEnabled, }, { - name: "key present", - key: "ssh-rsa: dummy-key", + name: "key present hyperthreading enabled", + key: "ssh-rsa: dummy-key", + hyperthreading: types.HyperthreadingEnabled, expectedMachineConfig: `--- apiVersion: machineconfiguration.openshift.io/v1 kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: worker + name: 99-worker-ssh +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: + users: + - name: core + sshAuthorizedKeys: + - 'ssh-rsa: dummy-key' + storage: {} + systemd: {} + osImageURL: "" +`, + }, + { + name: "no key hyperthreading disabled", + hyperthreading: types.HyperthreadingDisabled, + expectedMachineConfig: `--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: worker + name: 99-worker-hyperthreading +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: {} + storage: + files: + - contents: + source: data:text/plain;charset=utf-8;base64,QUREIG5vc210 + verification: {} + filesystem: root + mode: 384 + path: /etc/pivot/kernel-args + user: + name: root + systemd: {} + osImageURL: "" +`, + }, + { + name: "key present hyperthreading disabled", + key: "ssh-rsa: dummy-key", + hyperthreading: types.HyperthreadingDisabled, + expectedMachineConfig: `--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig +metadata: + creationTimestamp: null + labels: + machineconfiguration.openshift.io/role: worker + name: 99-worker-hyperthreading +spec: + config: + ignition: + config: {} + security: + tls: {} + timeouts: {} + version: 2.2.0 + networkd: {} + passwd: {} + storage: + files: + - contents: + source: data:text/plain;charset=utf-8;base64,QUREIG5vc210 + verification: {} + filesystem: root + mode: 384 + path: /etc/pivot/kernel-args + user: + name: root + systemd: {} + osImageURL: "" +--- +apiVersion: machineconfiguration.openshift.io/v1 +kind: MachineConfig metadata: creationTimestamp: null labels: @@ -77,7 +176,8 @@ spec: }, Compute: []types.MachinePool{ { - Replicas: pointer.Int64Ptr(1), + Replicas: pointer.Int64Ptr(1), + Hyperthreading: tc.hyperthreading, Platform: types.MachinePoolPlatform{ AWS: &awstypes.MachinePool{ Zones: []string{"us-east-1a"}, diff --git a/pkg/types/defaults/installconfig.go b/pkg/types/defaults/installconfig.go index fca1eefbdda..097c1a8eb7a 100644 --- a/pkg/types/defaults/installconfig.go +++ b/pkg/types/defaults/installconfig.go @@ -42,28 +42,16 @@ func SetInstallConfigDefaults(c *types.InstallConfig) { }, } } - defaultReplicaCount := int64(3) - if c.Platform.Libvirt != nil { - defaultReplicaCount = 1 - } if c.ControlPlane == nil { - c.ControlPlane = &types.MachinePool{ - Replicas: &defaultReplicaCount, - } + c.ControlPlane = &types.MachinePool{} } c.ControlPlane.Name = "master" + SetMachinePoolDefaults(c.ControlPlane, c.Platform.Name()) if len(c.Compute) == 0 { - c.Compute = []types.MachinePool{ - { - Name: "worker", - Replicas: &defaultReplicaCount, - }, - } + c.Compute = []types.MachinePool{{Name: "worker"}} } - for i, p := range c.Compute { - if p.Replicas == nil { - c.Compute[i].Replicas = &defaultReplicaCount - } + for i := range c.Compute { + SetMachinePoolDefaults(&c.Compute[i], c.Platform.Name()) } switch { case c.Platform.AWS != nil: diff --git a/pkg/types/defaults/installconfig_test.go b/pkg/types/defaults/installconfig_test.go index ac6587f6460..2bbff685559 100644 --- a/pkg/types/defaults/installconfig_test.go +++ b/pkg/types/defaults/installconfig_test.go @@ -31,16 +31,8 @@ func defaultInstallConfig() *types.InstallConfig { }, }, }, - ControlPlane: &types.MachinePool{ - Name: "master", - Replicas: pointer.Int64Ptr(3), - }, - Compute: []types.MachinePool{ - { - Name: "worker", - Replicas: pointer.Int64Ptr(3), - }, - }, + ControlPlane: defaultMachinePool("master"), + Compute: []types.MachinePool{*defaultMachinePool("worker")}, } } @@ -169,6 +161,13 @@ func TestSetInstallConfigDefaults(t *testing.T) { return c }(), }, + { + name: "control plane present", + config: &types.InstallConfig{ + ControlPlane: &types.MachinePool{}, + }, + expected: defaultInstallConfig(), + }, { name: "Compute present", config: &types.InstallConfig{ @@ -176,12 +175,7 @@ func TestSetInstallConfigDefaults(t *testing.T) { }, expected: func() *types.InstallConfig { c := defaultInstallConfig() - c.Compute = []types.MachinePool{ - { - Name: "test-compute", - Replicas: pointer.Int64Ptr(3), - }, - } + c.Compute = []types.MachinePool{*defaultMachinePool("test-compute")} return c }(), }, diff --git a/pkg/types/defaults/machinepools.go b/pkg/types/defaults/machinepools.go new file mode 100644 index 00000000000..e9810c2e9c5 --- /dev/null +++ b/pkg/types/defaults/machinepools.go @@ -0,0 +1,20 @@ +package defaults + +import ( + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/libvirt" +) + +// SetMachinePoolDefaults sets the defaults for the machine pool. +func SetMachinePoolDefaults(p *types.MachinePool, platform string) { + defaultReplicaCount := int64(3) + if platform == libvirt.Name { + defaultReplicaCount = 1 + } + if p.Replicas == nil { + p.Replicas = &defaultReplicaCount + } + if p.Hyperthreading == "" { + p.Hyperthreading = types.HyperthreadingEnabled + } +} diff --git a/pkg/types/defaults/machinepools_test.go b/pkg/types/defaults/machinepools_test.go new file mode 100644 index 00000000000..4746ca72df5 --- /dev/null +++ b/pkg/types/defaults/machinepools_test.go @@ -0,0 +1,80 @@ +package defaults + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/utils/pointer" + + "github.com/openshift/installer/pkg/types" +) + +func defaultMachinePool(name string) *types.MachinePool { + return &types.MachinePool{ + Name: name, + Replicas: pointer.Int64Ptr(3), + Hyperthreading: types.HyperthreadingEnabled, + } +} + +func TestSetMahcinePoolDefaults(t *testing.T) { + cases := []struct { + name string + pool *types.MachinePool + platform string + expected *types.MachinePool + }{ + { + name: "empty", + pool: &types.MachinePool{}, + expected: defaultMachinePool(""), + }, + { + name: "default", + pool: defaultMachinePool("test-name"), + expected: defaultMachinePool("test-name"), + }, + { + name: "non-default replicas", + pool: func() *types.MachinePool { + p := defaultMachinePool("test-name") + p.Replicas = pointer.Int64Ptr(5) + return p + }(), + expected: func() *types.MachinePool { + p := defaultMachinePool("test-name") + p.Replicas = pointer.Int64Ptr(5) + return p + }(), + }, + { + name: "libvirt replicas", + pool: &types.MachinePool{}, + platform: "libvirt", + expected: func() *types.MachinePool { + p := defaultMachinePool("") + p.Replicas = pointer.Int64Ptr(1) + return p + }(), + }, + { + name: "non-default hyperthreading", + pool: func() *types.MachinePool { + p := defaultMachinePool("test-name") + p.Hyperthreading = types.HyperthreadingMode("test-hyperthreading") + return p + }(), + expected: func() *types.MachinePool { + p := defaultMachinePool("test-name") + p.Hyperthreading = types.HyperthreadingMode("test-hyperthreading") + return p + }(), + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + SetMachinePoolDefaults(tc.pool, tc.platform) + assert.Equal(t, tc.expected, tc.pool, "unexpected machine pool") + }) + } +} diff --git a/pkg/types/machinepools.go b/pkg/types/machinepools.go index eb61485f430..b6844d4aee3 100644 --- a/pkg/types/machinepools.go +++ b/pkg/types/machinepools.go @@ -7,6 +7,16 @@ import ( "github.com/openshift/installer/pkg/types/vsphere" ) +// HyperthreadingMode is the mode of hyperthreading for a machine. +type HyperthreadingMode string + +const ( + // HyperthreadingEnabled indicates that hyperthreading is enabled. + HyperthreadingEnabled HyperthreadingMode = "Enabled" + // HyperthreadingDisabled indicates that hyperthreading is disabled. + HyperthreadingDisabled HyperthreadingMode = "Disabled" +) + // MachinePool is a pool of machines to be installed. type MachinePool struct { // Name is the name of the machine pool. @@ -17,8 +27,14 @@ type MachinePool struct { // Replicas is the count of machines for this machine pool. Replicas *int64 `json:"replicas,omitempty"` - // Platform is configuration for machine pool specific to the platfrom. + // Platform is configuration for machine pool specific to the platform. Platform MachinePoolPlatform `json:"platform"` + + // Hyperthreading determines the mode of hyperthreading that machines in this + // pool will utilize. + // +optional + // Default is for hyperthreading to be enabled. + Hyperthreading HyperthreadingMode `json:"hyperthreading,omitempty"` } // MachinePoolPlatform is the platform-specific configuration for a machine diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index b8241881daf..af5a8d5918f 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -39,16 +39,8 @@ func validInstallConfig() *types.InstallConfig { }, }, }, - ControlPlane: &types.MachinePool{ - Name: "master", - Replicas: pointer.Int64Ptr(3), - }, - Compute: []types.MachinePool{ - { - Name: "worker", - Replicas: pointer.Int64Ptr(3), - }, - }, + ControlPlane: validMachinePool("master"), + Compute: []types.MachinePool{*validMachinePool("worker")}, Platform: types.Platform{ AWS: validAWSPlatform(), }, @@ -302,14 +294,8 @@ func TestValidateInstallConfig(t *testing.T) { installConfig: func() *types.InstallConfig { c := validInstallConfig() c.Compute = []types.MachinePool{ - { - Name: "worker", - Replicas: pointer.Int64Ptr(1), - }, - { - Name: "worker", - Replicas: pointer.Int64Ptr(2), - }, + *validMachinePool("worker"), + *validMachinePool("worker"), } return c }(), @@ -320,10 +306,11 @@ func TestValidateInstallConfig(t *testing.T) { installConfig: func() *types.InstallConfig { c := validInstallConfig() c.Compute = []types.MachinePool{ - { - Name: "worker", - Replicas: pointer.Int64Ptr(0), - }, + func() types.MachinePool { + p := *validMachinePool("worker") + p.Replicas = pointer.Int64Ptr(0) + return p + }(), } return c }(), @@ -333,13 +320,13 @@ func TestValidateInstallConfig(t *testing.T) { installConfig: func() *types.InstallConfig { c := validInstallConfig() c.Compute = []types.MachinePool{ - { - Name: "worker", - Replicas: pointer.Int64Ptr(3), - Platform: types.MachinePoolPlatform{ + func() types.MachinePool { + p := *validMachinePool("worker") + p.Platform = types.MachinePoolPlatform{ OpenStack: &openstack.MachinePool{}, - }, - }, + } + return p + }(), } return c }(), diff --git a/pkg/types/validation/machinepools.go b/pkg/types/validation/machinepools.go index aad285fca28..2196e666100 100644 --- a/pkg/types/validation/machinepools.go +++ b/pkg/types/validation/machinepools.go @@ -14,6 +14,21 @@ import ( openstackvalidation "github.com/openshift/installer/pkg/types/openstack/validation" ) +var ( + validHyperthreadingModes = map[types.HyperthreadingMode]bool{ + types.HyperthreadingDisabled: true, + types.HyperthreadingEnabled: true, + } + + validHyperthreadingModeValues = func() []string { + v := make([]string, 0, len(validHyperthreadingModes)) + for m := range validHyperthreadingModes { + v = append(v, string(m)) + } + return v + }() +) + // ValidateMachinePool checks that the specified machine pool is valid. func ValidateMachinePool(platform *types.Platform, p *types.MachinePool, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -24,6 +39,9 @@ func ValidateMachinePool(platform *types.Platform, p *types.MachinePool, fldPath } else { allErrs = append(allErrs, field.Required(fldPath.Child("replicas"), "replicas is required")) } + if !validHyperthreadingModes[p.Hyperthreading] { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("hyperthreading"), p.Hyperthreading, validHyperthreadingModeValues)) + } allErrs = append(allErrs, validateMachinePoolPlatform(platform, &p.Platform, fldPath.Child("platform"))...) return allErrs } diff --git a/pkg/types/validation/machinepools_test.go b/pkg/types/validation/machinepools_test.go index 572c4de3522..6c0e3b55808 100644 --- a/pkg/types/validation/machinepools_test.go +++ b/pkg/types/validation/machinepools_test.go @@ -13,10 +13,11 @@ import ( "github.com/openshift/installer/pkg/types/openstack" ) -func validMachinePool() *types.MachinePool { +func validMachinePool(name string) *types.MachinePool { return &types.MachinePool{ - Name: "test-pool", - Replicas: pointer.Int64Ptr(1), + Name: name, + Replicas: pointer.Int64Ptr(1), + Hyperthreading: types.HyperthreadingDisabled, } } @@ -30,14 +31,14 @@ func TestValidateMachinePool(t *testing.T) { { name: "minimal", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, - pool: validMachinePool(), + pool: validMachinePool("test-name"), valid: true, }, { name: "missing replicas", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Replicas = nil return p }(), @@ -47,7 +48,7 @@ func TestValidateMachinePool(t *testing.T) { name: "invalid replicas", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Replicas = pointer.Int64Ptr(-1) return p }(), @@ -57,7 +58,7 @@ func TestValidateMachinePool(t *testing.T) { name: "valid aws", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ AWS: &aws.MachinePool{}, } @@ -69,7 +70,7 @@ func TestValidateMachinePool(t *testing.T) { name: "invalid aws", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ AWS: &aws.MachinePool{ EC2RootVolume: aws.EC2RootVolume{ @@ -85,7 +86,7 @@ func TestValidateMachinePool(t *testing.T) { name: "valid libvirt", platform: &types.Platform{Libvirt: &libvirt.Platform{}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ Libvirt: &libvirt.MachinePool{}, } @@ -97,7 +98,7 @@ func TestValidateMachinePool(t *testing.T) { name: "valid openstack", platform: &types.Platform{OpenStack: &openstack.Platform{}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ OpenStack: &openstack.MachinePool{}, } @@ -109,7 +110,7 @@ func TestValidateMachinePool(t *testing.T) { name: "mis-matched platform", platform: &types.Platform{Libvirt: &libvirt.Platform{}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ AWS: &aws.MachinePool{}, } @@ -121,7 +122,7 @@ func TestValidateMachinePool(t *testing.T) { name: "multiple platforms", platform: &types.Platform{AWS: &aws.Platform{Region: "us-east-1"}}, pool: func() *types.MachinePool { - p := validMachinePool() + p := validMachinePool("test-name") p.Platform = types.MachinePoolPlatform{ AWS: &aws.MachinePool{}, Libvirt: &libvirt.MachinePool{},