From 582e5be91b2c12602b4dd0cd9e19ade29467e9ec Mon Sep 17 00:00:00 2001 From: Brent Barbachem Date: Thu, 19 Sep 2024 10:32:18 -0400 Subject: [PATCH] OCPBUGS-41184: GCP Validate Disk and Instance Type ** Update the error text and the way that disks and instances are validated. Before, the error message was backwards on indicating if the instance type or the disk type was the problem. Now, the disk type is validated against the instance type (rather than the opposite). --- pkg/asset/installconfig/gcp/validation.go | 78 +++++++++++-------- .../installconfig/gcp/validation_test.go | 12 ++- pkg/types/gcp/machinepools.go | 24 ++++-- 3 files changed, 73 insertions(+), 41 deletions(-) diff --git a/pkg/asset/installconfig/gcp/validation.go b/pkg/asset/installconfig/gcp/validation.go index f3734811b6b..406e6b117d1 100644 --- a/pkg/asset/installconfig/gcp/validation.go +++ b/pkg/asset/installconfig/gcp/validation.go @@ -73,20 +73,30 @@ func Validate(client API, ic *types.InstallConfig) error { return allErrs.ToAggregate() } -func validateInstanceType(fldPath *field.Path, instanceType string, validInstanceTypes []string, arch string) *field.Error { - if len(validInstanceTypes) == 0 { - return field.InternalError(fldPath, fmt.Errorf("no valid instance types found")) +func validateInstanceAndDiskType(fldPath *field.Path, diskType, instanceType, arch string) *field.Error { + if instanceType == "" { + // nothing to validate + return nil } - if instanceType != "" { - family, _, _ := strings.Cut(instanceType, "-") - acceptedInstanceTypes := sets.New(validInstanceTypes...) - if !acceptedInstanceTypes.Has(family) { - return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedInstanceTypes)) - } + family, _, _ := strings.Cut(instanceType, "-") + diskTypes, ok := gcp.InstanceTypeToDiskTypeMap[family] + if !ok { + return field.NotFound(fldPath.Child("type"), family) + } - if arch == types.ArchitectureARM64 && family != "t2a" { - return field.InternalError(fldPath, fmt.Errorf("instance type %s requires %s architecture", instanceType, types.ArchitectureARM64)) + acceptedArmFamilies := sets.New("t2a") + if arch == types.ArchitectureARM64 && !acceptedArmFamilies.Has(family) { + return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedArmFamilies)) + } + + if diskType != "" { + if !sets.New(diskTypes...).Has(diskType) { + return field.Invalid( + fldPath.Child("diskType"), + diskType, + fmt.Sprintf("%s instance requires one of the following disk types: %v", instanceType, diskTypes), + ) } } return nil @@ -104,16 +114,8 @@ func ValidateInstanceType(client API, fieldPath *field.Path, project, region str return append(allErrs, field.InternalError(nil, err)) } - instanceTypes := []string{} - ok := false - if diskType != "" { - instanceTypes, ok = gcp.DiskTypeToInstanceTypeMap[diskType] - if !ok { - return append(allErrs, field.NotFound(fieldPath.Child("diskType"), diskType)) - } - } - if err := validateInstanceType(fieldPath, instanceType, instanceTypes, arch); err != nil { - allErrs = append(allErrs, err) + if fieldErr := validateInstanceAndDiskType(fieldPath, diskType, instanceType, arch); fieldErr != nil { + return append(allErrs, fieldErr) } userZones := sets.New(zones...) @@ -226,18 +228,28 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList } } } - allErrs = append(allErrs, - ValidateInstanceType( - client, - field.NewPath("controlPlane", "platform", "gcp"), - ic.GCP.ProjectID, - ic.GCP.Region, - zones, - cpDiskType, - instanceType, - controlPlaneReq, - arch, - )...) + + // The IOPS minimum Control plane requirements are not met for pd-standard machines. + if cpDiskType == "pd-standard" { + allErrs = append(allErrs, + field.NotSupported(field.NewPath("controlPlane", "type"), + cpDiskType, + sets.List(gcp.ControlPlaneSupportedDisks)), + ) + } else { + allErrs = append(allErrs, + ValidateInstanceType( + client, + field.NewPath("controlPlane", "platform", "gcp"), + ic.GCP.ProjectID, + ic.GCP.Region, + zones, + cpDiskType, + instanceType, + controlPlaneReq, + arch, + )...) + } for idx, compute := range ic.Compute { fieldPath := field.NewPath("compute").Index(idx) diff --git a/pkg/asset/installconfig/gcp/validation_test.go b/pkg/asset/installconfig/gcp/validation_test.go index 4cccaecc404..da2ae913294 100644 --- a/pkg/asset/installconfig/gcp/validation_test.go +++ b/pkg/asset/installconfig/gcp/validation_test.go @@ -93,6 +93,10 @@ var ( ic.Platform.GCP.DefaultMachinePlatform.InstanceType = "n1-dne-1" } + invalidateControlPlaneDiskTypes = func(ic *types.InstallConfig) { + ic.ControlPlane.Platform.GCP.DiskType = "pd-standard" + } + invalidateNetwork = func(ic *types.InstallConfig) { ic.GCP.Network = "invalid-vpc" } invalidateComputeSubnet = func(ic *types.InstallConfig) { ic.GCP.ComputeSubnet = "invalid-compute-subnet" } invalidateCPSubnet = func(ic *types.InstallConfig) { ic.GCP.ControlPlaneSubnet = "invalid-cp-subnet" } @@ -234,6 +238,12 @@ func TestGCPInstallConfigValidation(t *testing.T) { expectedError: true, expectedErrMsg: `\[platform.gcp.defaultMachinePlatform.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 4 vCPUs, platform.gcp.defaultMachinePlatform.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 15360 MB Memory, controlPlane.platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 4 vCPUs, controlPlane.platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 15360 MB Memory, compute\[0\].platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 2 vCPUs, compute\[0\].platform.gcp.type: Invalid value: "n1-standard-1": instance type does not meet minimum resource requirements of 7680 MB Memory\]`, }, + { + name: "Invalid control plane machine disk types", + edits: editFunctions{validMachineTypes, invalidateControlPlaneDiskTypes}, + expectedError: true, + expectedErrMsg: `controlPlane.type: Unsupported value: "pd-standard": supported values: "hyperdisk-balanced", "pd-balanced", "pd-ssd"`, + }, { name: "Invalid control plane machine types", edits: editFunctions{invalidateControlPlaneMachineTypes}, @@ -851,7 +861,7 @@ func TestValidateInstanceType(t *testing.T) { instanceType: "n2-standard-4", diskType: "hyperdisk-balanced", expectedError: true, - expectedErrMsg: `[instance.diskType: Unsupported value: "n2": supported values: "c3", "c3d", "c4", m1", "n4"]`, + expectedErrMsg: `^\[instance.diskType: Invalid value: "hyperdisk\-balanced": n2\-standard\-4 instance requires one of the following disk types: \[pd\-standard pd\-ssd pd\-balanced\]\]$`, }, } diff --git a/pkg/types/gcp/machinepools.go b/pkg/types/gcp/machinepools.go index f9390226cb8..fac0e83b9de 100644 --- a/pkg/types/gcp/machinepools.go +++ b/pkg/types/gcp/machinepools.go @@ -27,13 +27,23 @@ var ( // ComputeSupportedDisks contains the supported disk types for control plane nodes. ComputeSupportedDisks = sets.New(HyperDiskBalanced, PDBalanced, PDSSD, PDStandard) - // DiskTypeToInstanceTypeMap contains a map where the key is the Disk Type, and the values are a list of - // instance types that are supported by the installer and correlate to the Disk Type. - DiskTypeToInstanceTypeMap = map[string][]string{ - PDStandard: {"a2", "e2", "n1", "n2", "n2d", "t2a", "t2d"}, - PDSSD: {"a2", "a3", "c3", "c3d", "e2", "m1", "n1", "n2", "n2d", "t2a", "t2d"}, - PDBalanced: {"a2", "a3", "c3", "c3d", "e2", "m1", "n1", "n2", "n2d", "t2a", "t2d"}, - HyperDiskBalanced: {"c3", "c3d", "m1", "n4"}, + // InstanceTypeToDiskTypeMap contains a map where the key is the Instance Type, and the + // values are a list of disk types that are supported by the installer and correlate to the Instance Type. + InstanceTypeToDiskTypeMap = map[string][]string{ + "a2": {PDStandard, PDSSD, PDBalanced}, + "a3": {PDSSD, PDBalanced}, + "c2": {PDStandard, PDSSD, PDBalanced}, + "c2d": {PDStandard, PDSSD, PDBalanced}, + "c3": {PDSSD, PDBalanced, HyperDiskBalanced}, + "c3d": {PDSSD, PDBalanced, HyperDiskBalanced}, + "e2": {PDStandard, PDSSD, PDBalanced}, + "m1": {PDSSD, PDBalanced, HyperDiskBalanced}, + "n1": {PDStandard, PDSSD, PDBalanced}, + "n2": {PDStandard, PDSSD, PDBalanced}, + "n2d": {PDStandard, PDSSD, PDBalanced}, + "n4": {HyperDiskBalanced}, + "t2a": {PDStandard, PDSSD, PDBalanced}, + "t2d": {PDStandard, PDSSD, PDBalanced}, } )