OSASINFRA-3180: openstack: volume type in failure domain

docs/user/failure-domains.md

@@ -86,8 +86,9 @@ An Azure failure domain will look something like the example below:
 
 ## OpenStack
 
-On OpenStack, the failure domains represented in the control plane machine set can be considered analogous to the
-OpenStack availability zones (for Nova and Cinder).
+On OpenStack, the failure domains represented in the control plane machine set
+include the OpenStack Nova availability zone for instance placement, as well as
+the Cinder availability zone and the volume type for root volume placement.
 
 > OpenStack Availability Zones are an end-user visible logical abstraction for partitioning an OpenStack cloud without
 > knowing the physical infrastructure. They are used to partition a cloud on arbitrary factors, such as location (country, datacenter, rack),
@@ -103,4 +104,5 @@ An OpenStack failure domain will look something like the example below:
 - availabilityZone: "<nova availability zone>"
   rootVolume:
     availabilityZone: "<cinder availability zone>"
+    volumeType: "<cinder volume type>"
 ```

docs/user/installation.md

@@ -241,8 +241,10 @@ plane machines and you should populate this on both the Machine and the ControlP
 
 #### Configuring a control plane machine set on OpenStack
 
-Two fields are supported for now: `availabilityZone` (instance AZ) and `rootVolume.availabilityZone` (root volume AZ).
-Gather the existing control plane machines and note the value of the zones of each if they exist.
+The OpenStack failureDomain configuration supports three fields:
+`availabilityZone` (instance AZ), `rootVolume.availabilityZone` (root volume
+AZ) and `rootVolume.volumeType`.
+Gather the existing control plane machines and note the value of the properties of each if they differ from each other.
 Aside from these fields, the remaining in spec the machines should be identical.
 
 Copy the value from one of the machines into the `providerSpec.value` (6) on the example above.
@@ -253,9 +255,11 @@ For each AZ you have in the cluster, configure a failure domain like below:
 - availabilityZone: "<nova availability zone>"
   rootVolume:
     availabilityZone: "<cinder availability zone>"
+    volumeType: "<cinder volume type>"
 ```
 
