Skip to content

CNTRLPLANE-3552: Multi-stream CoreOS metadata parsing and stream resolution#8669

Draft
jparrill wants to merge 2 commits into
openshift:mainfrom
jparrill:CNTRLPLANE-3552
Draft

CNTRLPLANE-3552: Multi-stream CoreOS metadata parsing and stream resolution#8669
jparrill wants to merge 2 commits into
openshift:mainfrom
jparrill:CNTRLPLANE-3552

Conversation

@jparrill

@jparrill jparrill commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Phase 1 steps 1-2 of the Dual-Stream RHEL 9/10 NodePool enhancement (OCPSTRAT-3014). Teaches HyperShift to parse multi-stream release payloads and resolve which RHEL stream a NodePool should use.

Changes:

  • DeserializeImageMetadata() — now returns (*stream.Stream, map[string]*stream.Stream, error), parsing both the legacy stream key and the new multi-stream streams key
  • ReleaseImage.OSStreams — new field holding per-stream metadata (nil for OCP < 5.0)
  • ReleaseImage.StreamForName(name) — resolves stream metadata by name; empty name returns the default stream
  • GetRHELStream(explicitStream, releaseVersion, usesRunc) — resolves which RHEL stream to use based on explicit selection, release version, and runc usage
  • RegistryMirrorProviderDecorator — propagates OSStreams to prevent silent data loss on mirror-configured clusters
  • 5.0 fixture — multi-stream ConfigMap with rhel-9 and rhel-10 streams (x86_64, aarch64, ppc64le)

Tests:

  • Unit tests: TestDeserializeImageMetadata (11 cases), TestDeserializeImageMetadataMultiStreamContent (9 cases), TestStreamForName (6 cases), TestGetRHELStream (17 cases with error message assertions)
  • Integration tests: TestOSStreamsEndToEnd (8 cases), TestOSStreamsStreamForNameErrors (2 cases), TestOSStreamsLegacyPayloadBackwardCompat (2 cases)

Which issue(s) this PR fixes

Fixes CNTRLPLANE-3552

Dependencies (must merge first)

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.
  • This change includes integration tests.

Test plan

  • go build ./support/releaseinfo/... ./hypershift-operator/controllers/nodepool/ — compiles
  • go test ./support/releaseinfo/ -count=1 — unit tests pass
  • go test ./hypershift-operator/controllers/nodepool/ -run TestGetRHELStream -count=1 — unit tests pass
  • go test -tags=integration ./test/integration/osstreams/... -count=1 — integration tests pass
  • make lint-fix && make verify — 0 lint issues

🤖 Generated with Claude Code

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 4, 2026
@openshift-ci-robot

openshift-ci-robot commented Jun 4, 2026

Copy link
Copy Markdown

@jparrill: This pull request references CNTRLPLANE-3552 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.

Details

In response to this:

Fixes

Summary

  • Add multi-stream boot image ConfigMap parsing to DeserializeImageMetadata — supports both the legacy stream key (OCP < 5.0) and the new streams key (OCP >= 5.0)
  • Add OSStreams field to ReleaseImage and StreamForName() method for stream-specific boot image 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
  • Integration test validating end-to-end flow: parse payload → resolve stream → look up boot images

Phase 1 of CNTRLPLANE-3018 (epic). No API changes, no platform wiring — parsing and resolution logic only. This code is not reachable in production until Phase 2 connects GetRHELStream into the NodePool controller (NewToken(), validMachineConfigCondition). The multi-stream parsing only activates when a >= 5.0 payload carries the streams ConfigMap key.

Stream Resolution Table

explicit stream release runc result
unset 4.x - "" (legacy)
unset 5.x false rhel-10
unset 5.x true rhel-9 (fallback)
rhel-9 any - rhel-9
rhel-10 <5.0 - error
rhel-10 >=5.0 false rhel-10
rhel-10 >=5.0 true error

Test plan

  • Unit tests — parsing and stream resolution:
go test ./support/releaseinfo/... -v -count=1
go test -run TestGetRHELStream ./hypershift-operator/controllers/nodepool/ -v -count=1
  • Integration tests — end-to-end flow with real 5.0 EC payload:
go test -tags integration ./test/integration/osstreams/... -v -count=1

Note: requires -tags integration build tag. No cluster needed.

  • make lint-fix — 0 issues
  • make verify — passes

🤖 Generated with Claude Code

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 openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2026
@openshift-ci

