Skip to content

Commit

Permalink
OCPBUGS-41184: GCP Validate Disk and Instance Type
Browse files Browse the repository at this point in the history 
** 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).
  • Loading branch information
@barbacbd
barbacbd committed Sep 23, 2024
1 parent 10951c5 commit e68fbcb
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 39 deletions.
75 changes: 43 additions & 32 deletions pkg/asset/installconfig/gcp/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,29 @@ 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 {
diskTypes := []string{}
ok := false
if instanceType != "" {
family, _, _ := strings.Cut(instanceType, "-")
acceptedInstanceTypes := sets.New(validInstanceTypes...)
if !acceptedInstanceTypes.Has(family) {
return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedInstanceTypes))
diskTypes, ok = gcp.DiskTypeToInstanceTypeMap[family]
if !ok {
return field.NotFound(fldPath.Child("type"), family)
}

acceptedArmFamilies := sets.New("t2a")
if arch == types.ArchitectureARM64 && !acceptedArmFamilies.Has(family) {
return field.NotSupported(fldPath.Child("type"), family, sets.List(acceptedArmFamilies))
}
}

if arch == types.ArchitectureARM64 && family != "t2a" {
return field.InternalError(fldPath, fmt.Errorf("instance type %s requires %s architecture", instanceType, types.ArchitectureARM64))
if len(diskTypes) > 0 && 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
Expand All @@ -104,16 +113,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...)
Expand Down Expand Up @@ -226,18 +227,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").Child("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)
Expand Down
12 changes: 11 additions & 1 deletion pkg/asset/installconfig/gcp/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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\]\]$`,
},
}

Expand Down
22 changes: 16 additions & 6 deletions pkg/types/gcp/machinepools.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.
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"},
"a2": {PDStandard, PDSSD, PDBalanced},
"a3": {PDSSD, PDBalanced},
"c2": {PDSSD, PDSSD, PDBalanced},
"c2d": {PDSSD, 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},
}
)

Expand Down

0 comments on commit e68fbcb

Please sign in to comment.