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 9 commits into
Conversation
Replace 18 custom CoreOS stream metadata Go types with canonical equivalents from github.com/coreos/stream-metadata-go v0.4.11. This eliminates duplicated type definitions and gains upstream helper methods (e.g. GetAMI(), URN()), laying groundwork for the dual-stream RHEL NodePool feature. Key type mappings: - CoreOSStreamMetadata -> stream.Stream - CoreOSArchitecture -> stream.Arch - CoreRHCOSImage -> *rhcos.Extensions (pointer) - CoreOSAWSImages -> *stream.AwsImage (pointer) - CoreOSGCPImage -> *stream.GcpImage (pointer) - CoreOSPowerVSImage -> *stream.SingleObject - CoreOSKubevirtImages -> *stream.ContainerImage (pointer) - HyperVGen1/HyperVGen2 -> Gen1/Gen2 - SHA256 -> Sha256, URL -> Url (PowerVS) Pure refactor with no behavioral changes. All platform controllers updated with nil checks for pointer-typed upstream fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…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 Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Integration test that validates the complete flow from real 5.0 EC payload parsing through stream resolution to boot image lookup: - Parse multi-stream ConfigMap from 5.0.0-ec.2 payload fixture - Resolve RHEL stream via GetRHELStream for each scenario - Look up stream-specific boot images via StreamForName - Verify correct AWS AMI, GCP image, aarch64 AMI, and KubeVirt digest-ref for the resolved stream Covers realistic 5.0+ scenarios only (implicit default, explicit rhel-9/rhel-10, runc fallback, error cases) plus legacy 4.10 backward compatibility with real fixture data. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
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.
This is Phase 1 step 3 (AWS) of the dual-stream RHEL enhancement
(OCPSTRAT-3014). The same pattern will be applied to other platform
resolvers in CNTRLPLANE-3027.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Add TestBootImageStreamResolution with AWS subtests covering the
end-to-end flow from HostedCluster + NodePool + ReleaseImage through
ResolveAWSAMI. Tests multi-stream payload resolution, user AMI
override precedence, legacy 4.x backward compatibility, unsupported
architecture and missing region error paths. Structured with
t.Run("AWS", ...) so future providers (GCP, KubeVirt, etc.) are
added as new subtests in CNTRLPLANE-3027.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…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
- 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) <noreply@anthropic.com>
Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Extend TestBootImageStreamResolution with GCP subtests covering multi-stream image resolution (rhel-9, rhel-10), default stream backward compatibility, and nil metadata error path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR migrates HyperShift's release image metadata handling from internal CoreOS*-based types to the external Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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
| streamMeta, err := releaseImage.StreamForName("") | ||
| if err != nil { | ||
| return "", fmt.Errorf("couldn't resolve stream metadata: %w", err) | ||
| } | ||
| ami, err := defaultNodePoolAMI(region, arch, streamMeta) |
There was a problem hiding this comment.
Guard releaseImage before StreamForName to avoid panic.
Line 137 and Line 368 call 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/condition.
Suggested fix
func ResolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) {
@@
// 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)
}
@@
} 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("")
if err != nil {Also applies to: 368-379
🤖 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("") (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.
| rhel9Stream, _ := ri.StreamForName("rhel-9") | ||
| rhel10Stream, _ := ri.StreamForName("rhel-10") |
There was a problem hiding this comment.
Check StreamForName errors in test setup.
Line 574 and Line 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 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.
Source: Coding guidelines
| 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
…tream Add nil guards for streamMeta in getPowerVSImage, OpenstackDefaultImage, OpenStackReleaseImage, PrefixedClusterImageName, and defaultImage (kubevirt) matching the AWS/GCP pattern. Add NodePoolValidPlatformImageType status conditions for stream resolution failures in setPowerVSconditions and setOpenStackConditions, matching setAWSConditions. Replace manual architecture switch in Azure with hyperv1.ArchAliases. Move GetRHELStream from controllers/nodepool to support/releaseinfo to fix the dependency direction for Phase 2 consumers. Add unit tests for getPowerVSImage, getImageRegion, and kubevirt defaultImage error paths. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update integration test imports to reference GetRHELStream from support/releaseinfo instead of controllers/nodepool, fixing the dependency direction. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
|
3 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()— exported, accepts*stream.StreamgetPowerVSImage()— accepts*stream.StreamgetAzureMarketplaceMetadata()— accepts*stream.StreamOpenstackDefaultImage(),OpenStackReleaseImage(),PrefixedClusterImageName()— accept*stream.StreamdefaultImage()— accepts*stream.Stream;GetImage()resolves stream once and passes to bothdefaultImageandopenstack.OpenstackDefaultImage(fallback path)Integration tests:
TestBootImageStreamResolution/GCP— 4 test cases covering rhel-9/rhel-10 resolution, default stream backward compat, and nil metadata errorThis completes Phase 1 step 3 of the enhancement for all platforms.
Which issue(s) this PR fixes
Fixes CNTRLPLANE-3027
Dependencies (must merge first)
Checklist
Test plan
go build ./hypershift-operator/...— compilesgo test ./hypershift-operator/controllers/nodepool/... -count=1— all unit tests passgo test -tags=integration ./test/integration/osstreams/... -count=1— all integration tests passmake verify— lint and generation pass (only verify-git-clean expected to fail pre-merge)🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests
Chores