CORS-4537: Add API-backed disk type availability validation#10687
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4537 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesGCP disk availability and validation
GCP universe-domain handling
Azure generated metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/types/gcp/platform.go (1)
216-228: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMinor: avoid rebuilding the prefix set on every call.
sets.New(sovereignCloudProjectPrefixes...)allocates a new set from the same static slice on everyGetCloudEnvironmentcall. With only one entry today the cost is trivial, but it's wasted work on a function invoked repeatedly during defaulting/validation. Consider hoisting the set to a package-level var.♻️ Suggested refactor
var ( // sovereignCloudProjectPrefixes contains known project ID prefixes for sovereign clouds. // Project IDs in sovereign clouds use the format: <prefix>:<project-id> // This list helps distinguish from organization-scoped public GCP projects (orgname:project-id). sovereignCloudProjectPrefixes = []string{ "eu0", // European sovereign cloud (Germany) } + sovereignCloudProjectPrefixSet = sets.New(sovereignCloudProjectPrefixes...) ) func GetCloudEnvironment(projectID string) string { if strings.Contains(projectID, ":") { parts := strings.SplitN(projectID, ":", 2) - if len(parts) == 2 && sets.New(sovereignCloudProjectPrefixes...).Has(parts[0]) { + if len(parts) == 2 && sovereignCloudProjectPrefixSet.Has(parts[0]) { return CloudEnvironmentSovereign } } return "" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/types/gcp/platform.go` around lines 216 - 228, Hoist the set built from sovereignCloudProjectPrefixes into a package-level variable, then update GetCloudEnvironment to reuse that variable instead of calling sets.New on each invocation. Preserve the existing prefix parsing and CloudEnvironmentSovereign return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/installconfig/gcp/client.go`:
- Around line 230-267: Add direct unit tests for Client.GetDiskTypeWithZones
covering 404 responses that continue to later zones, accumulation of all
successful zone names, empty-region handling, and propagation of non-404 API
errors. Mock the compute service and GetZones dependencies while preserving the
method’s existing behavior and return values.
In `@pkg/asset/installconfig/gcp/validation.go`:
- Around line 249-251: Update the sovereign-cloud branch in the relevant
instance-type validation function to account for arch instead of always
returning c3-standard-4. For ARM64, select a sovereign-supported ARM instance
family or explicitly reject the unsupported default; preserve the existing
c3-standard-4 behavior only for architectures it supports and keep validation
consistent with the c3 ARM64 restriction.
In `@pkg/clusterapi/system.go`:
- Around line 310-321: Remove the later duplicate GetUniverseDomain handling
block following the new guarded logic in the surrounding setup function. Keep
the existing universeDomain retrieval and gcp.IsNonDefaultUniverseDomain check
as the sole source of GOOGLE_CLOUD_UNIVERSE_DOMAIN configuration, including its
error handling and logging.
In `@pkg/types/gcp/platform.go`:
- Around line 211-235: Add focused tests covering GetCloudEnvironment
sovereign-prefix detection and public/unknown project IDs,
IsNonDefaultUniverseDomain for empty, default, and custom domains, and
DefaultDiskTypeForInstanceAndProjectID plus SetMachinePoolDefaults for sovereign
disk selection and public/fallback behavior. Update tests in
pkg/types/gcp/platform.go (lines 211-235), pkg/types/gcp/machinepools.go (lines
375-412), and pkg/types/gcp/defaults/machinepool.go (line 38); no production
changes are required.
---
Nitpick comments:
In `@pkg/types/gcp/platform.go`:
- Around line 216-228: Hoist the set built from sovereignCloudProjectPrefixes
into a package-level variable, then update GetCloudEnvironment to reuse that
variable instead of calling sets.New on each invocation. Preserve the existing
prefix parsing and CloudEnvironmentSovereign return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 97589c8c-5219-485f-ac96-50c5b9fea387
📒 Files selected for processing (13)
pkg/asset/installconfig/azure/mock/azureclient_generated.gopkg/asset/installconfig/gcp/client.gopkg/asset/installconfig/gcp/mock/gcpclient_generated.gopkg/asset/installconfig/gcp/session.gopkg/asset/installconfig/gcp/validation.gopkg/asset/installconfig/gcp/validation_test.gopkg/asset/machines/clusterapi.gopkg/asset/machines/master.gopkg/asset/machines/worker.gopkg/clusterapi/system.gopkg/types/gcp/defaults/machinepool.gopkg/types/gcp/machinepools.gopkg/types/gcp/platform.go
|
/cc @rochacbruno |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
|
||
| func defaultGCPMachinePoolPlatform(arch types.Architecture) gcptypes.MachinePool { | ||
| instanceType := icgcp.DefaultInstanceTypeForArch(arch) | ||
| func defaultGCPMachinePoolPlatform(arch types.Architecture, projectID, region string) gcptypes.MachinePool { |
There was a problem hiding this comment.
| func defaultGCPMachinePoolPlatform(arch types.Architecture, projectID, region string) gcptypes.MachinePool { | |
| func defaultGCPMachinePoolPlatform(arch types.Architecture, projectID) gcptypes.MachinePool { |
We can remove region as it is unused, right?
| if gcp.IsNonDefaultUniverseDomain(universeDomain) { | ||
| capgEnvVars["GOOGLE_CLOUD_UNIVERSE_DOMAIN"] = universeDomain | ||
| logrus.Infof("setting GOOGLE_CLOUD_UNIVERSE_DOMAIN to %s for capg infrastructure controller", universeDomain) |
There was a problem hiding this comment.
Did you mean to include this change or it should be removed to limit the PR scope to disk validations?
| // Check if project ID has a known sovereign cloud prefix | ||
| if strings.Contains(projectID, ":") { | ||
| parts := strings.SplitN(projectID, ":", 2) | ||
| if len(parts) == 2 && sets.New(sovereignCloudProjectPrefixes...).Has(parts[0]) { | ||
| // Known sovereign prefix is definitive - this IS a sovereign cloud project | ||
| return CloudEnvironmentSovereign | ||
| } | ||
| } |
There was a problem hiding this comment.
We can skip the strings.Contains(projectID, ":") check because strings.splitN will return 1 part without :, thus making the if on line 228 false (same as GetDefaultServiceAccount).
// Domain-scoped project IDs (e.g. "eu0:openshift") use a reversed
// dot-separated format in SA emails: "openshift.eu0".
if parts := strings.SplitN(projectID, ":", 2); len(parts) == 2 && sets.New(sovereignCloudProjectPrefixes...).Has(parts[0]) {
// Known sovereign prefix is definitive - this IS a sovereign cloud project
return CloudEnvironmentSovereign
}| func DefaultDiskTypeForInstanceAndProjectID(instanceType, projectID string) string { | ||
| defaultDiskType := PDSSD | ||
| diskTypes, ok := GetDiskTypes(instanceType) | ||
| if ok { | ||
| supportedDiskTypes := sets.New(diskTypes...) | ||
| switch { | ||
| case supportedDiskTypes.Has(PDSSD): | ||
| defaultDiskType = PDSSD | ||
| case supportedDiskTypes.Has(HyperDiskBalanced): | ||
| defaultDiskType = HyperDiskBalanced | ||
| default: | ||
| // this shouldn't happen because all supported instance types | ||
| // have either pd-ssd or hyperdisk balanced | ||
| defaultDiskType = diskTypes[0] | ||
| cloudEnv := GetCloudEnvironment(projectID) | ||
|
|
||
| // Sovereign cloud prefers hyperdisk-balanced | ||
| if cloudEnv == CloudEnvironmentSovereign { | ||
| switch { |
There was a problem hiding this comment.
I notice the two branches differ only in preference order. How about below where we define a precedence slice of disk types?
func DefaultDiskTypeForInstanceAndProjectID(instanceType, projectID string) string {
diskTypes, ok := GetDiskTypes(instanceType)
if !ok {
return PDSSD
}
preferred := []string{PDSSD, HyperDiskBalanced}
if GetCloudEnvironment(projectID) == CloudEnvironmentSovereign {
preferred = []string{HyperDiskBalanced, PDSSD}
}
supportedDiskTypes := sets.New(diskTypes...)
for _, dt := range preferred {
if supportedDiskTypes.Has(dt) {
return dt
}
}
return diskTypes[0]
}| userZones := sets.New(zones...) | ||
| if len(userZones) > 0 { | ||
| if diff := userZones.Difference(dtZones); len(diff) > 0 { | ||
| errMsg := fmt.Sprintf("disk type %s is not available in zones: %v", diskType, sets.List(diff)) | ||
| allErrs = append(allErrs, field.Invalid(fieldPath.Child("diskType"), diskType, errMsg)) | ||
| } | ||
| } |
There was a problem hiding this comment.
When the user doesn't define zones, the installer defaults to use zones where machine type is available, right 👇?
installer/pkg/asset/installconfig/gcp/validation.go
Lines 161 to 168 in 91e9633
In that case, the base zone list (if not defined by users) should be zones where machine type is available. For example, we can:
// Move validateDiskTypeAvailability call after line 171
// Variable userZones is either user-defined or default zones
// where machine type is available so we should pass that in
// instead of variable zones.
allErrs = append(allErrs, validateDiskTypeAvailability(client,
fieldPath, project, region,
sets.List(userZones), // We use the userZones variable instead of zones
diskType)...)| logrus.Warnf("Unable to validate disk type %s availability: %v", diskType, err) | ||
| return allErrs |
There was a problem hiding this comment.
I think non-API errors (e.g. DNS failures, TCP timeouts, TLS handshake errors, etc) should fail the validation right? That is: field.InternalError.
We should fail fast so bad combination won't come up late during machine provisioning 🤔
| if isSovereignCloud && osImage.Project == "" { | ||
| allErrs = append(allErrs, field.Required( | ||
| field.NewPath("controlPlane", "platform", "gcp", "osImage", "project"), | ||
| "must specify image project for sovereign cloud")) | ||
| } | ||
| if osImage.Project != "" { | ||
| image, err = client.GetImage(context.TODO(), osImage.Name, osImage.Project) | ||
| if err != nil { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("controlPlane", "platform", "gcp", "osImage"), *osImage, fmt.Sprintf(errorMessage, err))) | ||
| } | ||
| } | ||
| } else if isSovereignCloud && defaultOsImage == nil { |
There was a problem hiding this comment.
I guess we can drop the validations that requires BYO osImage in GCD since we have #10686, right?
There was a problem hiding this comment.
If we plan to keep only if isSovereignCloud && osImage.Project == "". Then, we should only validate it for controlPlane and compute pool since users can populate directly.
Also, defaultMachinePlatform is already merged into each controlPlane and compute pool before validation runs. So, by the time validateInstanceTypes runs, each controlPlane and compute pool already has the defaultMachinePlatform values baked in.
installer/pkg/types/gcp/defaults/machinepool.go
Lines 9 to 14 in 91e9633
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/asset/installconfig/gcp/validation.go`:
- Around line 201-206: Update the disk lookup error handling in
pkg/asset/installconfig/gcp/validation.go lines 201-206: preserve the
googleapi.Error validation error, but log a warning and return no
disk-availability field error for other failures. Update the network-timeout
expectation in pkg/asset/installconfig/gcp/validation_test.go lines 1451-1455 to
assert that validation succeeds without an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b604c75e-b166-4cb5-bcaa-da7855b17d8f
📒 Files selected for processing (8)
pkg/asset/installconfig/azure/mock/azureclient_generated.gopkg/asset/installconfig/gcp/client.gopkg/asset/installconfig/gcp/mock/gcpclient_generated.gopkg/asset/installconfig/gcp/validation.gopkg/asset/installconfig/gcp/validation_test.gopkg/clusterapi/system.gopkg/types/gcp/machinepools_test.gopkg/types/gcp/platform_test.go
💤 Files with no reviewable changes (1)
- pkg/clusterapi/system.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/types/gcp/machinepools_test.go
- pkg/asset/installconfig/azure/mock/azureclient_generated.go
- pkg/asset/installconfig/gcp/mock/gcpclient_generated.go
- pkg/types/gcp/platform_test.go
- pkg/asset/installconfig/gcp/client.go
| // Google Cloud Dedicated support: detect universe domain from | ||
| // credentials and pass it to the CAPG controller via env var. | ||
| ud, err := session.Credentials.GetUniverseDomain() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get universe domain from gcp credentials: %w", err) |
There was a problem hiding this comment.
Maybe we don't wanna remove this code block. Let's drop the changes in this file?
Disk type validation was previously entirely static, relying on hardcoded maps that encode which disk types are compatible with which instance families. This works for public GCP where all standard disk types are available in every region, but fails for sovereign clouds where regional availability differs. This change adds an API-backed availability check that queries each zone individually via DiskTypes.Get, following the existing pattern established by GetMachineTypeWithZones for machine type validation. The two validation layers are complementary: 1. Static check (validateInstanceAndDiskType): Is this disk type compatible with this instance family? Catches combinations that never work anywhere. 2. API check (validateDiskTypeAvailability): Is this disk type available in this specific region and zones? Catches disk types that are valid in general but unavailable in the target region. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling required tests: Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
Let's get some e2e runs to see whether this change requires any new permissions 👀
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/retest-required |
1 similar comment
|
/retest-required |
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
15956c8
into
openshift:main
5 participants
Disk type validation was previously entirely static, relying on
hardcoded maps (InstanceTypeToDiskTypeMap, ComputeSupportedDisks) that
encode which disk types are compatible with which instance families.
This works for public GCP where all standard disk types are available
in every region, but fails for sovereign clouds where regional
availability differs. For example, pd-ssd is not available in
u-germany-northeast1, but the static map would allow c3 + pd-ssd
since c3 supports pd-ssd in public GCP.
Summary by CodeRabbit
New Features
Bug Fixes
Tests