Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OCPBUGS-41184: GCP Validate Disk and Instance Type #9043

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-41184: GCP Validate Disk and Instance Type #9043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 43 additions & 32 deletions pkg/asset/installconfig/gcp/validation.go
View file
Open in desktop
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 != "" {
Comment on lines +77 to 79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about inverting this if?

Suggested change
diskTypes := []string{}
ok := false
if instanceType != "" {
if instanceType == "" {
// nothing to validate, our defaults are already valid
return nil
}

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
diskTypes, ok = gcp.DiskTypeToInstanceTypeMap[family]
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))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}


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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if len(diskTypes) > 0 && diskType != "" {
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
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
Open in desktop
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
Open in desktop
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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is now backwards, but I understand not wanting to rename it.

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": {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},
}
)

Expand Down