Merge pull request openshift#7860 from vr4manta/OCPBUGS-25453

OCPBUGS-25453: duplicate failure domains in CMPS

Author: openshift-merge-bot[bot]

pkg/asset/machines/vsphere/machines.go

@@ -83,9 +83,11 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine
 			"machine.openshift.io/cluster-api-machine-type": role,
 		}
 
-		failureDomains = append(failureDomains, machinev1.VSphereFailureDomain{
-			Name: failureDomain.Name,
-		})
+		if !hasFailureDomain(failureDomains, failureDomain.Name) {
+			failureDomains = append(failureDomains, machinev1.VSphereFailureDomain{
+				Name: failureDomain.Name,
+			})
+		}
 
 		osImageForZone := failureDomain.Topology.Template
 		if failureDomain.Topology.Template == "" {
@@ -255,3 +257,12 @@ func provider(clusterID string, vcenter *vsphere.VCenter, failureDomain vsphere.
 // ConfigMasters sets the PublicIP flag and assigns a set of load balancers to the given machines
 func ConfigMasters(machines []machineapi.Machine, clusterID string) {
 }
+
+func hasFailureDomain(failureDomains []machinev1.VSphereFailureDomain, failureDomain string) bool {
+	for _, fd := range failureDomains {
+		if fd.Name == failureDomain {
+			return true
+		}
+	}
+	return false
+}

pkg/asset/machines/vsphere/machines_test.go

@@ -215,6 +215,26 @@ var machinePoolReducedZones = types.MachinePool{
 	Architecture:   types.ArchitectureAMD64,
 }
 
+var machinePoolSingleZones = types.MachinePool{
+	Name:     "master",
+	Replicas: &machinePoolReplicas,
+	Platform: types.MachinePoolPlatform{
+		VSphere: &vsphere.MachinePool{
+			NumCPUs:           4,
+			NumCoresPerSocket: 2,
+			MemoryMiB:         16384,
+			OSDisk: vsphere.OSDisk{
+				DiskSizeGB: 60,
+			},
+			Zones: []string{
+				"deployzone-us-east-1a",
+			},
+		},
+	},
+	Hyperthreading: "true",
+	Architecture:   types.ArchitectureAMD64,
+}
+
 var machinePoolUndefinedZones = types.MachinePool{
 	Name:     "master",
 	Replicas: &machinePoolReplicas,
@@ -344,6 +364,21 @@ func TestConfigMasters(t *testing.T) {
 				},
 			},
 		},
+		{
+			testCase:                   "all masters in single zone / pool",
+			machinePool:                &machinePoolSingleZones,
+			maxAllowedWorkspaceMatches: 3,
+			installConfig:              installConfig,
+			workspaces: []machineapi.Workspace{
+				{
+					Server:       "your.vcenter.example.com",
+					Datacenter:   "dc1",
+					Folder:       "/dc1/vm/folder1",
+					Datastore:    "/dc1/datastore/datastore1",
+					ResourcePool: "/dc1/host/c1/Resources/rp1",
+				},
+			},
+		},
 		{
 			testCase:                   "full path to cluster resource pool when no pool provided via placement constraint",
 			machinePool:                &machinePoolValidZones,
@@ -378,7 +413,7 @@ func TestConfigMasters(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.testCase, func(t *testing.T) {
-			machines, _, err := Machines(clusterID, tc.installConfig, tc.machinePool, "", "", "")
+			machines, cpms, err := Machines(clusterID, tc.installConfig, tc.machinePool, "", "", "")
 			assertOnUnexpectedErrorState(t, tc.expectedError, err)
 
 			if len(tc.workspaces) > 0 {
@@ -404,6 +439,14 @@ func TestConfigMasters(t *testing.T) {
 						t.Errorf("machine workspace was enountered too few times[min: %d]", tc.minAllowedWorkspaceMatches)
 					}
 				}
+
+				if cpms != nil {
+					// Make sure FDs equal same quantity as config
+					fds := cpms.Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains
+					if len(fds.VSphere) != len(tc.workspaces) {
+						t.Errorf("machine workspace count %d does not equal number of failure domains [count: %d] in CPMS", len(tc.workspaces), len(fds.VSphere))
+					}
+				}
 			}
 		})
 	}

pkg/types/vsphere/validation/machinepool.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/strings/slices"
 
 	"github.com/openshift/installer/pkg/types"
 	"github.com/openshift/installer/pkg/types/vsphere"
