ROSAENG-6150 | fix: local zone and wavelength zone subnets no longer listed when creating machine pool - #3462
Conversation
…listed when creating machine pool
📝 WalkthroughWalkthroughPrivate subnet retrieval now excludes Local Zone and Wavelength Zone subnets for Hosted Control Plane clusters. Availability-zone type lookups are cached. Excluded subnet IDs are reported, and retrieval returns an error when no eligible subnets remain. Subnet option selection and availability-zone resolution use the filtered subnet set. Tests cover Hosted Control Plane filtering, classic-cluster behavior, empty results, propagated errors, and updated AWS mock expectations. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: markirish The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/machinepool/helper_test.go`:
- Around line 406-414: The test case around getSubnetOptions must also verify
the exclusion warning, not only the filtered subnet results. Capture or inspect
the reporter output and assert that it includes both excluded subnet IDs,
“subnet-local” and “subnet-wavelength”, in sorted order while preserving the
existing subnet option assertions.
In `@pkg/machinepool/helper.go`:
- Around line 153-155: Update the error return in the GetAvailabilityZoneType
call to wrap the original error with %w and include the az value in the
contextual message, while preserving error propagation to callers.
🪄 Autofix
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: Pro Plus
Run ID: 375ec22c-5f3e-4b24-8201-9266986e7339
📒 Files selected for processing (3)
pkg/machinepool/helper.gopkg/machinepool/helper_test.gopkg/machinepool/machinepool_test.go
| It("excludes local zone and wavelength zone subnets", func() { | ||
| subnetOptions, err := getSubnetOptions(r, cluster) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(subnetOptions).To(HaveLen(2)) | ||
| for _, opt := range subnetOptions { | ||
| Expect(opt).ToNot(ContainSubstring("subnet-local")) | ||
| Expect(opt).ToNot(ContainSubstring("subnet-wavelength")) | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the exclusion warning.
This test verifies filtered subnet options only. It does not verify the required warning. A regression that removes Reporter.Warnf still passes.
Add a focused assertion that the warning includes the excluded subnet IDs in sorted order. The PR objective states that excluded subnets generate a warning. As per coding guidelines, “tests should prove correctness.”
🤖 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/machinepool/helper_test.go` around lines 406 - 414, The test case around
getSubnetOptions must also verify the exclusion warning, not only the filtered
subnet results. Capture or inspect the reporter output and assert that it
includes both excluded subnet IDs, “subnet-local” and “subnet-wavelength”, in
sorted order while preserving the existing subnet option assertions.
Source: Coding guidelines
| zoneType, err = r.AWSClient.GetAvailabilityZoneType(az) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'GetAvailabilityZoneType\(|fmt\.Errorf\(.*%w' pkg/machinepool pkg/awsRepository: openshift/rosa
Length of output: 12012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pkg/machinepool/helper.go imports and target function ---'
sed -n '1,210p' pkg/machinepool/helper.go
printf '%s\n' '--- AWS implementation ---'
sed -n '1125,1165p' pkg/aws/client.go
printf '%s\n' '--- nearby machinepool error-return patterns ---'
rg -n -C3 'return nil, err|fmt\.Errorf\(' pkg/machinepool/helper.go
printf '%s\n' '--- static check of the target error path ---'
python3 - <<'PY'
from pathlib import Path
p = Path("pkg/machinepool/helper.go")
text = p.read_text()
needle = 'zoneType, err = r.AWSClient.GetAvailabilityZoneType(az)'
pos = text.index(needle)
window = text[pos:pos + 220]
print(window)
print("fmt imported:", '"fmt"' in text.split(")", 1)[0])
print("target wraps with %w:", "fmt.Errorf" in window and "%w" in window)
PYRepository: openshift/rosa
Length of output: 14598
Wrap the GetAvailabilityZoneType error with context.
At pkg/machinepool/helper.go:155, use %w and include az so callers can identify the failed lookup while retaining the original error.
🤖 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/machinepool/helper.go` around lines 153 - 155, Update the error return in
the GetAvailabilityZoneType call to wrap the original error with %w and include
the az value in the contextual message, while preserving error propagation to
callers.
Source: Coding guidelines
1 participant
PR Summary
Exclude local zone and wavelength zone subnets from HCP machinepool creation
Detailed Description of the Issue
When creating a machinepool for a ROSA HCP cluster interactively, the subnet picker listed all private subnets in the VPC, including subnets in AWS Local Zones and Wavelength Zones. HCP clusters do not support nodepools in these zone types, so presenting them as options was misleading and could lead to failed machinepool creation.
The cluster creation path (cmd/create/cluster/cmd.go) already filtered these out using GetAvailabilityZoneType, but the machinepool creation path in pkg/machinepool/helper.go did not. This change adds the same filtering to both getSubnetOptions (interactive subnet list) and getSubnetFromAvailabilityZone (AZ-based subnet selection) for HCP clusters. Classic clusters are unaffected since they can legitimately have machinepools in local zones. Excluded subnets are reported to the user via a warning message, and an explicit error is returned if no subnets remain after filtering.
Related Issues and PRs
#Type of Change
Previous Behavior
When creating a machine pool in an HCP cluster, all subnets were listed, including those in local zones and wavelength zones.
Behavior After This Change
Local zones and wavelength zones are now filtered out when creating machine pools in HCP clusters
How to Test (Step-by-Step)
Preconditions
Test Steps
Expected Results
Proof of the Fix
Breaking Changes
Breaking Change Details / Migration Plan
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit