From 482a2fef7da9c46a1c50f37e54c2a05509e9f435 Mon Sep 17 00:00:00 2001 From: Pierre Prinetti Date: Fri, 23 Jun 2023 16:18:37 +0200 Subject: [PATCH] OSASINFRA-3155 - OpenStack: Create ControlPlaneMachineSet CRDs Co-Authored-By: Pierre Prinetti Co-Authored-By: Emilien Macchi --- docs/user/openstack/README.md | 1 + .../openstack/control-plane-machine-set.md | 324 ++++++++++++++++++ pkg/asset/installconfig/openstack/validate.go | 4 +- pkg/asset/machines/master.go | 2 +- pkg/asset/machines/openstack/machines.go | 184 ++++++++-- pkg/asset/machines/openstack/machines_test.go | 213 ++++++++++++ pkg/asset/machines/openstack/machinesets.go | 2 +- pkg/types/openstack/defaults/machinepool.go | 11 +- .../base-case/install-config.yaml | 34 ++ .../manifest-tests/base-case/test_cpms.py | 27 ++ .../cinder-availability-zones/test_cpms.py | 63 ++++ .../nova-availability-zones/test_cpms.py | 44 +++ 12 files changed, 882 insertions(+), 27 deletions(-) create mode 100644 docs/user/openstack/control-plane-machine-set.md create mode 100644 pkg/asset/machines/openstack/machines_test.go create mode 100644 scripts/openstack/manifest-tests/base-case/install-config.yaml create mode 100755 scripts/openstack/manifest-tests/base-case/test_cpms.py create mode 100755 scripts/openstack/manifest-tests/cinder-availability-zones/test_cpms.py create mode 100755 scripts/openstack/manifest-tests/nova-availability-zones/test_cpms.py diff --git a/docs/user/openstack/README.md b/docs/user/openstack/README.md index c517189b52..984fa054d9 100644 --- a/docs/user/openstack/README.md +++ b/docs/user/openstack/README.md @@ -55,6 +55,7 @@ In addition, it covers the installation with the default CNI (OVNKubernetes), as ## Reference Documents - [Privileges](privileges.md) +- [Control plane machine set](control-plane-machine-set.md) - [Known Issues and Workarounds](known-issues.md) - [Using the OSP 4 installer with Kuryr](kuryr.md) - [Troubleshooting your cluster](troubleshooting.md) diff --git a/docs/user/openstack/control-plane-machine-set.md b/docs/user/openstack/control-plane-machine-set.md new file mode 100644 index 0000000000..776e177dcb --- /dev/null +++ b/docs/user/openstack/control-plane-machine-set.md @@ -0,0 +1,324 @@ +# OpenStack Control plane machine set + +The [cluster-control-plane-machine-set-operator](https://github.com/openshift/cluster-control-plane-machine-set-operator) manages the Control plane machines through the `ControlPlaneMachineSet` resource. + +The `ControlPlaneMachineSet` (CPMS) contains a template of the Control plane Machine provider Spec. The template is applied uniformly to all Control plane Machines, except for a few properties of the template can change between Machines: these variable properties are defined in the `FailureDomain` stanza of the CPMS spec. + +--- + +## Example 1: availability zones set to the default + +The "default" availability zone `""` means that Nova will schedule instances ignoring availability zones. This is the default setting in OpenShift. + +This is the CPMS that the Installer generates for a cluster with name `ocp1-2g2xs`, for which no `zones` have been set in `install-config.yaml`: + +```yaml +apiVersion: machine.openshift.io/v1 +kind: ControlPlaneMachineSet +metadata: + creationTimestamp: null + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + name: cluster + namespace: openshift-machine-api +spec: + replicas: 3 + selector: + matchLabels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + state: Active + strategy: {} + template: + machineType: machines_v1beta1_machine_openshift_io + machines_v1beta1_machine_openshift_io: + failureDomains: # <-- Empty property + platform: "" + metadata: + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + spec: + lifecycleHooks: {} + metadata: {} + providerSpec: + value: # <-- The OpenStack providerSpec + apiVersion: machine.openshift.io/v1alpha1 + cloudName: openstack + cloudsSecret: + name: openstack-cloud-credentials + namespace: openshift-machine-api + flavor: m1.xlarge + image: ocp1-2g2xs-rhcos + kind: OpenstackProviderSpec + metadata: + creationTimestamp: null + networks: + - filter: {} + subnets: + - filter: + name: ocp1-2g2xs-nodes + tags: openshiftClusterID=ocp1-2g2xs + securityGroups: + - filter: {} + name: ocp1-2g2xs-master + serverGroupName: ocp1-2g2xs-master + serverMetadata: + Name: ocp1-2g2xs-master + openshiftClusterID: ocp1-2g2xs + tags: + - openshiftClusterID=ocp1-2g2xs + trunk: true + userDataSecret: + name: master-user-data +``` + +In this case, the `spec.template.machines_v1beta1_machine_openshift_io.failureDomains` stanza does not contain the `openstack` property. + +Leaving `failureDomains` without a platform value means: do not substitute the values in the `providerSpec`. In this case, the `providerSpec` does not contain the `availabilityZone` property, because it's implicitly set to the empty string. + +--- + +## Example 2: three Compute availability zones + +```yaml +apiVersion: machine.openshift.io/v1 +kind: ControlPlaneMachineSet +metadata: + creationTimestamp: null + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + name: cluster + namespace: openshift-machine-api +spec: + replicas: 3 + selector: + matchLabels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + state: Active + strategy: {} + template: + machineType: machines_v1beta1_machine_openshift_io + machines_v1beta1_machine_openshift_io: + failureDomains: + openstack: + - availabilityZone: zone-one + - availabilityZone: zone-two + - availabilityZone: zone-three + platform: OpenStack + metadata: + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + spec: + lifecycleHooks: {} + metadata: {} + providerSpec: + value: # <-- The OpenStack providerSpec + apiVersion: machine.openshift.io/v1alpha1 + cloudName: openstack + cloudsSecret: + name: openstack-cloud-credentials + namespace: openshift-machine-api + flavor: m1.xlarge + image: ocp1-2g2xs-rhcos + kind: OpenstackProviderSpec + metadata: + creationTimestamp: null + networks: + - filter: {} + subnets: + - filter: + name: ocp1-2g2xs-nodes + tags: openshiftClusterID=ocp1-2g2xs + securityGroups: + - filter: {} + name: ocp1-2g2xs-master + serverGroupName: ocp1-2g2xs-master + serverMetadata: + Name: ocp1-2g2xs-master + openshiftClusterID: ocp1-2g2xs + tags: + - openshiftClusterID=ocp1-2g2xs + trunk: true + userDataSecret: + name: master-user-data +``` + +When reconciling the Machines, cluster-control-plane-machine-set-operator will match their spec against the template, after substituting the `availabilityZone` properties for each of them. + +The three Control plane Machines will each be provisioned on a different availability zone. + +--- + +## Example 3: three Storage availability zones + +The storage availability zones apply to the root volume. The `providerSpec` must contain a `rootVolume` property. + +```yaml +apiVersion: machine.openshift.io/v1 +kind: ControlPlaneMachineSet +metadata: + creationTimestamp: null + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + name: cluster + namespace: openshift-machine-api +spec: + replicas: 3 + selector: + matchLabels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + state: Active + strategy: {} + template: + machineType: machines_v1beta1_machine_openshift_io + machines_v1beta1_machine_openshift_io: + failureDomains: + openstack: + - rootVolume: + availabilityZone: cinder-one + - rootVolume: + availabilityZone: cinder-two + - rootVolume: + availabilityZone: cinder-three + platform: OpenStack + metadata: + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + spec: + lifecycleHooks: {} + metadata: {} + providerSpec: + value: # <-- The OpenStack providerSpec + apiVersion: machine.openshift.io/v1alpha1 + cloudName: openstack + cloudsSecret: + name: openstack-cloud-credentials + namespace: openshift-machine-api + flavor: m1.xlarge + image: ocp1-2g2xs-rhcos + kind: OpenstackProviderSpec + metadata: + creationTimestamp: null + networks: + - filter: {} + subnets: + - filter: + name: ocp1-2g2xs-nodes + tags: openshiftClusterID=ocp1-2g2xs + rootVolume: + diskSize: 30 + volumeType: performance + securityGroups: + - filter: {} + name: ocp1-2g2xs-master + serverGroupName: ocp1-2g2xs-master + serverMetadata: + Name: ocp1-2g2xs-master + openshiftClusterID: ocp1-2g2xs + tags: + - openshiftClusterID=ocp1-2g2xs + trunk: true + userDataSecret: + name: master-user-data +``` + +When reconciling the Machines, cluster-control-plane-machine-set-operator will match their spec against the template, after substituting the `availabilityZone` property of the `rootVolume` for each of them. + +The three Control plane Machines will each have their `rootVolume` provisioned on a different availability zone. + +--- + +## Example 4: three Compute availability zones, three Storage availability zones + +The storage availability zones apply to the root volume. The `providerSpec` must contain a `rootVolume` property. + +```yaml +apiVersion: machine.openshift.io/v1 +kind: ControlPlaneMachineSet +metadata: + creationTimestamp: null + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + name: cluster + namespace: openshift-machine-api +spec: + replicas: 3 + selector: + matchLabels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + state: Active + strategy: {} + template: + machineType: machines_v1beta1_machine_openshift_io + machines_v1beta1_machine_openshift_io: + failureDomains: + openstack: + - availabilityZone: nova-one + rootVolume: + availabilityZone: cinder-one + - availabilityZone: nova-two + rootVolume: + availabilityZone: cinder-two + - availabilityZone: nova-three + rootVolume: + availabilityZone: cinder-three + platform: OpenStack + metadata: + labels: + machine.openshift.io/cluster-api-cluster: ocp1-2g2xs + machine.openshift.io/cluster-api-machine-role: master + machine.openshift.io/cluster-api-machine-type: master + spec: + lifecycleHooks: {} + metadata: {} + providerSpec: + value: # <-- The OpenStack providerSpec + apiVersion: machine.openshift.io/v1alpha1 + cloudName: openstack + cloudsSecret: + name: openstack-cloud-credentials + namespace: openshift-machine-api + flavor: m1.xlarge + image: ocp1-2g2xs-rhcos + kind: OpenstackProviderSpec + metadata: + creationTimestamp: null + networks: + - filter: {} + subnets: + - filter: + name: ocp1-2g2xs-nodes + tags: openshiftClusterID=ocp1-2g2xs + rootVolume: + diskSize: 30 + volumeType: performance + securityGroups: + - filter: {} + name: ocp1-2g2xs-master + serverGroupName: ocp1-2g2xs-master + serverMetadata: + Name: ocp1-2g2xs-master + openshiftClusterID: ocp1-2g2xs + tags: + - openshiftClusterID=ocp1-2g2xs + trunk: true + userDataSecret: + name: master-user-data +``` + +When reconciling the Machines, cluster-control-plane-machine-set-operator will match their spec against the template, after substituting `availabilityZone` and `rootVolume.availabilityZone` for each of them. + +The three Control plane Machines will all be provisioned on a different availability zone and have their `rootVolume` provisioned on a different availability zone. diff --git a/pkg/asset/installconfig/openstack/validate.go b/pkg/asset/installconfig/openstack/validate.go index 9241f79e96..bf3307026c 100644 --- a/pkg/asset/installconfig/openstack/validate.go +++ b/pkg/asset/installconfig/openstack/validate.go @@ -43,7 +43,7 @@ func Validate(ic *types.InstallConfig) error { controlPlane.Set(ic.Platform.OpenStack.DefaultMachinePlatform) controlPlane.Set(ic.ControlPlane.Platform.OpenStack) if controlPlane.RootVolume != nil && controlPlane.RootVolume.Zones == nil { - controlPlane.RootVolume.Zones = openstackdefaults.DefaultRootVolumeAZ() + controlPlane.RootVolume.Zones = []string{openstackdefaults.DefaultRootVolumeAZ()} } allErrs = append(allErrs, validation.ValidateMachinePool(&controlPlane, ci, true, field.NewPath("controlPlane", "platform", "openstack"))...) @@ -53,7 +53,7 @@ func Validate(ic *types.InstallConfig) error { compute.Set(ic.Platform.OpenStack.DefaultMachinePlatform) compute.Set(ic.Compute[idx].Platform.OpenStack) if compute.RootVolume != nil && compute.RootVolume.Zones == nil { - compute.RootVolume.Zones = openstackdefaults.DefaultRootVolumeAZ() + compute.RootVolume.Zones = []string{openstackdefaults.DefaultRootVolumeAZ()} } fldPath := field.NewPath("compute").Index(idx) allErrs = append(allErrs, validation.ValidateMachinePool(&compute, ci, false, fldPath.Child("platform", "openstack"))...) diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go index 510987be2c..ed20d3316d 100644 --- a/pkg/asset/machines/master.go +++ b/pkg/asset/machines/master.go @@ -330,7 +330,7 @@ func (m *Master) Generate(dependencies asset.Parents) error { imageName, _ := rhcosutils.GenerateOpenStackImageName(string(*rhcosImage), clusterID.InfraID) - machines, err = openstack.Machines(clusterID.InfraID, ic, &pool, imageName, "master", masterUserDataSecretName) + machines, controlPlaneMachineSet, err = openstack.Machines(clusterID.InfraID, ic, &pool, imageName, "master", masterUserDataSecretName) if err != nil { return errors.Wrap(err, "failed to create master machine objects") } diff --git a/pkg/asset/machines/openstack/machines.go b/pkg/asset/machines/openstack/machines.go index 48bdd9a883..ee124d6b2a 100644 --- a/pkg/asset/machines/openstack/machines.go +++ b/pkg/asset/machines/openstack/machines.go @@ -11,6 +11,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + v1 "github.com/openshift/api/config/v1" machinev1 "github.com/openshift/api/machine/v1" machinev1alpha1 "github.com/openshift/api/machine/v1alpha1" machineapi "github.com/openshift/api/machine/v1beta1" @@ -32,24 +33,19 @@ const ( ) // Machines returns a list of machines for a machinepool. -func Machines(clusterID string, config *types.InstallConfig, pool *types.MachinePool, osImage, role, userDataSecret string) ([]machineapi.Machine, error) { +func Machines(clusterID string, config *types.InstallConfig, pool *types.MachinePool, osImage, role, userDataSecret string) ([]machineapi.Machine, *machinev1.ControlPlaneMachineSet, error) { if configPlatform := config.Platform.Name(); configPlatform != openstack.Name { - return nil, fmt.Errorf("non-OpenStack configuration: %q", configPlatform) + return nil, nil, fmt.Errorf("non-OpenStack configuration: %q", configPlatform) } if poolPlatform := pool.Platform.Name(); poolPlatform != openstack.Name { - return nil, fmt.Errorf("non-OpenStack machine-pool: %q", poolPlatform) + return nil, nil, fmt.Errorf("non-OpenStack machine-pool: %q", poolPlatform) } mpool := pool.Platform.OpenStack platform := config.Platform.OpenStack trunkSupport, err := checkNetworkExtensionAvailability(platform.Cloud, "trunk", nil) if err != nil { - return nil, err - } - - volumeAZs := openstackdefaults.DefaultRootVolumeAZ() - if mpool.RootVolume != nil && len(mpool.RootVolume.Zones) != 0 { - volumeAZs = mpool.RootVolume.Zones + return nil, nil, err } total := int64(1) @@ -57,15 +53,9 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine total = *pool.Replicas } machines := make([]machineapi.Machine, 0, total) + failureDomains := failureDomainsFromSpec(*mpool) for idx := int64(0); idx < total; idx++ { - failureDomain := machinev1.OpenStackFailureDomain{ - AvailabilityZone: mpool.Zones[uint(idx)%uint(len(mpool.Zones))], - RootVolume: &machinev1.RootVolume{ - AvailabilityZone: volumeAZs[uint(idx)%uint(len(volumeAZs))], - }, - } - - var provider *machinev1alpha1.OpenstackProviderSpec + failureDomain := failureDomains[uint(idx)%uint(len(failureDomains))] provider, err := generateProvider( clusterID, @@ -78,9 +68,8 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine failureDomain, ) if err != nil { - return nil, err + return nil, nil, fmt.Errorf("failed to generate the Machine providerSpec for replica %d: %w", idx, err) } - machine := machineapi.Machine{ TypeMeta: metav1.TypeMeta{ APIVersion: "machine.openshift.io/v1beta1", @@ -105,7 +94,71 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine machines = append(machines, machine) } - return machines, nil + machineSetProvider, err := generateProvider( + clusterID, + platform, + mpool, + osImage, + role, + userDataSecret, + trunkSupport, + machinev1.OpenStackFailureDomain{RootVolume: &machinev1.RootVolume{}}, + ) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate the CPMS providerSpec: %w", err) + } + + replicas := int32(total) + + controlPlaneMachineSet := &machinev1.ControlPlaneMachineSet{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "machine.openshift.io/v1", + Kind: "ControlPlaneMachineSet", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "openshift-machine-api", + Name: "cluster", + Labels: map[string]string{ + "machine.openshift.io/cluster-api-cluster": clusterID, + }, + }, + Spec: machinev1.ControlPlaneMachineSetSpec{ + State: machinev1.ControlPlaneMachineSetStateActive, + Replicas: &replicas, + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "machine.openshift.io/cluster-api-cluster": clusterID, + "machine.openshift.io/cluster-api-machine-role": role, + "machine.openshift.io/cluster-api-machine-type": role, + }, + }, + Template: machinev1.ControlPlaneMachineSetTemplate{ + MachineType: machinev1.OpenShiftMachineV1Beta1MachineType, + OpenShiftMachineV1Beta1Machine: &machinev1.OpenShiftMachineV1Beta1MachineTemplate{ + ObjectMeta: machinev1.ControlPlaneMachineSetTemplateObjectMeta{ + Labels: map[string]string{ + "machine.openshift.io/cluster-api-cluster": clusterID, + "machine.openshift.io/cluster-api-machine-role": role, + "machine.openshift.io/cluster-api-machine-type": role, + }, + }, + Spec: machineapi.MachineSpec{ + ProviderSpec: machineapi.ProviderSpec{ + Value: &runtime.RawExtension{Object: machineSetProvider}, + }, + }, + }, + }, + }, + } + + if CPMSFailureDomains := pruneFailureDomains(failureDomains); CPMSFailureDomains != nil { + controlPlaneMachineSet.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains = machinev1.FailureDomains{ + Platform: v1.OpenStackPlatformType, + OpenStack: CPMSFailureDomains, + } + } + return machines, controlPlaneMachineSet, nil } func generateProvider(clusterID string, platform *openstack.Platform, mpool *openstack.MachinePool, osImage string, role, userDataSecret string, trunkSupport bool, failureDomain machinev1.OpenStackFailureDomain) (*machinev1alpha1.OpenstackProviderSpec, error) { @@ -198,6 +251,97 @@ func generateProvider(clusterID string, platform *openstack.Platform, mpool *ope return &spec, nil } +// failureDomainIsEmpty returns true if the failure domain only contains nil or +// zero values. +func failureDomainIsEmpty(failureDomain machinev1.OpenStackFailureDomain) bool { + if failureDomain.AvailabilityZone == "" { + if failureDomain.RootVolume == nil { + return true + } + if failureDomain.RootVolume.AvailabilityZone == "" { + return true + } + } + return false +} + +// pruneFailureDomains returns nil if the only failure domain in the given +// slice is empty. One empty failure domain is not syntactically valid in CPMS. +func pruneFailureDomains(failureDomains []machinev1.OpenStackFailureDomain) []machinev1.OpenStackFailureDomain { + if len(failureDomains) == 1 && failureDomainIsEmpty(failureDomains[0]) { + return nil + } + return failureDomains +} + +// failureDomainsFromSpec returns as many failure domains as there are zones in +// the given machine-pool. The returned failure domains have nil RootVolume if +// and only if the given machine-pool has nil RootVolume. The returned failure +// domain slice is guaranteed to have at least one element. +func failureDomainsFromSpec(mpool openstack.MachinePool) []machinev1.OpenStackFailureDomain { + var numberOfFailureDomains int + { + numberOfFailureDomains = len(mpool.Zones) + + if mpool.RootVolume != nil { + // At this point, after validation, there can't be a + // number of Compute zones different from the nunmber + // of Root volumes zones. However, we want to consider + // the case where one of them is left as its default + // (length zero), or is set to one value (length one). + // + // As a consequence, one of these is true: + // + // * there are as many Compute zones as Root volumes zones + // * there are zero or one Compute zones + // * there are zero or one Root volumes zones + if computes, volumes := len(mpool.Zones), len(mpool.RootVolume.Zones); computes > 1 && volumes > 1 && computes != volumes { + panic("Compute and Storage availability zones in the machine-pool should have been validated to have equal length") + } + + if volumes := len(mpool.RootVolume.Zones); volumes > numberOfFailureDomains { + numberOfFailureDomains = volumes + } + } + } + + // No failure domain is exactly like one failure domain with the default values. + if numberOfFailureDomains < 1 { + numberOfFailureDomains = 1 + } + + failureDomains := make([]machinev1.OpenStackFailureDomain, numberOfFailureDomains) + + for i := range failureDomains { + switch len(mpool.Zones) { + case 0: + failureDomains[i].AvailabilityZone = openstackdefaults.DefaultComputeAZ() + case 1: + failureDomains[i].AvailabilityZone = mpool.Zones[0] + default: + failureDomains[i].AvailabilityZone = mpool.Zones[i] + } + + if mpool.RootVolume != nil { + switch len(mpool.RootVolume.Zones) { + case 0: + failureDomains[i].RootVolume = &machinev1.RootVolume{ + AvailabilityZone: openstackdefaults.DefaultRootVolumeAZ(), + } + case 1: + failureDomains[i].RootVolume = &machinev1.RootVolume{ + AvailabilityZone: mpool.RootVolume.Zones[0], + } + default: + failureDomains[i].RootVolume = &machinev1.RootVolume{ + AvailabilityZone: mpool.RootVolume.Zones[i], + } + } + } + } + return failureDomains +} + func checkNetworkExtensionAvailability(cloud, alias string, opts *clientconfig.ClientOpts) (bool, error) { if opts == nil { opts = openstackdefaults.DefaultClientOpts(cloud) diff --git a/pkg/asset/machines/openstack/machines_test.go b/pkg/asset/machines/openstack/machines_test.go new file mode 100644 index 0000000000..43c98ff000 --- /dev/null +++ b/pkg/asset/machines/openstack/machines_test.go @@ -0,0 +1,213 @@ +package openstack + +import ( + "fmt" + "testing" + + machinev1 "github.com/openshift/api/machine/v1" + "github.com/openshift/installer/pkg/types/openstack" +) + +func mpWithZones(zones ...string) func(*openstack.MachinePool) { + return func(mpool *openstack.MachinePool) { + mpool.Zones = zones + } +} + +func mpWithRootVolumeZones(zones ...string) func(*openstack.MachinePool) { + return func(mpool *openstack.MachinePool) { + mpool.RootVolume = &openstack.RootVolume{Zones: zones} + } +} + +func generateMachinePool(options ...func(*openstack.MachinePool)) openstack.MachinePool { + mpool := openstack.MachinePool{} + for _, apply := range options { + apply(&mpool) + } + return mpool +} + +func TestFailureDomains(t *testing.T) { + type checkFunc func([]machinev1.OpenStackFailureDomain, error) error + check := func(fns ...checkFunc) []checkFunc { return fns } + + hasComputeZones := func(wantZones ...string) checkFunc { + return func(fds []machinev1.OpenStackFailureDomain, _ error) error { + haveZones := make([]string, len(fds)) + for i := range fds { + haveZones[i] = fds[i].AvailabilityZone + } + + if wantLen, haveLen := len(wantZones), len(haveZones); wantLen != haveLen { + return fmt.Errorf("expected compute zones %v (len %d), got %v (len %d)", wantZones, wantLen, haveZones, haveLen) + } + + for i := range fds { + if want, have := wantZones[i], haveZones[i]; want != have { + return fmt.Errorf("expected compute zones %v, got %v", wantZones, haveZones) + } + } + + return nil + } + } + + hasNilRootVolume := func(fds []machinev1.OpenStackFailureDomain, _ error) error { + for i := range fds { + if fds[i].RootVolume != nil { + return fmt.Errorf("failure domain %d has unexpectedly non-nil RootVolume", i) + } + } + return nil + } + + hasRootVolumeZones := func(wantZones ...string) checkFunc { + return func(fds []machinev1.OpenStackFailureDomain, _ error) error { + haveZones := make([]string, len(fds)) + for i := range fds { + if fds[i].RootVolume == nil { + return fmt.Errorf("failure domain %d has unexpectedly nil RootVolume", i) + } + haveZones[i] = fds[i].RootVolume.AvailabilityZone + } + + if wantLen, haveLen := len(wantZones), len(haveZones); wantLen != haveLen { + return fmt.Errorf("expected root volume zones %v, got %v", wantZones, haveZones) + } + + for i := range fds { + if want, have := wantZones[i], haveZones[i]; want != have { + return fmt.Errorf("expected root volume zones %v, got %v", wantZones, haveZones) + } + } + + return nil + } + } + + doesNotPanic := func(_ []machinev1.OpenStackFailureDomain, have error) error { + if have != nil { + return fmt.Errorf("unexpected panic: %w", have) + } + return nil + } + + panicsWith := func(want string) checkFunc { + return func(_ []machinev1.OpenStackFailureDomain, have error) error { + if have == nil { + return fmt.Errorf("unexpectedly, didn't panic") + } + if have := fmt.Sprintf("%v", have); want != have { + return fmt.Errorf("expected panic with %q, got %q", want, have) + } + return nil + } + } + + for _, tc := range [...]struct { + name string + mpool openstack.MachinePool + checks []checkFunc + }{ + { + "no_zones", + generateMachinePool(), + check( + hasComputeZones(""), + hasNilRootVolume, + doesNotPanic, + ), + }, + { + "one_compute_zone", + generateMachinePool( + mpWithZones("one"), + ), + check( + hasComputeZones("one"), + hasNilRootVolume, + doesNotPanic, + ), + }, + { + "three_compute_zones", + generateMachinePool( + mpWithZones("one", "two", "three"), + ), + check( + hasComputeZones("one", "two", "three"), + hasNilRootVolume, + doesNotPanic, + ), + }, + { + "three_compute_zones_one_root_volume_zone", + generateMachinePool( + mpWithZones("one", "two", "three"), + mpWithRootVolumeZones("volume_one"), + ), + check( + hasComputeZones("one", "two", "three"), + hasRootVolumeZones("volume_one", "volume_one", "volume_one"), + doesNotPanic, + ), + }, + { + "one_compute_zone_three_root_volume_zones", + generateMachinePool( + mpWithZones("one"), + mpWithRootVolumeZones("volume_one", "volume_two", "volume_three"), + ), + check( + hasComputeZones("one", "one", "one"), + hasRootVolumeZones("volume_one", "volume_two", "volume_three"), + doesNotPanic, + ), + }, + { + "three_compute_zone_three_root_volume_zones", + generateMachinePool( + mpWithZones("one", "two", "three"), + mpWithRootVolumeZones("volume_one", "volume_two", "volume_three"), + ), + check( + hasComputeZones("one", "two", "three"), + hasRootVolumeZones("volume_one", "volume_two", "volume_three"), + doesNotPanic, + ), + }, + { + "three_compute_zone_two_root_volume_zones_panics", + generateMachinePool( + mpWithZones("one", "two", "three"), + mpWithRootVolumeZones("volume_one", "volume_two"), + ), + check( + panicsWith("Compute and Storage availability zones in the machine-pool should have been validated to have equal length"), + ), + }, + } { + t.Run(tc.name, func(t *testing.T) { + failureDomains, recoveredPanic := func() (fds []machinev1.OpenStackFailureDomain, recoveredPanic error) { + defer func() { + if r := recover(); r != nil { + recoveredPanic = fmt.Errorf("%v", r) + } + }() + + fds = failureDomainsFromSpec(tc.mpool) + return + }() + + for _, check := range tc.checks { + if err := check(failureDomains, recoveredPanic); err != nil { + t.Error(err) + } + } + }) + } +} + +func TestPruneFailureDomains(t *testing.T) { +} diff --git a/pkg/asset/machines/openstack/machinesets.go b/pkg/asset/machines/openstack/machinesets.go index 2c6445f24a..be4e51a16d 100644 --- a/pkg/asset/machines/openstack/machinesets.go +++ b/pkg/asset/machines/openstack/machinesets.go @@ -30,7 +30,7 @@ func MachineSets(clusterID string, config *types.InstallConfig, pool *types.Mach return nil, err } - volumeAZs := openstackdefaults.DefaultRootVolumeAZ() + volumeAZs := []string{openstackdefaults.DefaultRootVolumeAZ()} if mpool.RootVolume != nil && len(mpool.RootVolume.Zones) != 0 { volumeAZs = mpool.RootVolume.Zones } diff --git a/pkg/types/openstack/defaults/machinepool.go b/pkg/types/openstack/defaults/machinepool.go index c48e50140b..a381271d37 100644 --- a/pkg/types/openstack/defaults/machinepool.go +++ b/pkg/types/openstack/defaults/machinepool.go @@ -1,6 +1,11 @@ package defaults -// DefaultRootVolumeAZ returns the default value for Root Volume availability zones. -func DefaultRootVolumeAZ() []string { - return []string{""} +// DefaultRootVolumeAZ returns the default value for Root Volume availability zone. +func DefaultRootVolumeAZ() string { + return "" +} + +// DefaultComputeAZ returns the default value for Compute availability zone. +func DefaultComputeAZ() string { + return "" } diff --git a/scripts/openstack/manifest-tests/base-case/install-config.yaml b/scripts/openstack/manifest-tests/base-case/install-config.yaml new file mode 100644 index 0000000000..db3c48087d --- /dev/null +++ b/scripts/openstack/manifest-tests/base-case/install-config.yaml @@ -0,0 +1,34 @@ +apiVersion: v1 +baseDomain: shiftstack.example.com +controlPlane: + hyperthreading: Enabled + architecture: amd64 + name: master + platform: + openstack: + type: ${COMPUTE_FLAVOR} + replicas: 3 +compute: +- name: worker + platform: + openstack: + type: ${COMPUTE_FLAVOR} + replicas: 3 +metadata: + name: manifests1 +networking: + clusterNetwork: + - cidr: 10.128.0.0/14 + hostPrefix: 23 + machineNetwork: + - cidr: 10.0.128.0/17 + networkType: OVNKubernetes + serviceNetwork: + - 172.30.0.0/16 +platform: + openstack: + cloud: ${OS_CLOUD} + externalNetwork: ${EXTERNAL_NETWORK} + computeFlavor: ${COMPUTE_FLAVOR} # deprecated in 4.7 + lbFloatingIP: ${API_FIP} +pullSecret: ${PULL_SECRET} diff --git a/scripts/openstack/manifest-tests/base-case/test_cpms.py b/scripts/openstack/manifest-tests/base-case/test_cpms.py new file mode 100755 index 0000000000..663b4dc9a5 --- /dev/null +++ b/scripts/openstack/manifest-tests/base-case/test_cpms.py @@ -0,0 +1,27 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +import unittest +import xmlrunner + +import os +import sys +import yaml + +ASSETS_DIR = "" + +class ControlPlaneMachineSet(unittest.TestCase): + def setUp(self): + """Parse the CPMS into a Python data structure.""" + with open(f'{ASSETS_DIR}/openshift/99_openshift-machine-api_master-control-plane-machine-set.yaml') as f: + self.cpms = yaml.load(f, Loader=yaml.FullLoader) + + def test_compute_zones(self): + """Assert that the OpenStack CPMS failureDomains value is empty.""" + self.assertNotIn("openstack", self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["failureDomains"]) + + +if __name__ == '__main__': + ASSETS_DIR = sys.argv.pop() + with open(os.environ.get('JUNIT_FILE', '/dev/null'), 'wb') as output: + unittest.main(testRunner=xmlrunner.XMLTestRunner(output=output), failfast=False, buffer=False, catchbreak=False, verbosity=2) diff --git a/scripts/openstack/manifest-tests/cinder-availability-zones/test_cpms.py b/scripts/openstack/manifest-tests/cinder-availability-zones/test_cpms.py new file mode 100755 index 0000000000..da5623474b --- /dev/null +++ b/scripts/openstack/manifest-tests/cinder-availability-zones/test_cpms.py @@ -0,0 +1,63 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +import unittest +import xmlrunner + +import os +import sys +import yaml + +ASSETS_DIR = "" + +EXPECTED_MASTER_ZONE_NAMES = ["MasterAZ1", "MasterAZ2", "MasterAZ3"] +EXPECTED_MASTER_ROOT_VOLUME_ZONE_NAMES = ["VolumeAZ1", "VolumeAZ2", "VolumeAZ3"] + +class ControlPlaneMachineSet(unittest.TestCase): + def setUp(self): + """Parse the CPMS into a Python data structure.""" + with open(f'{ASSETS_DIR}/openshift/99_openshift-machine-api_master-control-plane-machine-set.yaml') as f: + self.cpms = yaml.load(f, Loader=yaml.FullLoader) + + def test_providerspec_failuredomain_fields(self): + """Assert that the failure-domain-managed fields in the CPMS providerSpec are omitted.""" + provider_spec = self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["spec"]["providerSpec"]["value"] + self.assertNotIn("availabilityZone", provider_spec) + self.assertNotIn("availabilityZone", provider_spec["rootVolume"]) + + def test_compute_zones(self): + """Assert that the CPMS failure domain zones match the expected machine-pool zones.""" + self.assertIn("failureDomains", self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]) + failure_domains = self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["failureDomains"]["openstack"] + + compute_zones = [] + for failure_domain in failure_domains: + zone = failure_domain["availabilityZone"] + compute_zones.append(zone) + self.assertIn(zone, EXPECTED_MASTER_ZONE_NAMES) + + for expected_zone in EXPECTED_MASTER_ZONE_NAMES: + self.assertIn(expected_zone, compute_zones) + + def test_storage_zones(self): + """Assert that the CPMS failure domain root volume zones match the expected machine-pool zones.""" + self.assertIn("failureDomains", self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]) + self.assertIn("openstack", self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["failureDomains"]) + failure_domains = self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["failureDomains"]["openstack"] + + storage_zones = [] + for failure_domain in failure_domains: + self.assertIn("rootVolume", failure_domain) + self.assertIn("availabilityZone", failure_domain["rootVolume"]) + rootVolumeZone = failure_domain["rootVolume"]["availabilityZone"] + storage_zones.append(rootVolumeZone) + self.assertIn(rootVolumeZone, EXPECTED_MASTER_ROOT_VOLUME_ZONE_NAMES) + + for expected_zone in EXPECTED_MASTER_ROOT_VOLUME_ZONE_NAMES: + self.assertIn(expected_zone, storage_zones) + + +if __name__ == '__main__': + ASSETS_DIR = sys.argv.pop() + with open(os.environ.get('JUNIT_FILE', '/dev/null'), 'wb') as output: + unittest.main(testRunner=xmlrunner.XMLTestRunner(output=output), failfast=False, buffer=False, catchbreak=False, verbosity=2) diff --git a/scripts/openstack/manifest-tests/nova-availability-zones/test_cpms.py b/scripts/openstack/manifest-tests/nova-availability-zones/test_cpms.py new file mode 100755 index 0000000000..7683111d67 --- /dev/null +++ b/scripts/openstack/manifest-tests/nova-availability-zones/test_cpms.py @@ -0,0 +1,44 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +import unittest +import xmlrunner + +import os +import sys +import yaml + +ASSETS_DIR = "" + +EXPECTED_MASTER_ZONE_NAMES = ["masterzone", "masterztwo", "masterzthree"] + +class ControlPlaneMachineSet(unittest.TestCase): + def setUp(self): + """Parse the CPMS into a Python data structure.""" + with open(f'{ASSETS_DIR}/openshift/99_openshift-machine-api_master-control-plane-machine-set.yaml') as f: + self.cpms = yaml.load(f, Loader=yaml.FullLoader) + + def test_providerspec_failuredomain_fields(self): + """Assert that the failure-domain-managed fields in the CPMS providerSpec are omitted.""" + provider_spec = self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["spec"]["providerSpec"]["value"] + self.assertNotIn("availabilityZone", provider_spec) + + def test_compute_zones(self): + """Assert that the CPMS failure domain zones match the expected machine-pool zones.""" + self.assertIn("failureDomains", self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]) + failure_domains = self.cpms["spec"]["template"]["machines_v1beta1_machine_openshift_io"]["failureDomains"]["openstack"] + + compute_zones = [] + for failure_domain in failure_domains: + zone = failure_domain["availabilityZone"] + compute_zones.append(zone) + self.assertIn(zone, EXPECTED_MASTER_ZONE_NAMES) + + for expected_zone in EXPECTED_MASTER_ZONE_NAMES: + self.assertIn(expected_zone, compute_zones) + + +if __name__ == '__main__': + ASSETS_DIR = sys.argv.pop() + with open(os.environ.get('JUNIT_FILE', '/dev/null'), 'wb') as output: + unittest.main(testRunner=xmlrunner.XMLTestRunner(output=output), failfast=False, buffer=False, catchbreak=False, verbosity=2)