@@ -51,14 +52,24 @@ func ValidateMachinePool(platform *vsphere.Platform, machinePool *types.MachineP
 	}
 
 	if len(vspherePool.Zones) > 0 {
+		var zoneRefs []string
 		if len(platform.FailureDomains) == 0 {
 			return append(allErrs, field.Required(fldPath.Child("zones"), "failureDomains must be defined if zones are defined"))
 		}
-		for _, zone := range vspherePool.Zones {
+		for index, zone := range vspherePool.Zones {
 			err := validate.ClusterName1035(zone)
 			if err != nil {
 				allErrs = append(allErrs, field.Invalid(fldPath.Child("zones"), vspherePool.Zones, err.Error()))
 			}
+
+			// This checks to make sure ref is not duplicated
+			if slices.Contains(zoneRefs, zone) {
+				allErrs = append(allErrs, field.Duplicate(fldPath.Child("zones").Index(index), zone))
+			} else {
+				zoneRefs = append(zoneRefs, zone)
+			}
+
+			// Verify zone references a valid failure domain
 			zoneDefined := false
 			for _, failureDomain := range platform.FailureDomains {
 				if failureDomain.Name == zone {

pkg/types/vsphere/validation/machinepool_test.go

@@ -154,6 +154,22 @@ func TestValidateMachinePool(t *testing.T) {
 			expectedZones:  &[]string{"test-east-1a", "test-east-2a"},
 			expectedErrMsg: "",
 		},
+		{
+			name:     "multi-zone duplicate zones defined for control plane pool",
+			platform: validPlatform(),
+			pool: &types.MachinePool{
+				Name: types.MachinePoolControlPlaneRoleName,
+				Platform: types.MachinePoolPlatform{
+					VSphere: &vsphere.MachinePool{
+						Zones: []string{
+							"test-east-1a",
+							"test-east-1a",
+						},
+					},
+				},
+			},
+			expectedErrMsg: `test-path.zones\[1]: Duplicate value: "test-east-1a"`,
+		},
 		{
 			name:     "multi-zone no zones defined for compute pool",
 			platform: validPlatform(),
@@ -166,6 +182,22 @@ func TestValidateMachinePool(t *testing.T) {
 			expectedZones:  &[]string{"test-east-1a", "test-east-2a"},
 			expectedErrMsg: "",
 		},
+		{
+			name:     "multi-zone duplicate zones defined for compute pool",
+			platform: validPlatform(),
+			pool: &types.MachinePool{
+				Name: types.MachinePoolComputeRoleName,
+				Platform: types.MachinePoolPlatform{
+					VSphere: &vsphere.MachinePool{
+						Zones: []string{
+							"test-east-1a",
+							"test-east-1a",
+						},
+					},
+				},
+			},
+			expectedErrMsg: `test-path.zones\[1]: Duplicate value: "test-east-1a"`,
+		},
 		{
 			name:     "multi-zone undefined zone",
 			platform: validPlatform(),

pkg/types/vsphere/validation/platform.go

@@ -9,6 +9,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/utils/strings/slices"
 
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/openshift/installer/pkg/types"
@@ -115,12 +116,20 @@ func validateVCenters(p *vsphere.Platform, fldPath *field.Path) field.ErrorList
 }
 
 func validateFailureDomains(p *vsphere.Platform, fldPath *field.Path, isLegacyUpi bool) field.ErrorList {
+	var fdNames []string
 	allErrs := field.ErrorList{}
 	topologyFld := fldPath.Child("topology")
 	var associatedVCenter *vsphere.VCenter
-	for _, failureDomain := range p.FailureDomains {
+	for index, failureDomain := range p.FailureDomains {
 		if len(failureDomain.Name) == 0 {
 			allErrs = append(allErrs, field.Required(fldPath.Child("name"), "must specify the name"))
+		} else {
+			// Check and make sure the name is not duplicated w/ any other Failure Domains
+			if slices.Contains(fdNames, failureDomain.Name) {
+				allErrs = append(allErrs, field.Duplicate(fldPath.Child("name").Index(index), failureDomain.Name))
+			} else {
+				fdNames = append(fdNames, failureDomain.Name)
+			}
 		}
 		if len(failureDomain.Server) > 0 {
 			for i, vcenter := range p.VCenters {

pkg/types/vsphere/validation/platform_test.go

@@ -319,6 +319,15 @@ func TestValidatePlatform(t *testing.T) {
 			}(),
 			expectedError: `^test-path\.failureDomains\.name: Required value: must specify the name`,
 		},
+		{
+			name: "Multi-zone platform failureDomain duplicate names",
+			platform: func() *vsphere.Platform {
+				p := validPlatform()
+				p.FailureDomains[1].Name = p.FailureDomains[0].Name
+				return p
+			}(),
+			expectedError: `test-path\.failureDomains\.name\[1\]: Duplicate value: "test-east-1a"`,
+		},
 		{
 			name: "Multi-zone platform failureDomain zone missing tag category",
 			platform: func() *vsphere.Platform {