-With these zones, the complete `failureDomains` (4 and 5) on the example above should look something like below:
+OpenStack failure domains may not be empty, however each individual property is optional.
+With these zones, the `failureDomains` (4 and 5) on the example above should look something like below:
 ```yaml
 failureDomains:
   platform: OpenStack
@@ -275,4 +279,3 @@ Prior to 4.14, if the masters were configured with Availability Zones (AZ), the
 one ServerGroup in OpenStack (the one initially created for master-0, ending with the name of the AZ) but configure
 the Machine ProviderSpec with different ServerGroups, one per AZ.
 So if you upgrade a cluster from a previous release to 4.14, you'll need to follow this [solution](https://access.redhat.com/solutions/7013893).
-

go.mod

@@ -12,7 +12,7 @@ require (
 	github.com/onsi/gomega v1.27.7
 	github.com/openshift/api v0.0.0-20230705100138-391aead809e1
 	github.com/openshift/client-go v0.0.0-20230607134213-3cd0021bbee3
-	github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230622171654-75c6bcfa831c
+	github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230706132925-77764237f2e6
 	github.com/openshift/library-go v0.0.0-20230523150659-ab179469ba38
 	github.com/spf13/pflag v1.0.5
 	k8s.io/api v0.27.3

go.sum

@@ -449,8 +449,8 @@ github.com/openshift/api v0.0.0-20230705100138-391aead809e1 h1:bNnNrF6qDQemp9DSP
 github.com/openshift/api v0.0.0-20230705100138-391aead809e1/go.mod h1:yimSGmjsI+XF1mr+AKBs2//fSXIOhhetHGbMlBEfXbs=
 github.com/openshift/client-go v0.0.0-20230607134213-3cd0021bbee3 h1:uVCq/Sx2y4UZh+qCsCL1BBUJpc3DULHkN4j7XHHgHtw=
 github.com/openshift/client-go v0.0.0-20230607134213-3cd0021bbee3/go.mod h1:M+VUIcqx5IvgzejcbgmQnxETPrXRYlcufHpw2bAgz9Y=
-github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230622171654-75c6bcfa831c h1:2EpVQ7ZZIvpm3PExUIjrIHknRAfyJBr0xjUJFQjYaxA=
-github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230622171654-75c6bcfa831c/go.mod h1:w4P7zcu7okmBpkjKJK71rl5hp1a8RFm1NraVrxVqiUs=
+github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230706132925-77764237f2e6 h1:6Gq1pKceLaK0EU/tkXLkkqE0DI3bcZoXUB33scvwHUM=
+github.com/openshift/cluster-api-actuator-pkg/testutils v0.0.0-20230706132925-77764237f2e6/go.mod h1:w4P7zcu7okmBpkjKJK71rl5hp1a8RFm1NraVrxVqiUs=
 github.com/openshift/library-go v0.0.0-20230523150659-ab179469ba38 h1:rKEpSwRxeQ6eN915GbcuyikwyWu//V61w5zIUWD9b2U=
 github.com/openshift/library-go v0.0.0-20230523150659-ab179469ba38/go.mod h1:PJVatR/oS/EaFciwylyAr9hORSqQHrC+5bXf4L0wsBY=
 github.com/otiai10/copy v1.2.0 h1:HvG945u96iNadPoG2/Ja2+AUJeW5YuFQMixq9yirC+k=

pkg/controllers/controlplanemachinesetgenerator/controller_test.go

@@ -2197,21 +2197,21 @@ var _ = Describe("controlplanemachinesetgenerator controller on Nutanix", func()
 var _ = Describe("controlplanemachinesetgenerator controller on OpenStack", func() {
 
 	var (
-		az1FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az1").WithRootVolume(machinev1.RootVolume{
+		az1FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az1").WithRootVolume(&machinev1.RootVolume{
 			AvailabilityZone: "cinder-az1",
 		})
 
-		az2FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az2").WithRootVolume(machinev1.RootVolume{
+		az2FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az2").WithRootVolume(&machinev1.RootVolume{
 			AvailabilityZone: "cinder-az2",
 		})
 
-		az3FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az3").WithRootVolume(machinev1.RootVolume{
+		az3FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az3").WithRootVolume(&machinev1.RootVolume{
 			AvailabilityZone: "cinder-az3",
 		})
 
 		az4FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az4")
 
-		az5FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(machinev1.RootVolume{
+		az5FailureDomainBuilderOpenStack = machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(&machinev1.RootVolume{
 			AvailabilityZone: "cinder-az5",
 		})
 

pkg/controllers/controlplanemachinesetgenerator/openstack.go

@@ -70,7 +70,7 @@ func generateControlPlaneMachineSetOpenStackSpec(logger logr.Logger, machines []
 		return machinev1builder.ControlPlaneMachineSetSpecApplyConfiguration{}, fmt.Errorf("failed to build ControlPlaneMachineSet's OpenStack failure domains: %w", err)
 	}
 
-	controlPlaneMachineSetMachineSpecApplyConfig, err := buildControlPlaneMachineSetOpenStackMachineSpec(logger, machines)
+	controlPlaneMachineSetMachineSpecApplyConfig, err := buildControlPlaneMachineSetOpenStackMachineSpec(logger, machines, controlPlaneMachineSetMachineFailureDomainsApplyConfig)
 	if err != nil {
 		return machinev1builder.ControlPlaneMachineSetSpecApplyConfiguration{}, fmt.Errorf("failed to build ControlPlaneMachineSet's OpenStack spec: %w", err)
 	}
@@ -84,7 +84,7 @@ func generateControlPlaneMachineSetOpenStackSpec(logger logr.Logger, machines []
 }
 
 // buildControlPlaneMachineSetOpenStackMachineSpec builds an OpenStack flavored MachineSpec for the ControlPlaneMachineSet.
-func buildControlPlaneMachineSetOpenStackMachineSpec(logger logr.Logger, machines []machinev1beta1.Machine) (*machinev1beta1builder.MachineSpecApplyConfiguration, error) {
+func buildControlPlaneMachineSetOpenStackMachineSpec(logger logr.Logger, machines []machinev1beta1.Machine, failureDomains *machinev1builder.FailureDomainsApplyConfiguration) (*machinev1beta1builder.MachineSpecApplyConfiguration, error) {
 	// The machines slice is sorted by the creation time.
 	// We want to get the provider config for the newest machine.
 	providerConfig, err := providerconfig.NewProviderConfigFromMachineSpec(logger, machines[0].Spec)
@@ -94,11 +94,14 @@ func buildControlPlaneMachineSetOpenStackMachineSpec(logger logr.Logger, machine
 
 	openStackProviderSpec := providerConfig.OpenStack().Config()
 
-	// Remove field related to the failure domain.
-	openStackProviderSpec.AvailabilityZone = ""
+	// If there are failure domains, remove the corresponding managed fields from the providerSpec.
+	if failureDomains != nil && len(failureDomains.OpenStack) > 0 {
+		openStackProviderSpec.AvailabilityZone = ""
 
-	if openStackProviderSpec.RootVolume != nil {
-		openStackProviderSpec.RootVolume.Zone = ""
+		if openStackProviderSpec.RootVolume != nil {
+			openStackProviderSpec.RootVolume.Zone = ""
+			openStackProviderSpec.RootVolume.VolumeType = ""
+		}
 	}
 
 	rawBytes, err := json.Marshal(openStackProviderSpec)

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain.go

@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 
 	configv1 "github.com/openshift/api/config/v1"
 	machinev1 "github.com/openshift/api/machine/v1"
@@ -317,24 +318,29 @@ func gcpFailureDomainToString(fd machinev1.GCPFailureDomain) string {
 
 // openstackFailureDomainToString converts the OpenStackFailureDomain into a string.
 func openstackFailureDomainToString(fd machinev1.OpenStackFailureDomain) string {
-	displayRootVolume := func(rootVolume machinev1.RootVolume) string {
-		return fmt.Sprintf("{AvailabilityZone:%s}", rootVolume.AvailabilityZone)
+	if fd.AvailabilityZone == "" && fd.RootVolume == nil {
+		return unknownFailureDomain
 	}
 
-	// AvailabilityZone only
-	if fd.AvailabilityZone != "" && fd.RootVolume == nil {
-		return fmt.Sprintf("OpenStackFailureDomain{AvailabilityZone:%s}", fd.AvailabilityZone)
-	}
+	var failureDomain []string
 
-	// RootVolume only
-	if fd.AvailabilityZone == "" && fd.RootVolume != nil {
-		return fmt.Sprintf("OpenStackFailureDomain{RootVolume:%s}", displayRootVolume(*fd.RootVolume))
+	if fd.AvailabilityZone != "" {
+		failureDomain = append(failureDomain, "AvailabilityZone:"+fd.AvailabilityZone)
 	}
 
-	// AvailabilityZone and RootVolume
-	if fd.AvailabilityZone != "" && fd.RootVolume != nil {
-		return fmt.Sprintf("OpenStackFailureDomain{AvailabilityZone:%s, RootVolume:%s}", fd.AvailabilityZone, displayRootVolume(*fd.RootVolume))
+	if fd.RootVolume != nil {
+		var rootVolume []string
+
+		if fd.RootVolume.AvailabilityZone != "" {
+			rootVolume = append(rootVolume, "AvailabilityZone:"+fd.RootVolume.AvailabilityZone)
+		}
+
+		if fd.RootVolume.VolumeType != "" {
+			rootVolume = append(rootVolume, "VolumeType:"+fd.RootVolume.VolumeType)
+		}
+
+		failureDomain = append(failureDomain, "RootVolume:{"+strings.Join(rootVolume, ", ")+"}")
 	}
 
-	return unknownFailureDomain
+	return "OpenStackFailureDomain{" + strings.Join(failureDomain, ", ") + "}"
 }

pkg/machineproviders/providers/openshift/machine/v1beta1/failuredomain/failuredomain_test.go

@@ -196,6 +196,33 @@ var _ = Describe("FailureDomains", func() {
 			})
 		})
 
+		Context("With OpenStack failure domain configuration with volumeType", func() {
+			var failureDomains []FailureDomain
+			var err error
+
+			BeforeEach(func() {
+				config := machinev1resourcebuilder.OpenStackFailureDomains().WithFailureDomainBuilders(
+					machinev1resourcebuilder.OpenStackFailureDomainBuilder{AvailabilityZone: "nova-az0", RootVolume: &machinev1.RootVolume{VolumeType: "volume.hostA"}},
+					machinev1resourcebuilder.OpenStackFailureDomainBuilder{AvailabilityZone: "nova-az1", RootVolume: &machinev1.RootVolume{VolumeType: "volume.hostB"}},
+					machinev1resourcebuilder.OpenStackFailureDomainBuilder{AvailabilityZone: "nova-az2", RootVolume: &machinev1.RootVolume{VolumeType: "volume.hostC"}},
+				).BuildFailureDomains()
+
+				failureDomains, err = NewFailureDomains(config)
+			})
+
+			It("should not error", func() {
+				Expect(err).ToNot(HaveOccurred())
+			})
+
+			It("should construct a list of failure domains", func() {
+				Expect(failureDomains).To(ConsistOf(
+					HaveField("String()", "OpenStackFailureDomain{AvailabilityZone:nova-az0, RootVolume:{VolumeType:volume.hostA}}"),
+					HaveField("String()", "OpenStackFailureDomain{AvailabilityZone:nova-az1, RootVolume:{VolumeType:volume.hostB}}"),
+					HaveField("String()", "OpenStackFailureDomain{AvailabilityZone:nova-az2, RootVolume:{VolumeType:volume.hostC}}"),
+				))
+			})
+		})
+
 		Context("With invalid OpenStack failure domain configuration", func() {
 			var failureDomains []FailureDomain
 			var err error
@@ -354,7 +381,7 @@ var _ = Describe("FailureDomains", func() {
 		Context("with a Compute and Storage availability zone", func() {
 			BeforeEach(func() {
 				fd.openstack = machinev1resourcebuilder.OpenStackFailureDomain().WithComputeAvailabilityZone("nova-az0").
-					WithRootVolume(filterRootVolume).Build()
+					WithRootVolume(&filterRootVolume).Build()
 			})
 
 			It("returns the Compute and Storage availability zones for String()", func() {
@@ -373,7 +400,7 @@ var _ = Describe("FailureDomains", func() {
 		})
 		Context("with a Storage availability zone only", func() {
 			BeforeEach(func() {
-				fd.openstack = machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(filterRootVolume).Build()
+				fd.openstack = machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(&filterRootVolume).Build()
 			})
 
 			It("returns the Storage availability zone for String()", func() {
@@ -483,11 +510,11 @@ var _ = Describe("FailureDomains", func() {
 			BeforeEach(func() {
 				fd1 = failureDomain{
 					platformType: configv1.OpenStackPlatformType,
-					openstack:    machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(filterRootVolume).Build(),
+					openstack:    machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(&filterRootVolume).Build(),
 				}
 				fd2 = failureDomain{
 					platformType: configv1.OpenStackPlatformType,
-					openstack:    machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(filterRootVolume).Build(),
+					openstack:    machinev1resourcebuilder.OpenStackFailureDomain().WithRootVolume(&filterRootVolume).Build(),
 				}
 			})
 

pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/openstack.go

@@ -42,8 +42,14 @@ func (a OpenStackProviderConfig) InjectFailureDomain(fd machinev1.OpenStackFailu
 		newOpenStackProviderConfig.providerConfig.AvailabilityZone = fd.AvailabilityZone
 	}
 
-	if fd.RootVolume != nil && newOpenStackProviderConfig.providerConfig.RootVolume != nil && fd.RootVolume.AvailabilityZone != "" {
-		newOpenStackProviderConfig.providerConfig.RootVolume.Zone = fd.RootVolume.AvailabilityZone
+	if fd.RootVolume != nil && newOpenStackProviderConfig.providerConfig.RootVolume != nil {
+		if fd.RootVolume.AvailabilityZone != "" {
+			newOpenStackProviderConfig.providerConfig.RootVolume.Zone = fd.RootVolume.AvailabilityZone
+		}
+
+		if fd.RootVolume.VolumeType != "" {
+			newOpenStackProviderConfig.providerConfig.RootVolume.VolumeType = fd.RootVolume.VolumeType
+		}
 	}
 
 	return newOpenStackProviderConfig
@@ -52,20 +58,22 @@ func (a OpenStackProviderConfig) InjectFailureDomain(fd machinev1.OpenStackFailu
 // ExtractFailureDomain returns an OpenStackFailureDomain based on the failure domain
 // information stored within the OpenStackProviderConfig.
 func (a OpenStackProviderConfig) ExtractFailureDomain() machinev1.OpenStackFailureDomain {
-	rootVolume := func(pc machinev1alpha1.OpenstackProviderSpec) *machinev1.RootVolume {
-		if pc.RootVolume != nil && pc.RootVolume.Zone != "" {
-			rootVolume := &machinev1.RootVolume{}
-			rootVolume.AvailabilityZone = a.providerConfig.RootVolume.Zone
-
-			return rootVolume
-		} else {
-			return nil
+	var failureDomainRootVolume *machinev1.RootVolume
+
+	if a.providerConfig.RootVolume != nil {
+		// Be liberal in accepting an empty rootVolume in the
+		// OpenStackFailureDomain. It should count as nil.
+		if az, vt := a.providerConfig.RootVolume.Zone, a.providerConfig.RootVolume.VolumeType; az != "" || vt != "" {
+			failureDomainRootVolume = &machinev1.RootVolume{
+				AvailabilityZone: az,
+				VolumeType:       vt,
+			}
 		}
 	}
 
 	return machinev1.OpenStackFailureDomain{
 		AvailabilityZone: a.providerConfig.AvailabilityZone,
-		RootVolume:       rootVolume(a.providerConfig),
+		RootVolume:       failureDomainRootVolume,
 	}
 }