CNTRLPLANE-3027: Decouple all platform boot image resolvers for dual-stream support#8709
CNTRLPLANE-3027: Decouple all platform boot image resolvers for dual-stream support#8709jparrill wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
|
@jparrill: This pull request references CNTRLPLANE-3027 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
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:
📝 WalkthroughWalkthroughThis PR refactors HyperShift's nodepool controllers to support multi-stream release payloads. It extends Suggested reviewers
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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
area/platform/openstack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hypershift-operator/controllers/nodepool/azure_test.go (2)
909-951:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame test data inconsistency in Gen1/Gen2 skip tests.
Tests "skip defaulting with Gen1 imageGeneration when no marketplace metadata" (lines 909-929) and "skip defaulting with Gen2 imageGeneration when no marketplace metadata" (lines 931-951) both pass
hasMarketplaceMetadata=truedespite the test names claiming "no marketplace metadata".These should likely use
hasMarketplaceMetadata=falseto match their stated intent.Suggested fix
{ name: "skip defaulting with Gen1 imageGeneration when no marketplace metadata", // ... - releaseImage: createMockReleaseImage("4.20.0", true), + releaseImage: createMockReleaseImage("4.20.0", false), expectedImageType: "", expectedMarketplaceImage: nil, }, { name: "skip defaulting with Gen2 imageGeneration when no marketplace metadata", // ... - releaseImage: createMockReleaseImage("4.20.0", true), + releaseImage: createMockReleaseImage("4.20.0", false), expectedImageType: "", expectedMarketplaceImage: nil, },🤖 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 `@hypershift-operator/controllers/nodepool/azure_test.go` around lines 909 - 951, The two tests "skip defaulting with Gen1 imageGeneration when no marketplace metadata" and "skip defaulting with Gen2 imageGeneration when no marketplace metadata" are passing hasMarketplaceMetadata=true to createMockReleaseImage but should simulate no marketplace metadata; update the call to createMockReleaseImage("4.20.0", false) in those test cases so the second argument is false, leaving the rest of the nodePool/AzureMarketplaceImage ImageGeneration setup unchanged.
892-907:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest data contradicts test name - appears to be a copy-paste error.
This test case passes
hasMarketplaceMetadata=truebut claims to test "skip defaulting when no marketplace metadata". Compare with the test at lines 992-1013 which has an identical setup (4.20.0,hasMarketplaceMetadata=true, emptyAzureVMImage) but expects defaulting to occur.Either:
- This test should use
hasMarketplaceMetadata=falseto match the test name, or- The test name is wrong and expectations need updating
Additionally, the current assertion only checks when
expectedImageType != "", so this test doesn't actually verify that defaults were NOT applied—it would pass even if the function applies defaults.Suggested fix if the intent is to test "no marketplace metadata"
{ name: "skip defaulting when no marketplace metadata for OCP >= 4.20 (current behavior)", nodePool: &hyperv1.NodePool{ // ... same ... }, - releaseImage: createMockReleaseImage("4.20.0", true), + releaseImage: createMockReleaseImage("4.20.0", false), expectedImageType: "", expectedMarketplaceImage: nil, },🤖 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 `@hypershift-operator/controllers/nodepool/azure_test.go` around lines 892 - 907, The test case named "skip defaulting when no marketplace metadata for OCP >= 4.20 (current behavior)" is passing hasMarketplaceMetadata=true but should either set it false or be renamed and have expectations updated; update the call to createMockReleaseImage(...) to pass hasMarketplaceMetadata=false to match the name (or rename the test and adjust expectedImageType/expectedMarketplaceImage to reflect defaulting), and add an explicit assertion verifying defaults were NOT applied for AzureVMImage on the NodePool (e.g., assert expectedImageType == "" and expectedMarketplaceImage == nil) so the test fails if defaulting occurs; locate the case by the test name and references to AzureVMImage, createMockReleaseImage, expectedImageType, and expectedMarketplaceImage.
🧹 Nitpick comments (2)
hypershift-operator/controllers/nodepool/azure_test.go (1)
1100-1114: ⚡ Quick winTest assertions don't verify that defaults were NOT applied.
When
expectedImageTypeis empty orexpectedMarketplaceImageis nil, the assertions are skipped entirely. This means tests claiming to verify "skip defaulting" behavior don't actually check that the NodePool remains unmodified.Consider adding explicit assertions for skip scenarios.
Suggested stronger assertions
} else { g.Expect(err).ToNot(HaveOccurred()) - if tc.expectedImageType != "" { - g.Expect(tc.nodePool.Spec.Platform.Azure.Image.Type).To(Equal(tc.expectedImageType)) - } - if tc.expectedMarketplaceImage != nil { - g.Expect(tc.nodePool.Spec.Platform.Azure.Image.AzureMarketplace).To(Equal(tc.expectedMarketplaceImage)) - } + // Always verify image type and marketplace match expectations + g.Expect(tc.nodePool.Spec.Platform.Azure.Image.Type).To(Equal(tc.expectedImageType)) + if tc.expectedMarketplaceImage == nil { + // For skip scenarios, verify marketplace was not set (or unchanged) + // Note: may need to capture original value before calling defaultAzureNodePoolImage + g.Expect(tc.nodePool.Spec.Platform.Azure.Image.AzureMarketplace).To(BeNil()) + } else { + g.Expect(tc.nodePool.Spec.Platform.Azure.Image.AzureMarketplace).To(Equal(tc.expectedMarketplaceImage)) + } }Note: For tests where the NodePool already has a partial
AzureMarketplacestruct set (like the Gen1/Gen2 tests), you may need to capture the original state and verify it wasn't modified, or restructure those test cases.🤖 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 `@hypershift-operator/controllers/nodepool/azure_test.go` around lines 1100 - 1114, The test currently skips assertions when tc.expectedImageType is empty or tc.expectedMarketplaceImage is nil, so add explicit checks that no defaults were applied by capturing the original state of nodePool.Spec.Platform.Azure.Image (and its AzureMarketplace sub-struct) before running the code under test and then asserting after execution that the Image.Type and Image.AzureMarketplace are unchanged (for nil cases assert the field stays nil/zero and for partial structs compare equality to the captured original). Update the table-driven cases to, for each test case, store a preCall := tc.nodePool.Spec.Platform.Azure.Image (or pointer) and then use g.Expect(...).To(Equal(preCall)) / g.Expect(...).To(BeNil()) as appropriate when tc.expectedImageType == "" or tc.expectedMarketplaceImage == nil so tests verify "skip defaulting" precisely.hypershift-operator/controllers/nodepool/openstack/openstack.go (1)
165-175: 💤 Low valueConsider validating that
Releaseis non-empty.
OpenStackReleaseImagereturnsopenStack.Releasedirectly without checking if it's an empty string. If stream metadata contains an empty release version, this would silently propagate and result in image names likerhcos-(missing version suffix).🔧 Optional validation
func OpenStackReleaseImage(streamMeta *stream.Stream) (string, error) { arch, foundArch := streamMeta.Architectures["x86_64"] if !foundArch { return "", fmt.Errorf("couldn't find OS metadata for architecture %q", "x86_64") } openStack, exists := arch.Artifacts["openstack"] if !exists { return "", fmt.Errorf("couldn't find OS metadata for openstack") } + if openStack.Release == "" { + return "", fmt.Errorf("openstack artifact has empty release version") + } return openStack.Release, nil }🤖 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 `@hypershift-operator/controllers/nodepool/openstack/openstack.go` around lines 165 - 175, The OpenStackReleaseImage function currently returns openStack.Release without checking it's non-empty; add a validation after retrieving openStack (from stream.Stream->Architectures["x86_64"]->Artifacts["openstack"]) to trim and verify openStack.Release is not empty (e.g., strings.TrimSpace(openStack.Release) != ""), and if empty return a descriptive error (e.g., "openstack release is empty or missing in stream metadata") instead of returning an empty string so callers don't receive malformed image names.
🤖 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 `@hypershift-operator/controllers/nodepool/aws.go`:
- Around line 137-141: The code calls releaseImage.StreamForName("") (and later
uses releaseImage again around the defaultNodePoolAMI call) without checking
releaseImage for nil; add a nil guard before any method call on releaseImage
(e.g., in the block that currently calls StreamForName and where
defaultNodePoolAMI is invoked) and return a clear error (fmt.Errorf or
reconcile-friendly error/condition) when releaseImage == nil so reconciliation
fails gracefully instead of panicking; reference the releaseImage variable, the
StreamForName call, and the defaultNodePoolAMI invocation when applying the
guard.
In `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go`:
- Around line 574-575: The test setup currently ignores errors from
ri.StreamForName when assigning rhel9Stream and rhel10Stream; change those calls
to capture the error (e.g., rhel9Stream, err := ri.StreamForName("rhel-9") and
rhel10Stream, err := ri.StreamForName("rhel-10")) and assert/fail the test on
error (use your test helper like require.NoError/Expect/if err != nil {
t.Fatalf(...) }) so any fixture regressions surface immediately; reference the
ri variable and StreamForName function and replace the discarded-error
assignments for rhel9Stream and rhel10Stream accordingly.
In `@test/integration/osstreams/osstreams_test.go`:
- Around line 324-327: The "GCP" subtest in TestBootImageStreamResolution is
using the parent Gomega instance `g` so failures aren't tied to the subtest;
inside the `t.Run("GCP", ...)` closure, create a subtest-scoped Gomega with `g
:= NewWithT(t)` and replace uses of the parent `g.Expect(...)` (e.g. around
`multiStreamRelease.StreamForName("rhel-9")` and `StreamForName("rhel-10")`
checks for `rhel9Stream` and `rhel10Stream`) with the new `g.Expect(...)` so
assertions are attributed to the "GCP" subtest.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/azure_test.go`:
- Around line 909-951: The two tests "skip defaulting with Gen1 imageGeneration
when no marketplace metadata" and "skip defaulting with Gen2 imageGeneration
when no marketplace metadata" are passing hasMarketplaceMetadata=true to
createMockReleaseImage but should simulate no marketplace metadata; update the
call to createMockReleaseImage("4.20.0", false) in those test cases so the
second argument is false, leaving the rest of the nodePool/AzureMarketplaceImage
ImageGeneration setup unchanged.
- Around line 892-907: The test case named "skip defaulting when no marketplace
metadata for OCP >= 4.20 (current behavior)" is passing
hasMarketplaceMetadata=true but should either set it false or be renamed and
have expectations updated; update the call to createMockReleaseImage(...) to
pass hasMarketplaceMetadata=false to match the name (or rename the test and
adjust expectedImageType/expectedMarketplaceImage to reflect defaulting), and
add an explicit assertion verifying defaults were NOT applied for AzureVMImage
on the NodePool (e.g., assert expectedImageType == "" and
expectedMarketplaceImage == nil) so the test fails if defaulting occurs; locate
the case by the test name and references to AzureVMImage,
createMockReleaseImage, expectedImageType, and expectedMarketplaceImage.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/azure_test.go`:
- Around line 1100-1114: The test currently skips assertions when
tc.expectedImageType is empty or tc.expectedMarketplaceImage is nil, so add
explicit checks that no defaults were applied by capturing the original state of
nodePool.Spec.Platform.Azure.Image (and its AzureMarketplace sub-struct) before
running the code under test and then asserting after execution that the
Image.Type and Image.AzureMarketplace are unchanged (for nil cases assert the
field stays nil/zero and for partial structs compare equality to the captured
original). Update the table-driven cases to, for each test case, store a preCall
:= tc.nodePool.Spec.Platform.Azure.Image (or pointer) and then use
g.Expect(...).To(Equal(preCall)) / g.Expect(...).To(BeNil()) as appropriate when
tc.expectedImageType == "" or tc.expectedMarketplaceImage == nil so tests verify
"skip defaulting" precisely.
In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 165-175: The OpenStackReleaseImage function currently returns
openStack.Release without checking it's non-empty; add a validation after
retrieving openStack (from
stream.Stream->Architectures["x86_64"]->Artifacts["openstack"]) to trim and
verify openStack.Release is not empty (e.g.,
strings.TrimSpace(openStack.Release) != ""), and if empty return a descriptive
error (e.g., "openstack release is empty or missing in stream metadata") instead
of returning an empty string so callers don't receive malformed image names.
🪄 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: 143e0dcd-329f-4ebe-89a1-d75e3ccf9b62
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (31)
go.modhypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/fake/fake.gosupport/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yamlsupport/releaseinfo/fixtures/fixtures.gosupport/releaseinfo/registry_image_content_policies_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.gosupport/releaseinfo/testutils/testutils.gotest/integration/osstreams/osstreams_test.go
| rhel9Stream, err := multiStreamRelease.StreamForName("rhel-9") | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| rhel10Stream, err := multiStreamRelease.StreamForName("rhel-10") | ||
| g.Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify assertion scope in this file and inspect nearby context.
rg -n -C3 't\.Run\("GCP"|NewWithT\(t\)|g\.Expect\(err\)' test/integration/osstreams/osstreams_test.goRepository: openshift/hypershift
Length of output: 4950
Use a subtest-scoped Gomega instance in the "GCP" subtest.
In test/integration/osstreams/osstreams_test.go (TestBootImageStreamResolution, t.Run("GCP", ...) around lines 323-327), the "GCP" assertions currently use the parent g (g.Expect(...)) instead of creating g := NewWithT(t) inside the "GCP" closure, so failures won’t be attributed cleanly to the right subtest.
🤖 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 `@test/integration/osstreams/osstreams_test.go` around lines 324 - 327, The
"GCP" subtest in TestBootImageStreamResolution is using the parent Gomega
instance `g` so failures aren't tied to the subtest; inside the `t.Run("GCP",
...)` closure, create a subtest-scoped Gomega with `g := NewWithT(t)` and
replace uses of the parent `g.Expect(...)` (e.g. around
`multiStreamRelease.StreamForName("rhel-9")` and `StreamForName("rhel-10")`
checks for `rhel9Stream` and `rhel10Stream`) with the new `g.Expect(...)` so
assertions are attributed to the "GCP" subtest.
Source: Coding guidelines
jparrill
force-pushed
the
CNTRLPLANE-3027
branch
from
3b8fa2a to
87fe36f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/powervs_test.go (1)
121-125: ⚡ Quick winAssert the returned image fields in success paths.
The test only verifies
img != niland region, so regressions in returnedRelease/Object/Url(including dot→hyphen normalization) can slip through.Suggested test hardening
testCases := []struct { name string region string streamMeta *stream.Stream expectedRegion string + expectedRelease string + expectedObject string expectedErr string }{ { name: "When region maps to a valid COS region it should return the image and region", region: "syd", streamMeta: basicStream, expectedRegion: "au-syd", + expectedRelease: "9-8-20260403-0", + expectedObject: "rhcos-9-8-20260403-0-ppc64le-powervs.ova.gz", }, @@ } else { g.Expect(err).ToNot(HaveOccurred()) g.Expect(img).ToNot(BeNil()) g.Expect(region).To(Equal(tc.expectedRegion)) + g.Expect(img.Release).To(Equal(tc.expectedRelease)) + g.Expect(img.Object).To(Equal(tc.expectedObject)) }🤖 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 `@hypershift-operator/controllers/nodepool/powervs_test.go` around lines 121 - 125, The test's success branch only checks img != nil and region but must also assert the image fields to catch regressions: add expectations on img.Release, img.Object, and img.URL (or the exact struct field names used in powervs_test.go) to equal the testcase's expected values (e.g., tc.expectedRelease, tc.expectedObject, tc.expectedURL), and include a test case that exercises dot→hyphen normalization to validate the URL/object transformation; update the success path where g.Expect(img).ToNot(BeNil()) and g.Expect(region).To(Equal(tc.expectedRegion)) to also assert these image fields.
🤖 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.
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/powervs_test.go`:
- Around line 121-125: The test's success branch only checks img != nil and
region but must also assert the image fields to catch regressions: add
expectations on img.Release, img.Object, and img.URL (or the exact struct field
names used in powervs_test.go) to equal the testcase's expected values (e.g.,
tc.expectedRelease, tc.expectedObject, tc.expectedURL), and include a test case
that exercises dot→hyphen normalization to validate the URL/object
transformation; update the success path where g.Expect(img).ToNot(BeNil()) and
g.Expect(region).To(Equal(tc.expectedRegion)) to also assert these image fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: afc0063f-50be-411d-9a98-6039a8a8726a
📒 Files selected for processing (12)
hypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/token.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.gotest/integration/osstreams/osstreams_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- hypershift-operator/controllers/nodepool/token.go
- hypershift-operator/controllers/nodepool/azure.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- hypershift-operator/controllers/nodepool/openstack.go
- support/releaseinfo/releaseinfo.go
- hypershift-operator/controllers/nodepool/powervs.go
- support/releaseinfo/releaseinfo_test.go
- hypershift-operator/controllers/nodepool/openstack/openstack_test.go
|
Now I have all the evidence. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Konflux Enterprise Contract (EC) checks failed because the Root CauseThe The Enterprise Contract policy at
On PR #8709, the task result changed to
Recommendations
Evidence
|
jparrill
force-pushed
the
CNTRLPLANE-3027
branch
from
87fe36f to
ce1296e
Compare
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 `@hypershift-operator/controllers/nodepool/powervs.go`:
- Around line 169-172: When releaseImage.StreamForName("") fails, ensure you
update the NodePool condition NodePoolValidPlatformImageType to False before
returning so a stale True isn't left behind; locate the error branch after
calling releaseImage.StreamForName in powervs.go, set the NodePool's
NodePoolValidPlatformImageType condition (via whatever condition helper is used
in this controller) to False with an appropriate message including the error,
persist/update the NodePool status, then return the original fmt.Errorf("%w")
error to preserve existing error propagation.
🪄 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: bc21352a-320a-4d54-aaf4-50823e8cd30e
📒 Files selected for processing (26)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.gohypershift-operator/controllers/nodepool/token.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yamlsupport/releaseinfo/fixtures/fixtures.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.gotest/integration/osstreams/osstreams_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- support/releaseinfo/registry_mirror_provider.go
- hypershift-operator/controllers/nodepool/token.go
- support/releaseinfo/fixtures/fixtures.go
- hypershift-operator/controllers/nodepool/openstack.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- hypershift-operator/controllers/nodepool/gcp.go
- support/releaseinfo/deserialize.go
- hypershift-operator/controllers/nodepool/nodepool_controller.go
- support/releaseinfo/registryclient_provider.go
- hypershift-operator/controllers/nodepool/azure.go
- support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
- hypershift-operator/controllers/nodepool/openstack/openstack.go
- test/integration/osstreams/osstreams_test.go
- hypershift-operator/controllers/nodepool/openstack/openstack_test.go
…m resolution Add support for parsing the new multi-stream boot image ConfigMap format introduced in OCP 5.0 payloads. The ConfigMap now carries a "streams" key alongside the legacy "stream" key, mapping stream names (rhel-9, rhel-10) to per-architecture boot image metadata. - Update DeserializeImageMetadata to parse both "streams" and "stream" keys, returning the parsed OSStreams map alongside the default metadata - Add OSStreams field to ReleaseImage for holding per-stream metadata - Add StreamForName convenience method on ReleaseImage for stream lookup - Add GetRHELStream pure function implementing the stream resolution table from the dual-stream RHEL enhancement - Add 5.0 boot image fixture extracted from 5.0.0-ec.2 release payload - Fallback: when ConfigMap has only "streams" without "stream", populate StreamMetadata from the first available stream to prevent nil panics in platform consumers Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
Refactor defaultNodePoolAMI() to accept *stream.Stream instead of
*releaseinfo.ReleaseImage, decoupling the AWS boot image resolver
from the global StreamMetadata. Callers (ResolveAWSAMI,
setAWSConditions, setKarpenterAMILabels) resolve the stream
internally via StreamForName(""), preserving backward compatibility.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
jparrill
force-pushed
the
CNTRLPLANE-3027
branch
from
ce1296e to
d0050f1
Compare
…seImage
Apply the same pattern from CNTRLPLANE-3026 (AWS) to all remaining
platforms: GCP, Azure, PowerVS, OpenStack, and KubeVirt. Each leaf
function now accepts *stream.Stream instead of *releaseinfo.ReleaseImage,
and callers resolve the stream internally via StreamForName("").
- GCP: DefaultNodePoolGCPImage() exported, accepts *stream.Stream
- PowerVS: getPowerVSImage() accepts *stream.Stream
- Azure: getAzureMarketplaceMetadata() accepts *stream.Stream
- OpenStack: OpenstackDefaultImage(), OpenStackReleaseImage(),
PrefixedClusterImageName() accept *stream.Stream with nil guards
- KubeVirt: defaultImage() accepts *stream.Stream, GetImage()
resolves stream once and passes to both defaultImage and
openstack.OpenstackDefaultImage (fallback path)
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hypershift-operator/controllers/nodepool/nodepool_controller_test.go (1)
574-575:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck
StreamForNameerrors in test setup.Lines 574 and 575 discard errors, which can hide stream fixture regressions and make failures harder to diagnose.
Suggested fix
- rhel9Stream, _ := ri.StreamForName("rhel-9") - rhel10Stream, _ := ri.StreamForName("rhel-10") + rhel9Stream, err := ri.StreamForName("rhel-9") + if err != nil { + t.Fatalf("failed to resolve rhel-9 stream: %v", err) + } + rhel10Stream, err := ri.StreamForName("rhel-10") + if err != nil { + t.Fatalf("failed to resolve rhel-10 stream: %v", err) + }As per coding guidelines, "Always check errors in Go — don't ignore them."
🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go` around lines 574 - 575, The test currently ignores errors returned by StreamForName for "rhel-9" and "rhel-10"; update the setup to check those errors (capture the second return values) and fail the test if non-nil (e.g., use t.Fatalf/t.Fatalf-like assert or require.NoError) so rhel9Stream and rhel10Stream are only used when StreamForName succeeded; ensure you reference the returned error variables from StreamForName and replace the discarded "_" with proper error handling around rhel9Stream and rhel10Stream.Source: Coding guidelines
hypershift-operator/controllers/nodepool/aws.go (1)
137-141:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
releaseImagebeforeStreamForNameto avoid panic.Lines 137 and 368 call a method on
releaseImagewithout a nil check. If release metadata fetch fails upstream and nil is propagated, reconciliation panics instead of returning a controlled error/condition.Suggested fix
func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) { // TODO: Should the region be included in the NodePool platform information? region := hostedCluster.Spec.Platform.AWS.Region arch := nodePool.Spec.Arch if nodePool.Spec.Platform.AWS.AMI != "" { return nodePool.Spec.Platform.AWS.AMI, nil } // Use Windows AMI mapping when ImageType is set to Windows if nodePool.Spec.Platform.AWS.ImageType == hyperv1.ImageTypeWindows { ami, err := getWindowsAMI(region, arch, releaseImage) if err != nil { return "", fmt.Errorf("couldn't discover a Windows AMI for release image: %w", err) } return ami, nil } // Default behavior for Linux/RHCOS AMIs + if releaseImage == nil { + return "", fmt.Errorf("release image is nil") + } streamMeta, err := releaseImage.StreamForName("") if err != nil { return "", fmt.Errorf("couldn't resolve stream metadata: %w", err) }And in
setAWSConditions:} else { // Default behavior for Linux/RHCOS AMIs + if releaseImage == nil { + SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolValidPlatformImageType, + Status: corev1.ConditionFalse, + Reason: hyperv1.NodePoolValidationFailedReason, + Message: fmt.Sprintf("Release image metadata is nil for release image %q", nodePool.Spec.Release.Image), + ObservedGeneration: nodePool.Generation, + }) + return fmt.Errorf("release image is nil") + } streamMeta, err := releaseImage.StreamForName("")🤖 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 `@hypershift-operator/controllers/nodepool/aws.go` around lines 137 - 141, The code calls releaseImage.StreamForName("") without checking releaseImage for nil (seen around the call to StreamForName and subsequent call to defaultNodePoolAMI), which can cause a panic if upstream fetch failed; add a nil guard before calling StreamForName (e.g., if releaseImage == nil return a controlled error or set the appropriate condition) and propagate that error instead of dereferencing nil; apply the same nil-check pattern in setAWSConditions where releaseImage is used so both defaultNodePoolAMI and any releaseImage-dependent logic safely handle a nil releaseImage.
🤖 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 `@hypershift-operator/controllers/nodepool/gcp.go`:
- Around line 171-175: The code calls releaseImage.StreamForName("") without
guarding releaseImage for nil; update the reconciliation logic to check if
releaseImage is nil before calling StreamForName and return a clear error (or
requeue) if it is nil. Specifically, in the block around releaseImage and the
subsequent call to StreamForName and DefaultNodePoolGCPImage, add a nil-check on
releaseImage (and optionally on streamMeta after StreamForName) and return an
informative fmt.Errorf referencing releaseImage so the caller can handle the
failure instead of panicking.
---
Duplicate comments:
In `@hypershift-operator/controllers/nodepool/aws.go`:
- Around line 137-141: The code calls releaseImage.StreamForName("") without
checking releaseImage for nil (seen around the call to StreamForName and
subsequent call to defaultNodePoolAMI), which can cause a panic if upstream
fetch failed; add a nil guard before calling StreamForName (e.g., if
releaseImage == nil return a controlled error or set the appropriate condition)
and propagate that error instead of dereferencing nil; apply the same nil-check
pattern in setAWSConditions where releaseImage is used so both
defaultNodePoolAMI and any releaseImage-dependent logic safely handle a nil
releaseImage.
In `@hypershift-operator/controllers/nodepool/nodepool_controller_test.go`:
- Around line 574-575: The test currently ignores errors returned by
StreamForName for "rhel-9" and "rhel-10"; update the setup to check those errors
(capture the second return values) and fail the test if non-nil (e.g., use
t.Fatalf/t.Fatalf-like assert or require.NoError) so rhel9Stream and
rhel10Stream are only used when StreamForName succeeded; ensure you reference
the returned error variables from StreamForName and replace the discarded "_"
with proper error handling around rhel9Stream and rhel10Stream.
🪄 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: 41c0d560-9c9d-426d-8b9b-cd8b19a9e19f
📒 Files selected for processing (24)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.gohypershift-operator/controllers/nodepool/token.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yamlsupport/releaseinfo/fixtures/fixtures.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.go
✅ Files skipped from review due to trivial changes (1)
- support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- support/releaseinfo/registry_mirror_provider.go
- hypershift-operator/controllers/nodepool/token.go
- hypershift-operator/controllers/nodepool/stream.go
- support/releaseinfo/fixtures/fixtures.go
- support/releaseinfo/registryclient_provider.go
- hypershift-operator/controllers/nodepool/openstack.go
- hypershift-operator/controllers/nodepool/powervs_test.go
- support/releaseinfo/releaseinfo.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
- hypershift-operator/controllers/nodepool/azure.go
- hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
- hypershift-operator/controllers/nodepool/openstack/openstack_test.go
- support/releaseinfo/releaseinfo_test.go
- hypershift-operator/controllers/nodepool/openstack/openstack.go
- support/releaseinfo/deserialize_test.go
- hypershift-operator/controllers/nodepool/nodepool_controller.go
| streamMeta, err := releaseImage.StreamForName("") | ||
| if err != nil { | ||
| return "", fmt.Errorf("couldn't resolve stream metadata: %w", err) | ||
| } | ||
| image, err := DefaultNodePoolGCPImage(nodePool.Spec.Arch, streamMeta) |
There was a problem hiding this comment.
Guard releaseImage before StreamForName to avoid panic.
Line 171 calls a method on releaseImage without a nil check. If release metadata fetch fails upstream and nil is propagated, reconciliation panics instead of returning a controlled error.
Suggested fix
func resolveGCPImage(nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) {
gcpPlatform := nodePool.Spec.Platform.GCP
// If user specified a custom image, use it
if gcpPlatform.Image != "" {
return gcpPlatform.Image, nil
}
// Resolve image from release metadata
+ if releaseImage == nil {
+ return "", fmt.Errorf("release image is nil")
+ }
streamMeta, err := releaseImage.StreamForName("")
if err != nil {
return "", fmt.Errorf("couldn't resolve stream metadata: %w", err)
}🤖 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 `@hypershift-operator/controllers/nodepool/gcp.go` around lines 171 - 175, The
code calls releaseImage.StreamForName("") without guarding releaseImage for nil;
update the reconciliation logic to check if releaseImage is nil before calling
StreamForName and return a clear error (or requeue) if it is nil. Specifically,
in the block around releaseImage and the subsequent call to StreamForName and
DefaultNodePoolGCPImage, add a nil-check on releaseImage (and optionally on
streamMeta after StreamForName) and return an informative fmt.Errorf referencing
releaseImage so the caller can handle the failure instead of panicking.
d0050f1 to
ad091ee
Compare
2 participants
What this PR does / why we need it
Phase 1 step 3 (all remaining platforms) of the Dual-Stream RHEL 9/10 NodePool enhancement (OCPSTRAT-3014). Applies the same decoupling pattern from CNTRLPLANE-3026 (AWS) to GCP, Azure, PowerVS, OpenStack, and KubeVirt.
Each platform's leaf boot image resolver now accepts
*stream.Streaminstead of*releaseinfo.ReleaseImage. Callers resolve the stream internally viaStreamForName(""), preserving backward compatibility.Changes per platform:
defaultNodePoolGCPImage()— accepts*stream.StreamgetPowerVSImage()— accepts*stream.StreamgetAzureMarketplaceMetadata()— accepts*stream.StreamOpenstackDefaultImage(),OpenStackReleaseImage(),PrefixedClusterImageName()— accept*stream.Streamwith nil guardsdefaultImage()— accepts*stream.Stream;GetImage()resolves stream once and passes to bothdefaultImageandopenstack.OpenstackDefaultImage(fallback path)This completes Phase 1 step 3 of the enhancement for all platforms.
Which issue(s) this PR fixes
Fixes CNTRLPLANE-3027
Dependencies (must merge first)
Special notes for your reviewer:
OpenStackfunctions remain exported because they're called from thekubevirtpackage (existing pattern)Checklist
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes