CNTRLPLANE-3552: Multi-stream CoreOS metadata parsing and stream resolution#8669
CNTRLPLANE-3552: Multi-stream CoreOS metadata parsing and stream resolution#8669jparrill wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@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. 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. |
|
Skipping CI for Draft Pull Request. |
|
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 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 ( Sequence DiagramsequenceDiagram
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.)
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 |
|
[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 |
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 `@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
📒 Files selected for processing (11)
hypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.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
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
6651f3c to
3121c68
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
support/releaseinfo/deserialize_test.go (2)
20-26: ⚡ Quick winMark test helper as a helper for cleaner failure locations.
testConfigMapis a helper but does not mark itself witht.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 winRun 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 winEnable 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
📒 Files selected for processing (11)
hypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.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 (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
|
/hold We need to discuss what's the best approach for the dependent PR #8673 |
|
/hold Until dependent PR got merged. |
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 (1)
hypershift-operator/controllers/nodepool/azure_test.go (1)
892-950:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese fixtures no longer cover the “no marketplace metadata” path.
createMockReleaseImage(..., true)now buildsRHELCoreOSExtensions.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,defaultAzureNodePoolImageshould populateAzureMarketplacehere, 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
⛔ 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 (27)
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_test.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.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_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
✅ 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
|
|
||
| if explicitStream != "" { | ||
| switch explicitStream { | ||
| case "rhel-9": |
There was a problem hiding this comment.
nit: can we make rhel-9/10 constants and use that all around for code/tests
|
this looks great to me overall, thanks! |
6eadd8e to
1ae6699
Compare
…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>
1ae6699 to
728708c
Compare
|
/hold cancel |
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 legacystreamkey and the new multi-streamstreamskeyReleaseImage.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 streamGetRHELStream(explicitStream, releaseVersion, usesRunc)— resolves which RHEL stream to use based on explicit selection, release version, and runc usageRegistryMirrorProviderDecorator— propagatesOSStreamsto prevent silent data loss on mirror-configured clustersTests:
TestDeserializeImageMetadata(11 cases),TestDeserializeImageMetadataMultiStreamContent(9 cases),TestStreamForName(6 cases),TestGetRHELStream(17 cases with error message assertions)TestOSStreamsEndToEnd(8 cases),TestOSStreamsStreamForNameErrors(2 cases),TestOSStreamsLegacyPayloadBackwardCompat(2 cases)Which issue(s) this PR fixes
Fixes CNTRLPLANE-3552
Dependencies (must merge first)
Checklist
Test plan
go build ./support/releaseinfo/... ./hypershift-operator/controllers/nodepool/— compilesgo test ./support/releaseinfo/ -count=1— unit tests passgo test ./hypershift-operator/controllers/nodepool/ -run TestGetRHELStream -count=1— unit tests passgo test -tags=integration ./test/integration/osstreams/... -count=1— integration tests passmake lint-fix && make verify— 0 lint issues🤖 Generated with Claude Code