openshift-ci Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR introduces multi-stream RHEL CoreOS image metadata support to the hypershift operator. It adds constants and resolution logic for rhel-9 and rhel-10 streams (GetRHELStream), extends the ReleaseImage data model with an OSStreams field and StreamForName lookup method, rewrites ConfigMap deserialization to parse both single-stream and multi-stream metadata from separate keys, integrates the parsed data into release info providers, adds defensive nil checks in image resolution code, and validates the entire flow with new OCP 5.0 fixture data and end-to-end integration tests covering happy paths, error cases, and backward compatibility with legacy OCP 4.10 payloads.

Sequence Diagram

sequenceDiagram
  participant NodePool as NodePool Controller
  participant GetRHELStream as GetRHELStream
  participant RegistryClient as RegistryClientProvider
  participant Deserialize as DeserializeImageMetadata
  participant ReleaseImage as ReleaseImage
  participant StreamForName as StreamForName
  participant ImageResolver as Image Resolver (kubevirt/openstack/powervs)

  NodePool->>GetRHELStream: explicitStream, releaseVersion, usesRunc
  GetRHELStream-->>NodePool: resolved stream name (or error)
  
  RegistryClient->>Deserialize: ConfigMap data bytes
  Deserialize-->>RegistryClient: (singleStreamMeta, multiStreamMap, error)
  RegistryClient->>ReleaseImage: populate StreamMetadata + OSStreams
  
  NodePool->>StreamForName: streamName
  StreamForName->>ReleaseImage: lookup in StreamMetadata or OSStreams
  StreamForName-->>NodePool: CoreOSStreamMetadata (or error)
  
  NodePool->>ImageResolver: resolved metadata
  ImageResolver->>ImageResolver: validate StreamMetadata not nil
  ImageResolver-->>NodePool: image artifact (AMI, GCP image, etc.)
Loading

Suggested reviewers

  • sdminonne
  • muraee
🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: multi-stream CoreOS metadata parsing and stream resolution logic added across the codebase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All test names in the PR are static string literals with no dynamic content; no fmt.Sprintf, variables, timestamps, UUIDs, or generated identifiers detected in test titles.
Test Structure And Quality ✅ Passed PR contains only standard Go unit tests (Test* functions with *testing.T), not Ginkgo tests. Check is not applicable.
Topology-Aware Scheduling Compatibility ✅ Passed PR introduces only utility functions and metadata parsing—no deployment manifests, scheduling constraints, affinity rules, or topology constraints.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed This PR adds standard Go unit/integration tests only, not Ginkgo e2e tests. No It()/Describe()/Context() patterns found. The custom check does not apply.
No-Weak-Crypto ✅ Passed No weak crypto algorithms (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or non-constant-time secret comparisons found in any modified files.
Container-Privileges ✅ Passed PR contains only Go source/test files and a ConfigMap fixture with boot image metadata. No container manifests, deployment specs, or privileged security configurations found.
No-Sensitive-Data-In-Logs ✅ Passed No sensitive data in logs. Error messages log only public metadata (image URLs, AMI IDs, GCP names, release versions) with no passwords, tokens, keys, PII, or secrets.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci

openshift-ci Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release approved Indicates a PR has been approved by an approver from all required OWNERS files. area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Jun 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@support/releaseinfo/deserialize.go`:
- Around line 41-48: The code currently initializes a zero-value
CoreOSStreamMetadata and returns its address even when the "stream" key is
absent; update the logic in the json unmarshal block and final return so that
when hasStreamData is false the function returns (nil, osStreams, nil) instead
of &coreOSMeta. Concretely, only allocate or populate coreOSMeta when
hasStreamData and json.Unmarshal succeeds (inside the if hasStreamData { ... }
block using json.Unmarshal into a local variable), and change the final return
to return the pointer to that populated struct or nil when hasStreamData was
false (i.e., return nil, osStreams, nil).
🪄 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: 39f31f55-c3e5-4714-a09f-53a65264d50e

📥 Commits

Reviewing files that changed from the base of the PR and between e25a87a and e3e1dce.

📒 Files selected for processing (11)
  • hypershift-operator/controllers/nodepool/stream.go
  • hypershift-operator/controllers/nodepool/stream_test.go
  • support/releaseinfo/deserialize.go
  • support/releaseinfo/deserialize_test.go
  • support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
  • support/releaseinfo/fixtures/fixtures.go
  • support/releaseinfo/registry_mirror_provider.go
  • support/releaseinfo/registryclient_provider.go
  • support/releaseinfo/releaseinfo.go
  • support/releaseinfo/releaseinfo_test.go
  • test/integration/osstreams/osstreams_test.go

Comment thread support/releaseinfo/deserialize.go Outdated
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.96970% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.59%. Comparing base (35c0190) to head (728708c).

Files with missing lines Patch % Lines
support/releaseinfo/registryclient_provider.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8669      +/-   ##
==========================================
+ Coverage   41.55%   41.59%   +0.04%     
==========================================
  Files         758      759       +1     
  Lines       93851    93906      +55     
==========================================
+ Hits        39001    39064      +63     
+ Misses      52104    52099       -5     
+ Partials     2746     2743       -3     
Files with missing lines Coverage Δ
hypershift-operator/controllers/nodepool/stream.go 100.00% <100.00%> (ø)
support/releaseinfo/deserialize.go 90.00% <100.00%> (+50.00%) ⬆️
support/releaseinfo/registry_mirror_provider.go 45.83% <100.00%> (+2.35%) ⬆️
support/releaseinfo/releaseinfo.go 51.00% <100.00%> (+5.14%) ⬆️
support/releaseinfo/registryclient_provider.go 0.00% <0.00%> (ø)
Flag Coverage Δ
cmd-support 35.05% <95.23%> (+0.09%) ⬆️
cpo-hostedcontrolplane 43.59% <ø> (ø)
cpo-other 43.17% <ø> (ø)
hypershift-operator 51.70% <100.00%> (+0.04%) ⬆️
other 31.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jparrill jparrill force-pushed the CNTRLPLANE-3552 branch 2 times, most recently from 6651f3c to 3121c68 Compare June 4, 2026 15:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (3)
support/releaseinfo/deserialize_test.go (2)

20-26: ⚡ Quick win

Mark test helper as a helper for cleaner failure locations.

testConfigMap is a helper but does not mark itself with t.Helper(), which makes failures point to the helper body instead of the calling test.

Suggested change
-func testConfigMap(dataFields map[string]string) []byte {
+func testConfigMap(t *testing.T, dataFields map[string]string) []byte {
+	t.Helper()
 	cm := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: test\ndata:\n"
 	for k, v := range dataFields {
 		cm += fmt.Sprintf("  %s: %s\n", k, v)
 	}
 	return []byte(cm)
}
// Update call sites in this file:
data: testConfigMap(t, map[string]string{ ... })

As per coding guidelines, "Use t.Helper() in Go helper functions to improve error tracebacks".

🤖 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 `@support/releaseinfo/deserialize_test.go` around lines 20 - 26, testConfigMap
is a test helper but doesn't call t.Helper() or accept *testing.T, so failures
point into the helper; change the signature of testConfigMap to accept t
*testing.T (e.g., testConfigMap(t *testing.T, dataFields map[string]string)),
call t.Helper() at the top of that function, and update all call sites in this
file (places that call testConfigMap(...)) to pass the test variable (e.g.,
testConfigMap(t, map[string]string{...})).

28-123: ⚡ Quick win

Run these unit tests in parallel.

Both test functions and their subtests can run independently; adding t.Parallel() aligns with repository test guidance and reduces suite runtime.

Suggested change
func TestDeserializeImageMetadata(t *testing.T) {
+	t.Parallel()
 	tests := []struct {
 		...
 	}

 	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			g := NewWithT(t)
 			...
 		})
 	}
}

func TestDeserializeImageMetadataMultiStreamContent(t *testing.T) {
+	t.Parallel()
 	defaultStream, osStreams, err := DeserializeImageMetadata(fixtures.CoreOSBootImagesYAML_5_0)
 	...

 	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			tt.assert(NewWithT(t))
 		})
 	}
}

As per coding guidelines, "Unit tests should use race detection and parallel execution".

Also applies to: 125-229

🤖 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 `@support/releaseinfo/deserialize_test.go` around lines 28 - 123, Add parallel
execution to the tests: call t.Parallel() at the start of the top-level
TestDeserializeImageMetadata function and also inside each subtest goroutine
(the func passed to t.Run) so subtests run in parallel; do the same for the
other test function referenced (the one around lines 125-229). Locate the test
functions by name (TestDeserializeImageMetadata and the other test function in
the same file) and add t.Parallel() both at the top of each test and at the
start of each t.Run subtest closure, ensuring no shared mutable state is
assumed.
hypershift-operator/controllers/nodepool/stream_test.go (1)

11-171: ⚡ Quick win

Enable parallel execution for this table-driven test.

This suite is a good candidate for t.Parallel() at both parent and subtest level.

Suggested change
func TestGetRHELStream(t *testing.T) {
+	t.Parallel()
 	tests := []struct {
 		...
 	}

 	for _, tt := range tests {
+		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			g := NewWithT(t)

 			result, err := GetRHELStream(tt.explicitStream, tt.releaseVersion, tt.usesRunc)
 			...
 		})
 	}
}

As per coding guidelines, "Unit tests should use race detection and parallel execution".

🤖 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/stream_test.go` around lines 11 -
171, The test TestGetRHELStream should enable parallel execution to follow
guidelines; add t.Parallel() as the first statement in TestGetRHELStream and
also call t.Parallel() at the start of each subtest inside the t.Run closure so
each case runs concurrently; locate TestGetRHELStream and the anonymous func
passed to t.Run around the table loop and insert the t.Parallel() calls while
ensuring no shared mutable state is accessed when invoking GetRHELStream.
🤖 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/stream_test.go`:
- Around line 11-171: The test TestGetRHELStream should enable parallel
execution to follow guidelines; add t.Parallel() as the first statement in
TestGetRHELStream and also call t.Parallel() at the start of each subtest inside
the t.Run closure so each case runs concurrently; locate TestGetRHELStream and
the anonymous func passed to t.Run around the table loop and insert the
t.Parallel() calls while ensuring no shared mutable state is accessed when
invoking GetRHELStream.

In `@support/releaseinfo/deserialize_test.go`:
- Around line 20-26: testConfigMap is a test helper but doesn't call t.Helper()
or accept *testing.T, so failures point into the helper; change the signature of
testConfigMap to accept t *testing.T (e.g., testConfigMap(t *testing.T,
dataFields map[string]string)), call t.Helper() at the top of that function, and
update all call sites in this file (places that call testConfigMap(...)) to pass
the test variable (e.g., testConfigMap(t, map[string]string{...})).
- Around line 28-123: Add parallel execution to the tests: call t.Parallel() at
the start of the top-level TestDeserializeImageMetadata function and also inside
each subtest goroutine (the func passed to t.Run) so subtests run in parallel;
do the same for the other test function referenced (the one around lines
125-229). Locate the test functions by name (TestDeserializeImageMetadata and
the other test function in the same file) and add t.Parallel() both at the top
of each test and at the start of each t.Run subtest closure, ensuring no shared
mutable state is assumed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c73114df-faea-4ddf-bd9d-f0e03b64c080

📥 Commits

Reviewing files that changed from the base of the PR and between e3e1dce and 6651f3c.

📒 Files selected for processing (11)
  • hypershift-operator/controllers/nodepool/stream.go
  • hypershift-operator/controllers/nodepool/stream_test.go
  • support/releaseinfo/deserialize.go
  • support/releaseinfo/deserialize_test.go
  • support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
  • support/releaseinfo/fixtures/fixtures.go
  • support/releaseinfo/registry_mirror_provider.go
  • support/releaseinfo/registryclient_provider.go
  • support/releaseinfo/releaseinfo.go
  • support/releaseinfo/releaseinfo_test.go
  • test/integration/osstreams/osstreams_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
  • support/releaseinfo/fixtures/fixtures.go
  • support/releaseinfo/releaseinfo_test.go
  • support/releaseinfo/releaseinfo.go
  • support/releaseinfo/registryclient_provider.go
  • support/releaseinfo/registry_mirror_provider.go
  • hypershift-operator/controllers/nodepool/stream.go
  • support/releaseinfo/deserialize.go
  • support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
  • test/integration/osstreams/osstreams_test.go

@jparrill

jparrill commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/hold

We need to discuss what's the best approach for the dependent PR #8673

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2026
@openshift-ci openshift-ci Bot added area/documentation Indicates the PR includes changes for documentation area/platform/aws PR/issue for AWS (AWSPlatform) platform area/platform/azure PR/issue for Azure (AzurePlatform) platform area/platform/gcp PR/issue for GCP (GCPPlatform) platform area/platform/kubevirt PR/issue for KubeVirt (KubevirtPlatform) platform area/platform/openstack PR/issue for OpenStack (OpenStackPlatform) platform area/platform/powervs PR/issue for PowerVS (PowerVSPlatform) platform labels Jun 8, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2026
@jparrill

jparrill commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/hold

Until dependent PR got merged.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 (1)
hypershift-operator/controllers/nodepool/azure_test.go (1)

892-950: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

These fixtures no longer cover the “no marketplace metadata” path.

createMockReleaseImage(..., true) now builds RHELCoreOSExtensions.Marketplace, so all three cases in this block hit the defaulting path instead of the “metadata absent” path their assertions describe. With the updated helper, defaultAzureNodePoolImage should populate AzureMarketplace here, so these expectations are now wrong.

Suggested fix
-			releaseImage:             createMockReleaseImage("4.20.0", true),
+			releaseImage:             createMockReleaseImage("4.20.0", false),
...
-			releaseImage:             createMockReleaseImage("4.20.0", true),
+			releaseImage:             createMockReleaseImage("4.20.0", false),
...
-			releaseImage:             createMockReleaseImage("4.20.0", true),
+			releaseImage:             createMockReleaseImage("4.20.0", false),
🤖 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 -
950, The tests in azure_test.go expecting the "no marketplace metadata" path are
now incorrect because createMockReleaseImage(..., true) populates
RHELCoreOSExtensions.Marketplace, so defaultAzureNodePoolImage will perform
defaulting and populate AzureMarketplace; update the test fixtures to either
call createMockReleaseImage(..., false) or adjust the expected results to
reflect that AzureMarketplace will be populated (i.e., set expectedImageType to
hyperv1.AzureMarketplace and set expectedMarketplaceImage to the expected
populated AzureMarketplace struct), and update the three test cases referencing
createMockReleaseImage(..., true) accordingly.
🤖 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/kubevirt/kubevirt.go`:
- Around line 53-57: The code dereferences releaseImage.StreamMetadata before
ensuring releaseImage/StreamMetadata is non-nil, which can panic; add a guard at
the start of the function handling the release image (check releaseImage != nil
&& releaseImage.StreamMetadata != nil) before using
releaseImage.StreamMetadata.ByArchitecture and return a descriptive error if
absent, then proceed to look up arch and keep the existing KubeVirt nil-check on
arch.Images.KubeVirt.DigestRef/ containerImage to avoid panics in functions
referencing releaseImage, StreamMetadata, and arch.

In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 147-151: OpenstackDefaultImage currently dereferences
releaseImage.StreamMetadata earlier and may panic; update OpenstackDefaultImage
to check that releaseImage and releaseImage.StreamMetadata are non-nil before
accessing StreamMetadata fields and return a descriptive error if missing, and
apply the same nil-guard pattern in OpenStackReleaseImage (check releaseImage !=
nil and releaseImage.StreamMetadata != nil) so both functions fail fast with an
error instead of panicking; keep the existing artifact.Disk nil check intact and
reuse the same error wording/handling style as used for
artifact.Disk.Location/Sha256.

In `@support/releaseinfo/registryclient_provider.go`:
- Around line 39-47: DeserializeImageMetadata can return a nil coreOSMeta for
payloads that only include streams; don't return a ReleaseImage with
StreamMetadata nil. In Lookup (the shown return of &ReleaseImage), detect when
coreOSMeta == nil and populate StreamMetadata with a backward-compatible default
derived from OSStreams (e.g., choose the default stream name or the first stream
via OSStreams.StreamForName/default accessor) so existing consumers that read
ReleaseImage.StreamMetadata continue to work; alternatively update those
consumers to call ReleaseImage.StreamForName(...) in the same change but do not
leave StreamMetadata nil.

---

Outside diff comments:
In `@hypershift-operator/controllers/nodepool/azure_test.go`:
- Around line 892-950: The tests in azure_test.go expecting the "no marketplace
metadata" path are now incorrect because createMockReleaseImage(..., true)
populates RHELCoreOSExtensions.Marketplace, so defaultAzureNodePoolImage will
perform defaulting and populate AzureMarketplace; update the test fixtures to
either call createMockReleaseImage(..., false) or adjust the expected results to
reflect that AzureMarketplace will be populated (i.e., set expectedImageType to
hyperv1.AzureMarketplace and set expectedMarketplaceImage to the expected
populated AzureMarketplace struct), and update the three test cases referencing
createMockReleaseImage(..., true) accordingly.
🪄 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: 29e768de-3c20-4f7f-977d-1ff55dbd2894

📥 Commits

Reviewing files that changed from the base of the PR and between 3121c68 and 6eadd8e.

⛔ Files ignored due to path filters (7)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/coreos/stream-metadata-go/LICENSE is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/coreos/stream-metadata-go/stream/stream.go is excluded by !vendor/**, !**/vendor/**
  • vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.go is excluded by !vendor/**, !**/vendor/**
  • vendor/modules.txt is excluded by !vendor/**, !**/vendor/**
📒 Files selected for processing (27)
  • go.mod
  • hypershift-operator/controllers/nodepool/aws.go
  • hypershift-operator/controllers/nodepool/aws_test.go
  • hypershift-operator/controllers/nodepool/azure.go
  • hypershift-operator/controllers/nodepool/azure_test.go
  • hypershift-operator/controllers/nodepool/gcp_test.go
  • hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
  • hypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.go
  • hypershift-operator/controllers/nodepool/nodepool_controller.go
  • hypershift-operator/controllers/nodepool/openstack/openstack.go
  • hypershift-operator/controllers/nodepool/openstack/openstack_test.go
  • hypershift-operator/controllers/nodepool/powervs.go
  • hypershift-operator/controllers/nodepool/stream.go
  • hypershift-operator/controllers/nodepool/stream_test.go
  • hypershift-operator/controllers/nodepool/token_test.go
  • support/releaseinfo/deserialize.go
  • support/releaseinfo/deserialize_test.go
  • support/releaseinfo/fake/fake.go
  • support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
  • support/releaseinfo/fixtures/fixtures.go
  • support/releaseinfo/registry_image_content_policies_test.go
  • support/releaseinfo/registry_mirror_provider.go
  • support/releaseinfo/registryclient_provider.go
  • support/releaseinfo/releaseinfo.go
  • support/releaseinfo/releaseinfo_test.go
  • support/releaseinfo/testutils/testutils.go
  • test/integration/osstreams/osstreams_test.go
✅ Files skipped from review due to trivial changes (1)
  • go.mod
🚧 Files skipped from review as they are similar to previous changes (9)
  • support/releaseinfo/registry_mirror_provider.go
  • support/releaseinfo/releaseinfo_test.go
  • support/releaseinfo/deserialize.go
  • hypershift-operator/controllers/nodepool/stream_test.go
  • support/releaseinfo/fixtures/fixtures.go
  • hypershift-operator/controllers/nodepool/stream.go
  • test/integration/osstreams/osstreams_test.go
  • support/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yaml
  • support/releaseinfo/deserialize_test.go

Comment thread hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go
Comment thread hypershift-operator/controllers/nodepool/openstack/openstack.go
Comment thread support/releaseinfo/registryclient_provider.go

if explicitStream != "" {
switch explicitStream {
case "rhel-9":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: can we make rhel-9/10 constants and use that all around for code/tests

@enxebre

enxebre commented Jun 8, 2026

Copy link
Copy Markdown
Member

this looks great to me overall, thanks!

jparrill and others added 2 commits June 11, 2026 17:51
…m resolution

Extend DeserializeImageMetadata to parse the multi-stream "streams"
key alongside the legacy "stream" key, returning both default and
per-stream metadata. Add OSStreams field and StreamForName() method
to ReleaseImage for named stream lookup. Add GetRHELStream() to
resolve which RHEL stream a NodePool should use based on explicit
selection, release version, and runc usage.

Propagate OSStreams through RegistryMirrorProviderDecorator to
prevent silent data loss on mirror-configured clusters.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
Integration tests verifying the full stream resolution pipeline:
GetRHELStream -> StreamForName -> platform boot image metadata
access. Covers multi-stream 5.0 payloads (rhel-9/rhel-10 across
AWS, GCP, arm64, KubeVirt), runc fallback, error paths, and
legacy 4.x backward compatibility.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
@jparrill

Copy link
Copy Markdown
Contributor Author

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/platform/aws PR/issue for AWS (AWSPlatform) platform area/platform/azure PR/issue for Azure (AzurePlatform) platform area/platform/gcp PR/issue for GCP (GCPPlatform) platform area/platform/kubevirt PR/issue for KubeVirt (KubevirtPlatform) platform area/platform/openstack PR/issue for OpenStack (OpenStackPlatform) platform area/platform/powervs PR/issue for PowerVS (PowerVSPlatform) platform area/testing Indicates the PR includes changes for e2e testing do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants