DNM: CNTRLPLANE-3612: Add dual-stream RHEL 9/10 NodePool support#8714
DNM: CNTRLPLANE-3612: Add dual-stream RHEL 9/10 NodePool support#8714hypershift-jira-solve-ci[bot] wants to merge 5 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-3612 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. |
|
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 metadata support by replacing the external Possibly related PRs
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8714 +/- ##
==========================================
+ Coverage 41.59% 41.61% +0.02%
==========================================
Files 758 759 +1
Lines 93925 94073 +148
==========================================
+ Hits 39066 39147 +81
- Misses 52113 52168 +55
- Partials 2746 2758 +12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/version_test.go (1)
184-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd test coverage for OSImageStream status field in setNodesInfoStatus tests.
The new OSImageStream status field (set at version.go:103-108) is not verified by any of the three existing test cases. The test at lines 215-251 creates a Machine with NodeInfo, but
expectedNodesInfoat line 246 only checksNodeVersions. Similarly, the other test cases don't verify the OSImageStream field.Add OSImageStream expectations to the test cases to ensure the integration path correctly populates the status field from Machine OSImage data.
🤖 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/version_test.go` around lines 184 - 304, The tests for setNodesInfoStatus don't assert the new OSImageStream status field: update each test case (the ones constructing machines and expectedNodesInfo in TestSetNodesInfoStatus) to include Machine.Status.NodeInfo.OSImage (add OSImage value on the Machine with NodeInfo where appropriate) and add the corresponding expected OSImageStream entries to the expectedNodesInfo for each case (for the "machines exist" case set the expected OSImageStream entry matching the Machine's OSImage; for the cases that should clear status set expectedNodesInfo.OSImageStreams to empty). Ensure you update the expectedNodesInfo structure used in the g.Expect comparison so setNodesInfoStatus (the function under test) is validated for OSImageStream population.
🧹 Nitpick comments (2)
hypershift-operator/controllers/nodepool/nodepool_controller.go (2)
764-786: ⚡ Quick winInclude requested stream in error message.
When
streamMetaisnil(line 773), the error message mentions the architecture but not the requested stream. For consistency with the recommendation ondefaultNodePoolAMIand to aid debugging, include the stream name when it's non-empty.📝 Proposed fix
streamMeta := releaseImage.StreamMetadataForStream(s) if streamMeta == nil { - return "", fmt.Errorf("release image stream metadata is nil, cannot determine GCP image for architecture %q", specifiedArch) + if s != "" { + return "", fmt.Errorf("release image has no metadata for stream %q, cannot determine GCP image for architecture %q", s, specifiedArch) + } + return "", fmt.Errorf("release image stream metadata is nil, cannot determine GCP image for architecture %q", specifiedArch) }🤖 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.go` around lines 764 - 786, The error returned when streamMeta is nil in defaultNodePoolGCPImage doesn't include the requested stream; update the error to include the stream (variable s) when non-empty so callers can see which stream was requested (e.g., change the fmt.Errorf call that currently reports only the architecture to also include the stream name), mirroring the behaviour in defaultNodePoolAMI and keeping the existing architecture message.
739-761: ⚡ Quick winInclude requested stream in error message.
When
streamMetaisnil(line 745), the error message doesn't mention which stream was requested. When debugging missing stream metadata (e.g., a payload lackingrhel-10images), knowing the requested stream would speed diagnosis.📝 Proposed fix
streamMeta := releaseImage.StreamMetadataForStream(s) if streamMeta == nil { - return "", fmt.Errorf("release image stream metadata is nil") + if s != "" { + return "", fmt.Errorf("release image has no metadata for stream %q", s) + } + return "", fmt.Errorf("release image stream metadata is 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/nodepool_controller.go` around lines 739 - 761, The error returned in defaultNodePoolAMI when streamMeta is nil doesn't include which stream was requested; update the error to include the requested stream value (the local variable s / the first variadic stream argument) so the message reads something like "release image stream metadata is nil for stream %q" — modify the nil-check that currently returns fmt.Errorf("release image stream metadata is nil") to include s for better diagnostics.
🤖 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/version.go`:
- Around line 103-108: The OSImageStream status is only updated when
osImageStreamFromMachines returns a non-empty string, leaving stale data
otherwise; modify the block around osImageStreamFromMachines(machines) so that
nodePool.Status.OSImageStream is always assigned: if stream != "" set
nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{Name: stream}
else clear it by assigning an empty/zero value (matching the pattern used for
NodesInfo at lines 99-101) so stale values are removed when no stream is
detected.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/version_test.go`:
- Around line 184-304: The tests for setNodesInfoStatus don't assert the new
OSImageStream status field: update each test case (the ones constructing
machines and expectedNodesInfo in TestSetNodesInfoStatus) to include
Machine.Status.NodeInfo.OSImage (add OSImage value on the Machine with NodeInfo
where appropriate) and add the corresponding expected OSImageStream entries to
the expectedNodesInfo for each case (for the "machines exist" case set the
expected OSImageStream entry matching the Machine's OSImage; for the cases that
should clear status set expectedNodesInfo.OSImageStreams to empty). Ensure you
update the expectedNodesInfo structure used in the g.Expect comparison so
setNodesInfoStatus (the function under test) is validated for OSImageStream
population.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 764-786: The error returned when streamMeta is nil in
defaultNodePoolGCPImage doesn't include the requested stream; update the error
to include the stream (variable s) when non-empty so callers can see which
stream was requested (e.g., change the fmt.Errorf call that currently reports
only the architecture to also include the stream name), mirroring the behaviour
in defaultNodePoolAMI and keeping the existing architecture message.
- Around line 739-761: The error returned in defaultNodePoolAMI when streamMeta
is nil doesn't include which stream was requested; update the error to include
the requested stream value (the local variable s / the first variadic stream
argument) so the message reads something like "release image stream metadata is
nil for stream %q" — modify the nil-check that currently returns
fmt.Errorf("release image stream metadata is nil") to include s for better
diagnostics.
🪄 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: 56af0f53-9b8b-4ec2-8d7a-2a7a7c424a1a
📒 Files selected for processing (20)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/rhel_stream.gohypershift-operator/controllers/nodepool/rhel_stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.go
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
enxebre
changed the title
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3612
branch
from
6c3e7d1 to
8a6804f
Compare
| // discover constituent component image information. | ||
| type ReleaseImage struct { | ||
| *imageapi.ImageStream `json:",inline"` | ||
| StreamMetadata *CoreOSStreamMetadata `json:"streamMetadata"` |
There was a problem hiding this comment.
can we instead use the upstream types for github.com/coreos/stream-metadata-go
There was a problem hiding this comment.
Good suggestion. I checked the upstream github.com/coreos/stream-metadata-go/stream types and they cover most of what we need: Stream ≈ CoreOSStreamMetadata, plus AwsImage, GcpImage, ContainerImage (KubeVirt), ReplicatedObject (PowerVS/IBMCloud) etc.
However, our custom types include Azure-specific extensions (CoreRHCOSImage, CoreAzureDisk, CoreMarketplace, CoreAzureMarketplaceImage) that don't exist in the upstream package — Azure marketplace data is accessed via rhcos.Extensions which has a different structure than what we parse today. Adopting upstream types would require:
- Adding the module as a dependency
- Updating all ~15 consumers to use upstream type names and field accessors
- Reworking the Azure marketplace path through
rhcos.Extensions - Verifying JSON tag compatibility (some differ:
sha256vsSHA256, etc.)
I'd prefer to do this as a follow-up PR to keep this one focused on the dual-stream plumbing. I'll create a JIRA for it. Sound reasonable?
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good point. The upstream github.com/coreos/stream-metadata-go package provides types like stream.Stream and stream.Arch that map to our custom CoreOSStreamMetadata and CoreOSArchitecture.
However, adopting the upstream types requires careful evaluation:
- The upstream types may include fields we don't need, increasing API surface
- Our types have custom JSON tags and HyperShift-specific nested structs (e.g.,
CoreOSGCPImage,CoreAzureMarketplace) that don't exist upstream - The upstream types would introduce a new dependency that needs to be tracked for compatibility
I'd propose handling this as a follow-up PR to keep this one focused on the dual-stream logic. We can evaluate which upstream types map cleanly and adopt them incrementally. Would that work for you?
AI-assisted response via Claude Code
| // TestReleaseInfoKubeVirt tests validates the presence of the kubevirt images | ||
| func TestReleaseInfoKubeVirt(t *testing.T) { | ||
| metadata, err := DeserializeImageMetadata(fixtures.CoreOSBootImagesYAML_4_10) | ||
| metadata, _, err := DeserializeImageMetadata(fixtures.CoreOSBootImagesYAML_4_10) |
There was a problem hiding this comment.
can we please have unit tests with a stub for dualstream
There was a problem hiding this comment.
We already have dual-stream unit tests using a 5.0-dual-stream-bootimages.yaml fixture (embedded via fixtures.CoreOSDualStreamBootImagesYAML_5_0):
TestDeserializeDualStreamImageMetadata— verifies legacy single-stream parsing, multi-stream parsing (rhel-9/rhel-10), correct stream names, and per-stream AWS AMI differentiationTestStreamMetadataForStream— verifies stream lookup for empty, known (rhel-10), and unknown stream names with correct fallback behavior
These are in support/releaseinfo/releaseinfo_test.go (lines 165–262). Would you like additional coverage for specific scenarios beyond what these already test?
AI-assisted response via Claude Code
There was a problem hiding this comment.
The dual-stream deserialization is already covered by tests in this file:
TestDeserializeDualStreamImageMetadata(line 165): parses the dual-stream fixture, validates bothrhel-9andrhel-10streams exist, verifies stream names, and checks per-stream AWS AMIs are differentTestStreamMetadataForStream(line 229): validatesStreamMetadataForStream()returns the correct metadata for empty, known, and unknown stream names
For the TestReleaseInfoKubeVirt test at this line, the change is minimal — it only updates the call to DeserializeImageMetadata to accept the new 3-value return (adding _ for streamsMeta). The test still validates KubeVirt images from the 4.10 single-stream fixture.
Would you like additional tests specifically for KubeVirt/PowerVS with dual-stream data? The current dual-stream fixture (CoreOSDualStreamBootImagesYAML_5_0) does include per-stream data for AWS/GCP, but we could extend it to include KubeVirt and PowerVS entries as well.
AI-assisted response via Claude Code
| } | ||
| // Default behavior for Linux/RHCOS AMIs | ||
| ami, err := defaultNodePoolAMI(region, arch, releaseImage) | ||
| ami, err := defaultNodePoolAMI(region, arch, releaseImage, nodePool.Spec.OSImageStream.Name) |
There was a problem hiding this comment.
Is there other platforms like azure that need to update how they pick default?
There was a problem hiding this comment.
Yes, good catch. Currently only AWS's defaultNodePoolAMI and defaultNodePoolGCPImage accept the stream parameter and use StreamMetadataForStream(). The following platforms still access releaseImage.StreamMetadata directly and would need updating:
- Azure (
azure.go:124) —getAzureMarketplaceMetadatausesreleaseImage.StreamMetadata - GCP (
gcp.go:171) —resolveGCPImagecallsdefaultNodePoolGCPImagewithout passing the stream name - KubeVirt (
kubevirt/kubevirt.go:47) — usesreleaseImage.StreamMetadatadirectly - PowerVS (
powervs.go:112) — usesreleaseImage.StreamMetadatadirectly - OpenStack (
openstack/openstack.go:135,161) — usesreleaseImage.StreamMetadatadirectly
For pre-5.0 releases this is fine (single stream = legacy StreamMetadata). For 5.0+ where StreamMetadata falls back to the first stream in the map, these platforms will still get a valid image — but they won't respect spec.osImageStream.name. I'll update them to use StreamMetadataForStream() consistently.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Yes, good catch. Currently the following platforms need updating for dual-stream support:
- GCP (
gcp.go:171):defaultNodePoolGCPImagealready accepts astream ...stringvariadic, butresolveGCPImagedoesn't passnodePool.Spec.OSImageStream.Nameyet - Azure (
azure.go:39):defaultAzureNodePoolImageaccessesreleaseImage.StreamMetadatadirectly without going throughStreamMetadataForStream() - PowerVS (
powervs.go:112):getPowerVSImageaccessesreleaseImage.StreamMetadata.Architectures["ppc64le"]directly — though ppc64le may not have RHEL 10 images in the near term - KubeVirt (
kubevirt.go:47):defaultImageaccessesreleaseImage.StreamMetadatadirectly
For this initial PR, I focused on AWS as the primary platform for RHEL 10 dual-stream. GCP and Azure should be updated to use StreamMetadataForStream() — I can add that in a follow-up or extend this PR if you'd prefer. PowerVS and KubeVirt are lower priority since ppc64le/s390x may not have RHEL 10 images initially.
AI-assisted response via Claude Code
| return &ctrl.Result{}, fmt.Errorf("invalid RHEL stream configuration: %w", streamErr) | ||
| } | ||
| // Informational message when stream falls back to rhel-9 due to runc. | ||
| if nodePool.Spec.OSImageStream.Name == "" && stream == rhelStreamRHEL9 { |
There was a problem hiding this comment.
how do we know it was due to runc here? couldn't it be the payload is < 5?
There was a problem hiding this comment.
Good question. When specStream == "", getRHELStream() returns exactly these values:
| Release | usesRunc | Returns |
|---|---|---|
| < 5.0 | false | "" (empty — legacy) |
| < 5.0 | true | "" (empty — legacy) |
| >= 5.0 | false | "rhel-10" (default) |
| >= 5.0 | true | "rhel-9" (runc fallback) |
So the condition specStream == "" && stream == rhelStreamRHEL9 can only be true when rv >= 5.0 && usesRunc — pre-5.0 payloads return empty string, not "rhel-9". The runc attribution in the message is correct.
That said, the code doesn't make this obvious at the call site. Would it be clearer to also check configGenerator.usesRunc explicitly in the condition?
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good question. Looking at getRHELStream() in rhel_stream.go, when specStream == "" (no explicit stream set), the possible return values are:
rv >= 5.0 && usesRunc→ returns"rhel-9"(this is the only path that returns"rhel-9"with empty specStream)rv >= 5.0 && !usesRunc→ returns"rhel-10"rv < 5.0→ returns""(legacy single-stream)
So the condition nodePool.Spec.OSImageStream.Name == "" && stream == rhelStreamRHEL9 can only be true via case 1 (runc fallback). Pre-5.0 payloads return "", not "rhel-9".
That said, the message could be clearer. I could add a comment explaining this reasoning, or make the condition more explicit by also checking configGenerator.usesRunc directly. Would that help?
AI-assisted response via Claude Code
| cg.rolloutConfig.additionalTrustBundleName = hostedCluster.Spec.AdditionalTrustBundle.Name | ||
| } | ||
|
|
||
| // rhelStream is populated directly from spec.osImageStream.name (empty when unset). |
There was a problem hiding this comment.
should we error here if userunC and rhelStream=10?
There was a problem hiding this comment.
This is already handled downstream. In conditions.go:394, after the config is generated, we call:
```go
stream, streamErr := getRHELStream(nodePool.Spec.OSImageStream.Name, releaseVersion, configGenerator.usesRunc)
```
getRHELStream returns an error for rhelStream=rhel-10 + usesRunc:
```go
case specStream == rhelStreamRHEL10 && usesRunc:
return "", fmt.Errorf("OS stream %s is incompatible with default_runtime=runc; RHEL 10 does not ship runc", rhelStreamRHEL10)
```
The config generator itself populates `rhelStream` from the raw spec value at line 104 — it's just storing the user's input. The validation happens in validMachineConfigCondition which runs getRHELStream() to catch the incompatible combination and sets a clear condition message. So the validation is there, just in a different layer (conditions vs config).
AI-assisted response via Claude Code
There was a problem hiding this comment.
This is already handled by getRHELStream() in rhel_stream.go:36-37:
case specStream == rhelStreamRHEL10 && usesRunc:
return "", fmt.Errorf("OS stream %s is incompatible with default_runtime=runc; RHEL 10 does not ship runc", rhelStreamRHEL10)The validation happens in validMachineConfigCondition() (conditions.go:394) which calls getRHELStream() and sets NodePoolValidMachineConfigCondition to False when this error is returned. So the error is surfaced as a condition before we ever reach NewConfigGenerator.
The code at this line (config.go:101) runs during NewConfigGenerator, which is called before the getRHELStream() validation in conditions.go. However, cg.rhelStream here is only used for the hash — the actual incompatibility validation is handled separately. The usesRunc flag is set later during generateMCORawConfig (line 106) when parsing ContainerRuntimeConfig manifests.
AI-assisted response via Claude Code
| // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. | ||
| func (cg *ConfigGenerator) Hash() string { | ||
| return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.globalConfig) | ||
| return supportutil.HashSimple(cg.mcoRawConfig + cg.releaseImage.Version() + cg.pullSecretName + cg.additionalTrustBundleName + cg.globalConfig + cg.rhelStream) |
There was a problem hiding this comment.
do we have coverage to validate this doesn't change the current hash when rhelStream is empty? otherwise that would trigger a rollout
There was a problem hiding this comment.
When `rhelStream` is empty (the default), the hash input is:
```
mcoRawConfig + version + pullSecretName + additionalTrustBundleName + globalConfig + ""
```
Since `"abc" + ""` == `"abc"` in Go string concatenation, appending an empty string produces the same hash as before. No rollout is triggered.
That said, an explicit test proving this invariant would be valuable — I'll add a test case that computes the hash with `rhelStream=""` and verifies it matches the hash computed without the `rhelStream` field present at all.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Yes — there's explicit test coverage for this in config_test.go:
TestHash (line 450):
{
name: "When rhelStream is empty it should not change the hash",
...
rhelStream: "",
expected: baseCaseHash, // identical to base case without rhelStream field
},This test verifies that an empty rhelStream (the default when spec.osImageStream is unset) produces the exact same hash as the base case. Since rhelStream defaults to "" and string concatenation with "" is a no-op, the hash input is identical to pre-PR behavior.
AI-assisted response via Claude Code
| // This is kept like this for now to contain the scope of the refactor and avoid backward compatibility issues. | ||
| func (cg *ConfigGenerator) HashWithoutVersion() string { | ||
| return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName) | ||
| return supportutil.HashSimple(cg.mcoRawConfig + cg.pullSecretName + cg.additionalTrustBundleName + cg.rhelStream) |
There was a problem hiding this comment.
do we have coverage to validate this doesn't change the current hash when rhelStream is empty? otherwise that would trigger a rollout
There was a problem hiding this comment.
Same answer as above — when rhelStream is empty (the default/unset case), appending "" to the hash input string is a no-op: "abc" + "" == "abc". The hash is identical to the old code. I'll make the test cover both Hash() and HashWithoutVersion() to validate this invariant explicitly.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Same as above — TestHashWithoutVersion (line 576) has the same coverage:
{
name: "When rhelStream is empty it should not change the hash",
...
rhelStream: "",
expected: baseCaseHash, // "85234650" — identical to base case
},Both Hash() and HashWithoutVersion() are covered. Empty rhelStream → no hash change → no rollout.
AI-assisted response via Claude Code
| } | ||
| case *mcfgv1.ContainerRuntimeConfig: | ||
| // Detect runc usage for dual-stream RHEL compatibility | ||
| if obj.Spec.ContainerRuntimeConfig != nil && |
There was a problem hiding this comment.
is this how the MCO validates this? please let this be a unit tested func
There was a problem hiding this comment.
Done. The runc detection logic has been extracted into containerRuntimeUsesRunc() in rhel_stream.go and unit tested in rhel_stream_test.go (TestContainerRuntimeUsesRunc) with coverage for runc, crun, nil config, and empty runtime cases. The MCO uses ContainerRuntimeConfig.Spec.ContainerRuntimeConfig.DefaultRuntime to determine the runtime — our check mirrors that.
AI-assisted response via Claude Code
Code Review: PR #8714 — Dual-stream RHEL 9/10 NodePool SupportReviewer: AI-assisted review (Claude Code with HyperShift SME agents) Overall Verdict: FAIL (3 blocking issues)The end-to-end architecture (spec → controller → token → ignition → MCC) is sound, and the hash stability design (explicit stream only in hash to avoid fleet-wide rollouts on 5.0 upgrade) is well-considered. Pure functions ( Required Actions (Blocking)1. Build failure —
|
| Function | File | Gap |
|---|---|---|
resolveStreamMetadata() |
nodepool_controller.go |
Zero tests — core dispatch logic for multi-stream |
DeserializeMultiStreamImageMetadata() |
deserialize.go |
Zero tests — parses new multi-stream ConfigMap format |
usesRunc detection |
config.go |
No test with defaultRuntime=runc verifying cg.usesRunc=true |
defaultNodePoolAMI with stream |
nodepool_controller.go |
Variadic stream param never exercised |
rhelStream in Hash |
config.go |
No test verifying hash changes when rhelStream changes |
os-stream in token secret |
token.go |
No test verifying the key is written |
Documentation
13. No forced migration path for existing RHEL 9 nodes — The implicit default switch from RHEL 9 to RHEL 10 on 5.0+ happens at the ignition payload level without changing the rollout hash. Existing nodes stay on RHEL 9; only new nodes get RHEL 10. This is likely intentional but should be documented explicitly in the enhancement.
Platform Coverage Summary
| Platform | Stream-aware? | Status |
|---|---|---|
| AWS | ✅ | Correct via resolveStreamMetadata() |
| GCP | ✅ | Correct via resolveStreamMetadata() |
| Azure | Inline resolution — should use shared helper | |
| KubeVirt | Inline resolution — should use shared helper | |
| PowerVS | ❌ | Nil-safety risk (blocking) |
| OpenStack | ❌ | Nil-safety risk |
| Karpenter | ❌ | Stream argument not passed |
What's Good
- Hash stability design — Using the raw spec value (not resolved default) for the rollout hash avoids fleet-wide rollouts when upgrading to 5.0. Well-commented.
getRHELStream()is clean — Pure function with clear switch-case logic and comprehensive tests (10 cases).- Token secret flow — The
os-streamkey absence for pre-5.0 is handled correctly (nil → empty string → no manifest). - OSImageStream CR timing — Written after MCO but before MCC, which is the correct placement in the ignition pipeline.
usesRuncdetection — Correctly re-initialized per reconcile via freshConfigGeneratorconstruction.
Generated with Claude Code using HyperShift SME agents (control-plane, data-plane, cloud-provider, API, architect)
|
|
||
| // rhelStream is populated directly from spec.osImageStream.name (empty when unset). | ||
| // Using the resolved value from getRHELStream() would inject implicit defaults into | ||
| // the hash, triggering fleet-wide rollouts on upgrade. |
There was a problem hiding this comment.
I think we should only let this change the hash and trigger a rollout if the value is different from the one in status
There was a problem hiding this comment.
Interesting suggestion. Let me walk through the current behavior and the tradeoff:
Current design: rhelStream in the hash comes directly from spec.osImageStream.name (empty when unset). This means:
- Empty → empty (upgrade to 5.0, no explicit stream set): hash unchanged, no rollout ✅
- Empty → "rhel-10" (user explicitly sets stream): hash changes, rollout triggered ✅
- "rhel-9" → "rhel-10" (user changes stream): hash changes, rollout triggered ✅
Your suggestion (compare with status): If spec == status, skip from hash. This would prevent a rollout when the user explicitly sets the stream to what's already running (e.g., setting "rhel-10" on a node already running rhel-10).
The tradeoff: this would require passing nodePool.Status.OSImageStream.Name into the ConfigGenerator, adding a coupling between the config hash computation and status. Currently the config generator is deliberately a pure function of spec inputs.
Also, status.osImageStream is populated from CAPI Machine NodeInfo (detected from running nodes), not from the controller's intent. There could be a window where status lags — e.g., the user sets rhel-10 but nodes haven't been updated yet, so status still shows rhel-9.
For now, the only case that would cause a "spurious" rollout is when a user explicitly sets the stream to match what they're already running — which seems rare and arguably intentional (explicit declarative intent). Would you still like this changed, or is the current behavior acceptable?
AI-assisted response via Claude Code
There was a problem hiding this comment.
Good point. The current design uses spec.osImageStream.name directly in the hash, which means:
- Empty → empty string → hash unchanged → no rollout ✅
- User sets "rhel-10" → hash changes → rollout ✅ (intended)
- User sets "rhel-9" on a cluster already running rhel-9 → hash changes → rollout
⚠️ (unnecessary)
Your suggestion to compare spec vs status would avoid the unnecessary rollout in case 3. Implementation would look something like:
// Only include rhelStream in hash if it differs from the observed stream in status,
// preventing unnecessary rollouts when explicitly setting the current stream.
if nodePool.Spec.OSImageStream.Name != nodePool.Status.OSImageStream.Name {
cg.rolloutConfig.rhelStream = nodePool.Spec.OSImageStream.Name
}However, there's a subtle concern: this creates a dependency between the hash calculation and the status, which is normally populated asynchronously from observed machine state. If status hasn't been populated yet (e.g., new NodePool), we'd need to handle that edge case.
Would you like me to implement this approach? The tradeoff is: simpler code (current) vs. avoiding unnecessary rollouts when the user sets spec.osImageStream to match the already-running stream.
AI-assisted response via Claude Code
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
openshift-ci
Bot
added
the
area/platform/kubevirt
openshift-ci
Bot
added
area/platform/openstack
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
|
/test address-review-comments |
|
Re: #8714 (comment) Review Comments AddressedAddressed the code review from #8714 (comment). Blocking Issues (all 3 fixed)All three blocking issues were already resolved in commit
Non-blocking Items AddressedFixed in new commit
Already fixed in commit
Items Deferred as Follow-up
Build & Test Status
AI-assisted response via Claude Code |
|
rebase and create a tmp.txt file in the root folder with a literal "test". |
openshift-ci
Bot
added
the
needs-rebase
enxebre
force-pushed
the
fix-CNTRLPLANE-3612
branch
from
d912c4b to
6fee43e
Compare
openshift-ci
Bot
added
area/platform/azure
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
hypershift-operator/controllers/nodepool/aws.go (1)
434-434:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing nil-safety guard for
releaseImage.StreamMetadata.Line 434 dereferences
releaseImage.StreamMetadata.Architectureswithout checking ifStreamMetadatais nil. When a payload uses only the newstreamsmetadata format (without legacy single-stream),StreamMetadatacan be nil, causing a panic.This is the same blocking nil-safety issue the reviewer flagged for PowerVS and OpenStack.
🛡️ Add nil guard before accessing StreamMetadata
func getWindowsAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) { if releaseImage == nil { return "", fmt.Errorf("release image is nil") } if releaseImage.StreamMetadata == nil { - return "", fmt.Errorf("release image stream metadata is nil") + return "", fmt.Errorf("release image stream metadata is nil") } archData, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]🤖 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` at line 434, Guard against nil releaseImage.StreamMetadata before accessing Architectures: in the code path using releaseImage.StreamMetadata.Architectures (the line computing archData, foundArch), add a nil check on releaseImage.StreamMetadata and handle the nil case (e.g., skip this lookup, set foundArch=false or use the alternative streams metadata lookup) to avoid dereferencing a nil StreamMetadata; update any downstream logic that assumes foundArch accordingly. Ensure references include releaseImage, StreamMetadata, Architectures, archData/foundArch, hyperv1.ArchAliases and specifiedArch so the change is applied exactly where the lookup happens.hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go (1)
47-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMultiple nil-safety risks in KubeVirt image resolution.
Two nil-dereference risks:
- Line 47:
releaseImage.StreamMetadata.Architectures— ifStreamMetadatais nil (multi-stream-only payload), this panics- Line 53:
arch.Images.Kubevirt.DigestRef— ifarch.Images.Kubevirtis nil, accessing.DigestRefpanicsThese are the same blocking nil-safety issues flagged for PowerVS/OpenStack.
🛡️ Add nil guards before dereferencing
func defaultImage(nodePoolArch string, releaseImage *releaseinfo.ReleaseImage) (string, string, error) { + if releaseImage.StreamMetadata == nil { + return "", "", fmt.Errorf("release image stream metadata is nil") + } + var archName string switch nodePoolArch { case hyperv1.ArchitectureS390X: archName = hyperv1.ArchitectureS390X default: archName = hyperv1.ArchAliases[hyperv1.ArchitectureAMD64] } arch, foundArch := releaseImage.StreamMetadata.Architectures[archName] if !foundArch { return "", "", fmt.Errorf("couldn't find OS metadata for architecture %q", archName) } + if arch.Images.Kubevirt == nil { + return "", "", fmt.Errorf("no kubevirt image metadata present in release") + } if arch.Images.Kubevirt.DigestRef == "" { return "", "", fmt.Errorf("no kubevirt image metadata present in release") }🤖 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/kubevirt/kubevirt.go` around lines 47 - 56, Check for nil StreamMetadata and nil nested image structs before dereferencing: guard that releaseImage.StreamMetadata is not nil before indexing into StreamMetadata.Architectures[archName], and after finding arch, ensure arch.Images and arch.Images.Kubevirt are non-nil before reading DigestRef; if any are nil return a clear error (e.g., "no stream metadata", "no image metadata for architecture %q", or "no kubevirt image metadata present in release") so containerImage is only assigned when these checks pass.hypershift-operator/controllers/nodepool/openstack/openstack.go (1)
134-135:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing nil-safety guard for
releaseImage.StreamMetadata(blocking issue).Line 135 dereferences
releaseImage.StreamMetadata.Architectureswithout checking ifStreamMetadatais nil. When a payload uses only the newstreamsmetadata format,StreamMetadatacan be nil, causing a panic.This is the blocking nil-safety issue the reviewer explicitly flagged for OpenStack in the PR objectives summary.
🛡️ Add nil guard before accessing StreamMetadata
func OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage) (string, string, error) { + if releaseImage.StreamMetadata == nil { + return "", "", fmt.Errorf("release image stream metadata is nil") + } + arch, foundArch := releaseImage.StreamMetadata.Architectures["x86_64"] if !foundArch {🤖 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 134 - 135, OpenstackDefaultImage currently dereferences releaseImage.StreamMetadata without a nil check; add a guard at the start of OpenstackDefaultImage to check if releaseImage == nil or releaseImage.StreamMetadata == nil before accessing StreamMetadata.Architectures, and if nil return a clear error (or fallback to the alternative streams metadata if available) so the function never panics when StreamMetadata is absent; reference the symbols OpenstackDefaultImage, releaseImage, StreamMetadata, and Architectures when applying the check and returning the error/fallback.hypershift-operator/controllers/nodepool/azure.go (1)
124-139:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMultiple nil-safety risks in marketplace metadata extraction.
Lines 128 and 136-137 dereference nested structs without nil-guarding intermediate fields:
- Line 128:
releaseImage.StreamMetadata.Architectures— ifStreamMetadatais nil (multi-stream-only payload), this panics- Line 136:
archData.RHCOS.Marketplace.Azure.NoPurchasePlan— ifRHCOS.MarketplaceorMarketplace.Azureis nil, this panicsThese are the same blocking nil-safety issues flagged for PowerVS/OpenStack.
🛡️ Add nil guards for nested struct access
func getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string) (*azureMarketplaceMetadata, error) { if releaseImage.StreamMetadata == nil { - return nil, nil // No stream metadata available + return nil, nil } archData, foundArch := releaseImage.StreamMetadata.Architectures[arch] if !foundArch { return nil, fmt.Errorf("architecture %s not found in stream metadata", arch) } + // Check for nil at each level of nested access + if archData.RHCOS.Marketplace == nil || archData.RHCOS.Marketplace.Azure == nil { + return nil, nil // No Azure marketplace data available + } + azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan if azureMarketplace.HyperVGen1 == nil && azureMarketplace.HyperVGen2 == 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.go` around lines 124 - 139, The code dereferences nested fields without nil checks causing panics; add explicit nil guards before accessing releaseImage.StreamMetadata and before archData.RHCOS, archData.RHCOS.Marketplace, archData.RHCOS.Marketplace.Azure and archData.RHCOS.Marketplace.Azure.NoPurchasePlan; specifically, in the function that reads releaseImage.StreamMetadata.Architectures and assigns azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan, first check if releaseImage.StreamMetadata == nil (return nil,nil), then after finding archData verify archData.RHCOS != nil && archData.RHCOS.Marketplace != nil && archData.RHCOS.Marketplace.Azure != nil && archData.RHCOS.Marketplace.Azure.NoPurchasePlan != nil (return nil,nil if any are nil) before using azureMarketplace and its HyperVGen1/Gen2 fields.
🧹 Nitpick comments (5)
hypershift-operator/controllers/nodepool/powervs.go (1)
111-131: ⚡ Quick winPowerVS doesn't support stream-aware image selection.
getPowerVSImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means PowerVS images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with AWS's stream-aware pattern and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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.go` around lines 111 - 131, The getPowerVSImage function currently reads releaseImage.StreamMetadata directly; change its signature to accept an optional stream parameter (e.g., stream ...string) and obtain the architecture metadata via releaseImage.StreamMetadataForStream(stream) instead of releaseImage.StreamMetadata so PowerVS becomes stream-aware; update the function body to use the returned StreamMetadata for the "ppc64le" lookup and keep the rest of the logic unchanged, and then update all call sites of getPowerVSImage to pass the stream when available.hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go (1)
39-67: ⚡ Quick winKubeVirt doesn't support stream-aware image selection.
defaultImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means KubeVirt images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with AWS and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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/kubevirt/kubevirt.go` around lines 39 - 67, The defaultImage function currently reads releaseImage.StreamMetadata directly, which prevents honoring spec.osImageStream; modify func defaultImage(nodePoolArch string, releaseImage *releaseinfo.ReleaseImage, stream ...string) (string, string, error) to accept an optional stream parameter, call releaseImage.StreamMetadataForStream(stream) to obtain the appropriate StreamMetadata (falling back when stream is empty), then use the returned metadata for the architecture lookup (arch, foundArch := streamMetadata.Architectures[archName]) and keep the existing validation and return values unchanged; ensure callers of defaultImage are updated to pass the optional stream where stream-aware selection is required.hypershift-operator/controllers/nodepool/openstack/openstack.go (1)
132-153: ⚡ Quick winOpenStack doesn't support stream-aware image selection.
OpenstackDefaultImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means OpenStack images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support, unlike AWS which correctly uses stream-aware selection.For consistency with AWS and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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 132 - 153, OpenstackDefaultImage currently reads releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change the function signature to accept an optional variadic stream parameter (e.g., OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage, stream ...string)) and use releaseImage.StreamMetadataForStream(stream) to obtain the stream-aware metadata before looking up Architectures["x86_64"], Artifacts["openstack"], Formats["qcow2.gz"] and the disk; ensure the function otherwise returns the same (disk.Location, disk.SHA256, error) and preserves existing error messages when lookups fail.hypershift-operator/controllers/nodepool/aws.go (1)
424-453: ⚡ Quick winWindows AMI path doesn't support stream-aware image selection.
getWindowsAMIaccessesreleaseImage.StreamMetadatadirectly and doesn't accept a stream parameter, unlike the Linux/RHCOS path which correctly passesnodePool.Spec.OSImageStream.NametodefaultNodePoolAMI(lines 137, 364). This means Windows AMIs can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with the Linux path and to properly support multi-stream payloads, consider:
- Adding an optional
stream ...stringparameter togetWindowsAMI- Using
releaseImage.StreamMetadataForStream(stream)instead of directStreamMetadataaccess- Passing
nodePool.Spec.OSImageStream.Namefrom callers (lines 343, 130)🤖 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 424 - 453, getWindowsAMI currently reads releaseImage.StreamMetadata directly so it ignores spec.osImageStream and cannot select stream-aware Windows AMIs; change getWindowsAMI to accept an optional stream parameter (e.g., stream ...string) and use releaseImage.StreamMetadataForStream(stream) instead of releaseImage.StreamMetadata, update callers (notably defaultNodePoolAMI and places that resolve AMIs) to pass nodePool.Spec.OSImageStream.Name when present so Windows AMI lookup mirrors the Linux/RHCOS stream-aware behavior.hypershift-operator/controllers/nodepool/azure.go (1)
122-167: ⚡ Quick winAzure marketplace doesn't support stream-aware image selection.
getAzureMarketplaceMetadataaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter, unlike AWS'sdefaultNodePoolAMIwhich correctly usesStreamMetadataForStream(). This means Azure can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency and to properly support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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.go` around lines 122 - 167, getAzureMarketplaceMetadata currently reads releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change its signature to accept an optional stream parameter (e.g., getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string, stream ...string)) and use releaseImage.StreamMetadataForStream(stream) (falling back to existing StreamMetadata when no stream passed) instead of direct access to releaseImage.StreamMetadata; update internal references (archData lookup, azureMarketplace extraction) to use the returned stream metadata so Azure respects Stream-aware image selection consistent with defaultNodePoolAMI and supports dual-stream RHEL 9/10 scenarios.
🤖 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_test.go`:
- Around line 716-733: The test case named "When RHELCoreOSExtensions is nil, it
should return error" has an expectedError string that doesn't match the actual
error returned by getWindowsAMI; update the test's expectedError to match the
production error path (e.g., the "no aws-winli regions data found..." message)
or change the fixture to trigger the rhel-coreos-extensions error if that's the
intended behavior; locate the test struct in aws_test.go (the case with name
matching the above), and set expectedError to the exact error returned by
getWindowsAMI for that input shape (or adjust the releaseImage/StreamMetadata so
getWindowsAMI hits the rhel-coreos-extensions branch).
In `@hypershift-operator/controllers/nodepool/powervs_test.go`:
- Around line 11-52: Add a test case to TestGetPowerVSImage that covers
releaseImage == nil and releaseImage.StreamMetadata == nil to prevent the
nil-dereference panic, and update getPowerVSImage to defensively check for nil
releaseImage and nil StreamMetadata at the top (returning a clear error like
"release image metadata is nil" or similar) before accessing
StreamMetadata.Architectures; reference getPowerVSImage in your patch and ensure
the new tests assert an error containing the new message.
In `@hypershift-operator/controllers/nodepool/powervs.go`:
- Around line 117-119: Add a nil-check for releaseImage.StreamMetadata before
dereferencing it: ensure releaseImage.StreamMetadata != nil before accessing
releaseImage.StreamMetadata.Architectures (the code that constructs arch and
later checks arch.Images.PowerVS.Regions). If StreamMetadata is nil, return an
appropriate error (similar to the existing error path) so the function does not
panic; reference releaseImage, StreamMetadata, Architectures, arch, Images,
PowerVS, and Regions when locating where to insert this guard.
In `@ignition-server/cmd/run_local_ignitionprovider.go`:
- Line 114: The call to GetPayload is ignoring the CLI --feature-gate-manifest
because the fifth argument is a literal empty string; update the GetPayload call
so it passes the stored option o.FeatureGateManifest (the same value previously
set on the provider) instead of the empty string. Locate the call to
p.GetPayload(...) and replace the 5th parameter with o.FeatureGateManifest so
the feature-gate manifest flag is forwarded to GetPayload.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/aws.go`:
- Line 434: Guard against nil releaseImage.StreamMetadata before accessing
Architectures: in the code path using releaseImage.StreamMetadata.Architectures
(the line computing archData, foundArch), add a nil check on
releaseImage.StreamMetadata and handle the nil case (e.g., skip this lookup, set
foundArch=false or use the alternative streams metadata lookup) to avoid
dereferencing a nil StreamMetadata; update any downstream logic that assumes
foundArch accordingly. Ensure references include releaseImage, StreamMetadata,
Architectures, archData/foundArch, hyperv1.ArchAliases and specifiedArch so the
change is applied exactly where the lookup happens.
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 124-139: The code dereferences nested fields without nil checks
causing panics; add explicit nil guards before accessing
releaseImage.StreamMetadata and before archData.RHCOS,
archData.RHCOS.Marketplace, archData.RHCOS.Marketplace.Azure and
archData.RHCOS.Marketplace.Azure.NoPurchasePlan; specifically, in the function
that reads releaseImage.StreamMetadata.Architectures and assigns
azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan, first check
if releaseImage.StreamMetadata == nil (return nil,nil), then after finding
archData verify archData.RHCOS != nil && archData.RHCOS.Marketplace != nil &&
archData.RHCOS.Marketplace.Azure != nil &&
archData.RHCOS.Marketplace.Azure.NoPurchasePlan != nil (return nil,nil if any
are nil) before using azureMarketplace and its HyperVGen1/Gen2 fields.
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go`:
- Around line 47-56: Check for nil StreamMetadata and nil nested image structs
before dereferencing: guard that releaseImage.StreamMetadata is not nil before
indexing into StreamMetadata.Architectures[archName], and after finding arch,
ensure arch.Images and arch.Images.Kubevirt are non-nil before reading
DigestRef; if any are nil return a clear error (e.g., "no stream metadata", "no
image metadata for architecture %q", or "no kubevirt image metadata present in
release") so containerImage is only assigned when these checks pass.
In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 134-135: OpenstackDefaultImage currently dereferences
releaseImage.StreamMetadata without a nil check; add a guard at the start of
OpenstackDefaultImage to check if releaseImage == nil or
releaseImage.StreamMetadata == nil before accessing
StreamMetadata.Architectures, and if nil return a clear error (or fallback to
the alternative streams metadata if available) so the function never panics when
StreamMetadata is absent; reference the symbols OpenstackDefaultImage,
releaseImage, StreamMetadata, and Architectures when applying the check and
returning the error/fallback.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/aws.go`:
- Around line 424-453: getWindowsAMI currently reads releaseImage.StreamMetadata
directly so it ignores spec.osImageStream and cannot select stream-aware Windows
AMIs; change getWindowsAMI to accept an optional stream parameter (e.g., stream
...string) and use releaseImage.StreamMetadataForStream(stream) instead of
releaseImage.StreamMetadata, update callers (notably defaultNodePoolAMI and
places that resolve AMIs) to pass nodePool.Spec.OSImageStream.Name when present
so Windows AMI lookup mirrors the Linux/RHCOS stream-aware behavior.
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 122-167: getAzureMarketplaceMetadata currently reads
releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change
its signature to accept an optional stream parameter (e.g.,
getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string,
stream ...string)) and use releaseImage.StreamMetadataForStream(stream) (falling
back to existing StreamMetadata when no stream passed) instead of direct access
to releaseImage.StreamMetadata; update internal references (archData lookup,
azureMarketplace extraction) to use the returned stream metadata so Azure
respects Stream-aware image selection consistent with defaultNodePoolAMI and
supports dual-stream RHEL 9/10 scenarios.
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go`:
- Around line 39-67: The defaultImage function currently reads
releaseImage.StreamMetadata directly, which prevents honoring
spec.osImageStream; modify func defaultImage(nodePoolArch string, releaseImage
*releaseinfo.ReleaseImage, stream ...string) (string, string, error) to accept
an optional stream parameter, call releaseImage.StreamMetadataForStream(stream)
to obtain the appropriate StreamMetadata (falling back when stream is empty),
then use the returned metadata for the architecture lookup (arch, foundArch :=
streamMetadata.Architectures[archName]) and keep the existing validation and
return values unchanged; ensure callers of defaultImage are updated to pass the
optional stream where stream-aware selection is required.
In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 132-153: OpenstackDefaultImage currently reads
releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change
the function signature to accept an optional variadic stream parameter (e.g.,
OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage, stream ...string))
and use releaseImage.StreamMetadataForStream(stream) to obtain the stream-aware
metadata before looking up Architectures["x86_64"], Artifacts["openstack"],
Formats["qcow2.gz"] and the disk; ensure the function otherwise returns the same
(disk.Location, disk.SHA256, error) and preserves existing error messages when
lookups fail.
In `@hypershift-operator/controllers/nodepool/powervs.go`:
- Around line 111-131: The getPowerVSImage function currently reads
releaseImage.StreamMetadata directly; change its signature to accept an optional
stream parameter (e.g., stream ...string) and obtain the architecture metadata
via releaseImage.StreamMetadataForStream(stream) instead of
releaseImage.StreamMetadata so PowerVS becomes stream-aware; update the function
body to use the returned StreamMetadata for the "ppc64le" lookup and keep the
rest of the logic unchanged, and then update all call sites of getPowerVSImage
to pass the stream when available.
🪄 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: c710fcdb-cc61-470d-bd2a-c5e6b808e139
📒 Files selected for processing (30)
hypershift-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/conditions.gohypershift-operator/controllers/nodepool/config.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/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/rhel_stream.gohypershift-operator/controllers/nodepool/rhel_stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.go
✅ Files skipped from review due to trivial changes (1)
- support/releaseinfo/registry_mirror_provider.go
🚧 Files skipped from review as they are similar to previous changes (13)
- support/releaseinfo/registryclient_provider.go
- ignition-server/controllers/tokensecret_controller.go
- hypershift-operator/controllers/nodepool/version_test.go
- hypershift-operator/controllers/nodepool/rhel_stream_test.go
- ignition-server/controllers/tokensecret_controller_test.go
- support/releaseinfo/deserialize_test.go
- hypershift-operator/controllers/nodepool/token.go
- hypershift-operator/controllers/nodepool/rhel_stream.go
- hypershift-operator/controllers/nodepool/conditions.go
- hypershift-operator/controllers/nodepool/nodepool_controller.go
- ignition-server/controllers/local_ignitionprovider.go
- support/releaseinfo/deserialize.go
- support/releaseinfo/releaseinfo_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
hypershift-operator/controllers/nodepool/aws.go (1)
434-434:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing nil-safety guard for
releaseImage.StreamMetadata.Line 434 dereferences
releaseImage.StreamMetadata.Architectureswithout checking ifStreamMetadatais nil. When a payload uses only the newstreamsmetadata format (without legacy single-stream),StreamMetadatacan be nil, causing a panic.This is the same blocking nil-safety issue the reviewer flagged for PowerVS and OpenStack.
🛡️ Add nil guard before accessing StreamMetadata
func getWindowsAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) { if releaseImage == nil { return "", fmt.Errorf("release image is nil") } if releaseImage.StreamMetadata == nil { - return "", fmt.Errorf("release image stream metadata is nil") + return "", fmt.Errorf("release image stream metadata is nil") } archData, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]🤖 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` at line 434, Guard against nil releaseImage.StreamMetadata before accessing Architectures: in the code path using releaseImage.StreamMetadata.Architectures (the line computing archData, foundArch), add a nil check on releaseImage.StreamMetadata and handle the nil case (e.g., skip this lookup, set foundArch=false or use the alternative streams metadata lookup) to avoid dereferencing a nil StreamMetadata; update any downstream logic that assumes foundArch accordingly. Ensure references include releaseImage, StreamMetadata, Architectures, archData/foundArch, hyperv1.ArchAliases and specifiedArch so the change is applied exactly where the lookup happens.hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go (1)
47-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMultiple nil-safety risks in KubeVirt image resolution.
Two nil-dereference risks:
- Line 47:
releaseImage.StreamMetadata.Architectures— ifStreamMetadatais nil (multi-stream-only payload), this panics- Line 53:
arch.Images.Kubevirt.DigestRef— ifarch.Images.Kubevirtis nil, accessing.DigestRefpanicsThese are the same blocking nil-safety issues flagged for PowerVS/OpenStack.
🛡️ Add nil guards before dereferencing
func defaultImage(nodePoolArch string, releaseImage *releaseinfo.ReleaseImage) (string, string, error) { + if releaseImage.StreamMetadata == nil { + return "", "", fmt.Errorf("release image stream metadata is nil") + } + var archName string switch nodePoolArch { case hyperv1.ArchitectureS390X: archName = hyperv1.ArchitectureS390X default: archName = hyperv1.ArchAliases[hyperv1.ArchitectureAMD64] } arch, foundArch := releaseImage.StreamMetadata.Architectures[archName] if !foundArch { return "", "", fmt.Errorf("couldn't find OS metadata for architecture %q", archName) } + if arch.Images.Kubevirt == nil { + return "", "", fmt.Errorf("no kubevirt image metadata present in release") + } if arch.Images.Kubevirt.DigestRef == "" { return "", "", fmt.Errorf("no kubevirt image metadata present in release") }🤖 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/kubevirt/kubevirt.go` around lines 47 - 56, Check for nil StreamMetadata and nil nested image structs before dereferencing: guard that releaseImage.StreamMetadata is not nil before indexing into StreamMetadata.Architectures[archName], and after finding arch, ensure arch.Images and arch.Images.Kubevirt are non-nil before reading DigestRef; if any are nil return a clear error (e.g., "no stream metadata", "no image metadata for architecture %q", or "no kubevirt image metadata present in release") so containerImage is only assigned when these checks pass.hypershift-operator/controllers/nodepool/openstack/openstack.go (1)
134-135:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing nil-safety guard for
releaseImage.StreamMetadata(blocking issue).Line 135 dereferences
releaseImage.StreamMetadata.Architectureswithout checking ifStreamMetadatais nil. When a payload uses only the newstreamsmetadata format,StreamMetadatacan be nil, causing a panic.This is the blocking nil-safety issue the reviewer explicitly flagged for OpenStack in the PR objectives summary.
🛡️ Add nil guard before accessing StreamMetadata
func OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage) (string, string, error) { + if releaseImage.StreamMetadata == nil { + return "", "", fmt.Errorf("release image stream metadata is nil") + } + arch, foundArch := releaseImage.StreamMetadata.Architectures["x86_64"] if !foundArch {🤖 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 134 - 135, OpenstackDefaultImage currently dereferences releaseImage.StreamMetadata without a nil check; add a guard at the start of OpenstackDefaultImage to check if releaseImage == nil or releaseImage.StreamMetadata == nil before accessing StreamMetadata.Architectures, and if nil return a clear error (or fallback to the alternative streams metadata if available) so the function never panics when StreamMetadata is absent; reference the symbols OpenstackDefaultImage, releaseImage, StreamMetadata, and Architectures when applying the check and returning the error/fallback.hypershift-operator/controllers/nodepool/azure.go (1)
124-139:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMultiple nil-safety risks in marketplace metadata extraction.
Lines 128 and 136-137 dereference nested structs without nil-guarding intermediate fields:
- Line 128:
releaseImage.StreamMetadata.Architectures— ifStreamMetadatais nil (multi-stream-only payload), this panics- Line 136:
archData.RHCOS.Marketplace.Azure.NoPurchasePlan— ifRHCOS.MarketplaceorMarketplace.Azureis nil, this panicsThese are the same blocking nil-safety issues flagged for PowerVS/OpenStack.
🛡️ Add nil guards for nested struct access
func getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string) (*azureMarketplaceMetadata, error) { if releaseImage.StreamMetadata == nil { - return nil, nil // No stream metadata available + return nil, nil } archData, foundArch := releaseImage.StreamMetadata.Architectures[arch] if !foundArch { return nil, fmt.Errorf("architecture %s not found in stream metadata", arch) } + // Check for nil at each level of nested access + if archData.RHCOS.Marketplace == nil || archData.RHCOS.Marketplace.Azure == nil { + return nil, nil // No Azure marketplace data available + } + azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan if azureMarketplace.HyperVGen1 == nil && azureMarketplace.HyperVGen2 == 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.go` around lines 124 - 139, The code dereferences nested fields without nil checks causing panics; add explicit nil guards before accessing releaseImage.StreamMetadata and before archData.RHCOS, archData.RHCOS.Marketplace, archData.RHCOS.Marketplace.Azure and archData.RHCOS.Marketplace.Azure.NoPurchasePlan; specifically, in the function that reads releaseImage.StreamMetadata.Architectures and assigns azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan, first check if releaseImage.StreamMetadata == nil (return nil,nil), then after finding archData verify archData.RHCOS != nil && archData.RHCOS.Marketplace != nil && archData.RHCOS.Marketplace.Azure != nil && archData.RHCOS.Marketplace.Azure.NoPurchasePlan != nil (return nil,nil if any are nil) before using azureMarketplace and its HyperVGen1/Gen2 fields.
🧹 Nitpick comments (5)
hypershift-operator/controllers/nodepool/powervs.go (1)
111-131: ⚡ Quick winPowerVS doesn't support stream-aware image selection.
getPowerVSImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means PowerVS images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with AWS's stream-aware pattern and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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.go` around lines 111 - 131, The getPowerVSImage function currently reads releaseImage.StreamMetadata directly; change its signature to accept an optional stream parameter (e.g., stream ...string) and obtain the architecture metadata via releaseImage.StreamMetadataForStream(stream) instead of releaseImage.StreamMetadata so PowerVS becomes stream-aware; update the function body to use the returned StreamMetadata for the "ppc64le" lookup and keep the rest of the logic unchanged, and then update all call sites of getPowerVSImage to pass the stream when available.hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go (1)
39-67: ⚡ Quick winKubeVirt doesn't support stream-aware image selection.
defaultImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means KubeVirt images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with AWS and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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/kubevirt/kubevirt.go` around lines 39 - 67, The defaultImage function currently reads releaseImage.StreamMetadata directly, which prevents honoring spec.osImageStream; modify func defaultImage(nodePoolArch string, releaseImage *releaseinfo.ReleaseImage, stream ...string) (string, string, error) to accept an optional stream parameter, call releaseImage.StreamMetadataForStream(stream) to obtain the appropriate StreamMetadata (falling back when stream is empty), then use the returned metadata for the architecture lookup (arch, foundArch := streamMetadata.Architectures[archName]) and keep the existing validation and return values unchanged; ensure callers of defaultImage are updated to pass the optional stream where stream-aware selection is required.hypershift-operator/controllers/nodepool/openstack/openstack.go (1)
132-153: ⚡ Quick winOpenStack doesn't support stream-aware image selection.
OpenstackDefaultImageaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter. This means OpenStack images can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support, unlike AWS which correctly uses stream-aware selection.For consistency with AWS and to support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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 132 - 153, OpenstackDefaultImage currently reads releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change the function signature to accept an optional variadic stream parameter (e.g., OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage, stream ...string)) and use releaseImage.StreamMetadataForStream(stream) to obtain the stream-aware metadata before looking up Architectures["x86_64"], Artifacts["openstack"], Formats["qcow2.gz"] and the disk; ensure the function otherwise returns the same (disk.Location, disk.SHA256, error) and preserves existing error messages when lookups fail.hypershift-operator/controllers/nodepool/aws.go (1)
424-453: ⚡ Quick winWindows AMI path doesn't support stream-aware image selection.
getWindowsAMIaccessesreleaseImage.StreamMetadatadirectly and doesn't accept a stream parameter, unlike the Linux/RHCOS path which correctly passesnodePool.Spec.OSImageStream.NametodefaultNodePoolAMI(lines 137, 364). This means Windows AMIs can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency with the Linux path and to properly support multi-stream payloads, consider:
- Adding an optional
stream ...stringparameter togetWindowsAMI- Using
releaseImage.StreamMetadataForStream(stream)instead of directStreamMetadataaccess- Passing
nodePool.Spec.OSImageStream.Namefrom callers (lines 343, 130)🤖 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 424 - 453, getWindowsAMI currently reads releaseImage.StreamMetadata directly so it ignores spec.osImageStream and cannot select stream-aware Windows AMIs; change getWindowsAMI to accept an optional stream parameter (e.g., stream ...string) and use releaseImage.StreamMetadataForStream(stream) instead of releaseImage.StreamMetadata, update callers (notably defaultNodePoolAMI and places that resolve AMIs) to pass nodePool.Spec.OSImageStream.Name when present so Windows AMI lookup mirrors the Linux/RHCOS stream-aware behavior.hypershift-operator/controllers/nodepool/azure.go (1)
122-167: ⚡ Quick winAzure marketplace doesn't support stream-aware image selection.
getAzureMarketplaceMetadataaccessesreleaseImage.StreamMetadatadirectly without accepting a stream parameter, unlike AWS'sdefaultNodePoolAMIwhich correctly usesStreamMetadataForStream(). This means Azure can't respectspec.osImageStreamfor dual-stream RHEL 9/10 support.For consistency and to properly support multi-stream payloads, consider adding an optional
stream ...stringparameter and usingreleaseImage.StreamMetadataForStream(stream).🤖 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.go` around lines 122 - 167, getAzureMarketplaceMetadata currently reads releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change its signature to accept an optional stream parameter (e.g., getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string, stream ...string)) and use releaseImage.StreamMetadataForStream(stream) (falling back to existing StreamMetadata when no stream passed) instead of direct access to releaseImage.StreamMetadata; update internal references (archData lookup, azureMarketplace extraction) to use the returned stream metadata so Azure respects Stream-aware image selection consistent with defaultNodePoolAMI and supports dual-stream RHEL 9/10 scenarios.
🤖 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_test.go`:
- Around line 716-733: The test case named "When RHELCoreOSExtensions is nil, it
should return error" has an expectedError string that doesn't match the actual
error returned by getWindowsAMI; update the test's expectedError to match the
production error path (e.g., the "no aws-winli regions data found..." message)
or change the fixture to trigger the rhel-coreos-extensions error if that's the
intended behavior; locate the test struct in aws_test.go (the case with name
matching the above), and set expectedError to the exact error returned by
getWindowsAMI for that input shape (or adjust the releaseImage/StreamMetadata so
getWindowsAMI hits the rhel-coreos-extensions branch).
In `@hypershift-operator/controllers/nodepool/powervs_test.go`:
- Around line 11-52: Add a test case to TestGetPowerVSImage that covers
releaseImage == nil and releaseImage.StreamMetadata == nil to prevent the
nil-dereference panic, and update getPowerVSImage to defensively check for nil
releaseImage and nil StreamMetadata at the top (returning a clear error like
"release image metadata is nil" or similar) before accessing
StreamMetadata.Architectures; reference getPowerVSImage in your patch and ensure
the new tests assert an error containing the new message.
In `@hypershift-operator/controllers/nodepool/powervs.go`:
- Around line 117-119: Add a nil-check for releaseImage.StreamMetadata before
dereferencing it: ensure releaseImage.StreamMetadata != nil before accessing
releaseImage.StreamMetadata.Architectures (the code that constructs arch and
later checks arch.Images.PowerVS.Regions). If StreamMetadata is nil, return an
appropriate error (similar to the existing error path) so the function does not
panic; reference releaseImage, StreamMetadata, Architectures, arch, Images,
PowerVS, and Regions when locating where to insert this guard.
In `@ignition-server/cmd/run_local_ignitionprovider.go`:
- Line 114: The call to GetPayload is ignoring the CLI --feature-gate-manifest
because the fifth argument is a literal empty string; update the GetPayload call
so it passes the stored option o.FeatureGateManifest (the same value previously
set on the provider) instead of the empty string. Locate the call to
p.GetPayload(...) and replace the 5th parameter with o.FeatureGateManifest so
the feature-gate manifest flag is forwarded to GetPayload.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/aws.go`:
- Line 434: Guard against nil releaseImage.StreamMetadata before accessing
Architectures: in the code path using releaseImage.StreamMetadata.Architectures
(the line computing archData, foundArch), add a nil check on
releaseImage.StreamMetadata and handle the nil case (e.g., skip this lookup, set
foundArch=false or use the alternative streams metadata lookup) to avoid
dereferencing a nil StreamMetadata; update any downstream logic that assumes
foundArch accordingly. Ensure references include releaseImage, StreamMetadata,
Architectures, archData/foundArch, hyperv1.ArchAliases and specifiedArch so the
change is applied exactly where the lookup happens.
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 124-139: The code dereferences nested fields without nil checks
causing panics; add explicit nil guards before accessing
releaseImage.StreamMetadata and before archData.RHCOS,
archData.RHCOS.Marketplace, archData.RHCOS.Marketplace.Azure and
archData.RHCOS.Marketplace.Azure.NoPurchasePlan; specifically, in the function
that reads releaseImage.StreamMetadata.Architectures and assigns
azureMarketplace := archData.RHCOS.Marketplace.Azure.NoPurchasePlan, first check
if releaseImage.StreamMetadata == nil (return nil,nil), then after finding
archData verify archData.RHCOS != nil && archData.RHCOS.Marketplace != nil &&
archData.RHCOS.Marketplace.Azure != nil &&
archData.RHCOS.Marketplace.Azure.NoPurchasePlan != nil (return nil,nil if any
are nil) before using azureMarketplace and its HyperVGen1/Gen2 fields.
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go`:
- Around line 47-56: Check for nil StreamMetadata and nil nested image structs
before dereferencing: guard that releaseImage.StreamMetadata is not nil before
indexing into StreamMetadata.Architectures[archName], and after finding arch,
ensure arch.Images and arch.Images.Kubevirt are non-nil before reading
DigestRef; if any are nil return a clear error (e.g., "no stream metadata", "no
image metadata for architecture %q", or "no kubevirt image metadata present in
release") so containerImage is only assigned when these checks pass.
In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 134-135: OpenstackDefaultImage currently dereferences
releaseImage.StreamMetadata without a nil check; add a guard at the start of
OpenstackDefaultImage to check if releaseImage == nil or
releaseImage.StreamMetadata == nil before accessing
StreamMetadata.Architectures, and if nil return a clear error (or fallback to
the alternative streams metadata if available) so the function never panics when
StreamMetadata is absent; reference the symbols OpenstackDefaultImage,
releaseImage, StreamMetadata, and Architectures when applying the check and
returning the error/fallback.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/aws.go`:
- Around line 424-453: getWindowsAMI currently reads releaseImage.StreamMetadata
directly so it ignores spec.osImageStream and cannot select stream-aware Windows
AMIs; change getWindowsAMI to accept an optional stream parameter (e.g., stream
...string) and use releaseImage.StreamMetadataForStream(stream) instead of
releaseImage.StreamMetadata, update callers (notably defaultNodePoolAMI and
places that resolve AMIs) to pass nodePool.Spec.OSImageStream.Name when present
so Windows AMI lookup mirrors the Linux/RHCOS stream-aware behavior.
In `@hypershift-operator/controllers/nodepool/azure.go`:
- Around line 122-167: getAzureMarketplaceMetadata currently reads
releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change
its signature to accept an optional stream parameter (e.g.,
getAzureMarketplaceMetadata(releaseImage *releaseinfo.ReleaseImage, arch string,
stream ...string)) and use releaseImage.StreamMetadataForStream(stream) (falling
back to existing StreamMetadata when no stream passed) instead of direct access
to releaseImage.StreamMetadata; update internal references (archData lookup,
azureMarketplace extraction) to use the returned stream metadata so Azure
respects Stream-aware image selection consistent with defaultNodePoolAMI and
supports dual-stream RHEL 9/10 scenarios.
In `@hypershift-operator/controllers/nodepool/kubevirt/kubevirt.go`:
- Around line 39-67: The defaultImage function currently reads
releaseImage.StreamMetadata directly, which prevents honoring
spec.osImageStream; modify func defaultImage(nodePoolArch string, releaseImage
*releaseinfo.ReleaseImage, stream ...string) (string, string, error) to accept
an optional stream parameter, call releaseImage.StreamMetadataForStream(stream)
to obtain the appropriate StreamMetadata (falling back when stream is empty),
then use the returned metadata for the architecture lookup (arch, foundArch :=
streamMetadata.Architectures[archName]) and keep the existing validation and
return values unchanged; ensure callers of defaultImage are updated to pass the
optional stream where stream-aware selection is required.
In `@hypershift-operator/controllers/nodepool/openstack/openstack.go`:
- Around line 132-153: OpenstackDefaultImage currently reads
releaseImage.StreamMetadata directly and thus ignores spec.osImageStream; change
the function signature to accept an optional variadic stream parameter (e.g.,
OpenstackDefaultImage(releaseImage *releaseinfo.ReleaseImage, stream ...string))
and use releaseImage.StreamMetadataForStream(stream) to obtain the stream-aware
metadata before looking up Architectures["x86_64"], Artifacts["openstack"],
Formats["qcow2.gz"] and the disk; ensure the function otherwise returns the same
(disk.Location, disk.SHA256, error) and preserves existing error messages when
lookups fail.
In `@hypershift-operator/controllers/nodepool/powervs.go`:
- Around line 111-131: The getPowerVSImage function currently reads
releaseImage.StreamMetadata directly; change its signature to accept an optional
stream parameter (e.g., stream ...string) and obtain the architecture metadata
via releaseImage.StreamMetadataForStream(stream) instead of
releaseImage.StreamMetadata so PowerVS becomes stream-aware; update the function
body to use the returned StreamMetadata for the "ppc64le" lookup and keep the
rest of the logic unchanged, and then update all call sites of getPowerVSImage
to pass the stream when available.
🪄 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: c710fcdb-cc61-470d-bd2a-c5e6b808e139
📒 Files selected for processing (30)
hypershift-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/conditions.gohypershift-operator/controllers/nodepool/config.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/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/rhel_stream.gohypershift-operator/controllers/nodepool/rhel_stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.go
✅ Files skipped from review due to trivial changes (1)
- support/releaseinfo/registry_mirror_provider.go
🚧 Files skipped from review as they are similar to previous changes (13)
- support/releaseinfo/registryclient_provider.go
- ignition-server/controllers/tokensecret_controller.go
- hypershift-operator/controllers/nodepool/version_test.go
- hypershift-operator/controllers/nodepool/rhel_stream_test.go
- ignition-server/controllers/tokensecret_controller_test.go
- support/releaseinfo/deserialize_test.go
- hypershift-operator/controllers/nodepool/token.go
- hypershift-operator/controllers/nodepool/rhel_stream.go
- hypershift-operator/controllers/nodepool/conditions.go
- hypershift-operator/controllers/nodepool/nodepool_controller.go
- ignition-server/controllers/local_ignitionprovider.go
- support/releaseinfo/deserialize.go
- support/releaseinfo/releaseinfo_test.go
🛑 Comments failed to post (4)
hypershift-operator/controllers/nodepool/aws_test.go (1)
716-733:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpected error assertion does not match the current
getWindowsAMIcontract.At Line 732, the expected error string (
no rhel-coreos-extensions data found...) doesn’t match the production path shown inhypershift-operator/controllers/nodepool/aws.go(which returnsno aws-winli regions data found...for this fixture shape). This will make the case fail for the wrong reason.Suggested fix
- { - name: "When RHELCoreOSExtensions is nil, it should return error", + { + name: "When aws-winli regions are missing, it should return error", region: "us-east-1", arch: hyperv1.ArchitectureAMD64, releaseImage: &releaseinfo.ReleaseImage{ ImageStream: &v1.ImageStream{ ObjectMeta: metav1.ObjectMeta{ Name: "4.17.0", }, }, StreamMetadata: &releaseinfo.CoreOSStreamMetadata{ Architectures: map[string]releaseinfo.CoreOSArchitecture{ "x86_64": {}, }, }, }, - expectedError: "no rhel-coreos-extensions data found in release image metadata", + expectedError: "no aws-winli regions data found in release image metadata", },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{ name: "When aws-winli regions are missing, it should return error", region: "us-east-1", arch: hyperv1.ArchitectureAMD64, releaseImage: &releaseinfo.ReleaseImage{ ImageStream: &v1.ImageStream{ ObjectMeta: metav1.ObjectMeta{ Name: "4.17.0", }, }, StreamMetadata: &releaseinfo.CoreOSStreamMetadata{ Architectures: map[string]releaseinfo.CoreOSArchitecture{ "x86_64": {}, }, }, }, expectedError: "no aws-winli regions data found in release image metadata", },🤖 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_test.go` around lines 716 - 733, The test case named "When RHELCoreOSExtensions is nil, it should return error" has an expectedError string that doesn't match the actual error returned by getWindowsAMI; update the test's expectedError to match the production error path (e.g., the "no aws-winli regions data found..." message) or change the fixture to trigger the rhel-coreos-extensions error if that's the intended behavior; locate the test struct in aws_test.go (the case with name matching the above), and set expectedError to the exact error returned by getWindowsAMI for that input shape (or adjust the releaseImage/StreamMetadata so getWindowsAMI hits the rhel-coreos-extensions branch).hypershift-operator/controllers/nodepool/powervs_test.go (1)
11-52:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNil metadata panic path is still untested (and likely still reachable).
Line 47 only exercises map-lookup failures. It does not cover
releaseImage == nil/releaseImage.StreamMetadata == nil, whilegetPowerVSImagecurrently dereferencesreleaseImage.StreamMetadatadirectly. Add this case so the panic path is caught and fixed.Suggested test addition
{ name: "When architecture is not found, it should return error", region: "us-south", releaseImage: &releaseinfo.ReleaseImage{ StreamMetadata: &releaseinfo.CoreOSStreamMetadata{ Architectures: map[string]releaseinfo.CoreOSArchitecture{}, }, }, expectedError: "couldn't find OS metadata for architecture", }, + { + name: "When stream metadata is nil, it should return error", + region: "us-south", + releaseImage: &releaseinfo.ReleaseImage{ + StreamMetadata: nil, + }, + expectedError: "release image stream metadata is 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/powervs_test.go` around lines 11 - 52, Add a test case to TestGetPowerVSImage that covers releaseImage == nil and releaseImage.StreamMetadata == nil to prevent the nil-dereference panic, and update getPowerVSImage to defensively check for nil releaseImage and nil StreamMetadata at the top (returning a clear error like "release image metadata is nil" or similar) before accessing StreamMetadata.Architectures; reference getPowerVSImage in your patch and ensure the new tests assert an error containing the new message.hypershift-operator/controllers/nodepool/powervs.go (1)
117-119:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPartial fix:
Regionsnil-guard added, but rootStreamMetadatanil-safety still missing.The added nil check for
arch.Images.PowerVS.Regionsis good, but line 112 still dereferencesreleaseImage.StreamMetadata.Architectureswithout guardingStreamMetadataitself. If a payload uses onlystreams(not legacystream),StreamMetadatacan be nil, causing a panic before reaching theRegionscheck.This is the primary blocking nil-safety issue the reviewer flagged in the PR objectives: "Nil-safety panic risk: powervs.go accesses releaseImage.StreamMetadata directly; if StreamMetadata is nil (e.g., payload uses only streams), this can panic."
🛡️ Add StreamMetadata nil guard before line 112
func getPowerVSImage(region string, releaseImage *releaseinfo.ReleaseImage) (*releaseinfo.CoreOSPowerVSImage, string, error) { + if releaseImage.StreamMetadata == nil { + return nil, "", fmt.Errorf("release image stream metadata is nil") + } + arch, foundArch := releaseImage.StreamMetadata.Architectures["ppc64le"] if !foundArch {🤖 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.go` around lines 117 - 119, Add a nil-check for releaseImage.StreamMetadata before dereferencing it: ensure releaseImage.StreamMetadata != nil before accessing releaseImage.StreamMetadata.Architectures (the code that constructs arch and later checks arch.Images.PowerVS.Regions). If StreamMetadata is nil, return an appropriate error (similar to the existing error path) so the function does not panic; reference releaseImage, StreamMetadata, Architectures, arch, Images, PowerVS, and Regions when locating where to insert this guard.ignition-server/cmd/run_local_ignitionprovider.go (1)
114-114:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe
--feature-gate-manifestflag is silently ignored.Line 114 passes
""as the 5th parameter (fgManifest) toGetPayload, but it should passo.FeatureGateManifest. The CLI accepts a--feature-gate-manifestflag (line 46), stores it ino.FeatureGateManifest, and even sets it on the provider struct (line 111), yet the value is never forwarded toGetPayload.🐛 Proposed fix
- payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", "", "") + payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", o.FeatureGateManifest, "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", o.FeatureGateManifest, "")🤖 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 `@ignition-server/cmd/run_local_ignitionprovider.go` at line 114, The call to GetPayload is ignoring the CLI --feature-gate-manifest because the fifth argument is a literal empty string; update the GetPayload call so it passes the stored option o.FeatureGateManifest (the same value previously set on the provider) instead of the empty string. Locate the call to p.GetPayload(...) and replace the 5th parameter with o.FeatureGateManifest so the feature-gate manifest flag is forwarded to GetPayload.
|
Now I have the complete picture. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe unit test Root CauseThe PR replaces the external The critical structural change is:
The test at The Recommendations
Evidence
|
Starting in OCP 5.0, the coreos-bootimages ConfigMap contains a "streams" key alongside the legacy "stream" key, with per-stream boot image metadata for RHEL 9 and RHEL 10. This change: - Adds DeserializeImageMetadataMultiStream to parse both formats - Adds StreamsMetadata field to ReleaseImage for multi-stream data - Adds StreamMetadataForStream helper for stream-specific lookups - Updates RegistryClientProvider to populate both metadata fields The legacy DeserializeImageMetadata is kept for backward compatibility. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Implements per-NodePool RHEL stream selection via spec.osImageStream, enabling mixed RHEL 9 and RHEL 10 workers in a HostedCluster. Key changes: - getRHELStream() resolves the effective stream from spec, release version, and runc usage with proper validation and fallback logic - Explicit stream included in config hash to trigger rollouts; implicit defaults excluded to prevent fleet-wide mass rollouts on upgrade - Token secret carries os-stream key for ignition payload generation - Boot image resolution (AMI, GCP) supports stream-specific metadata - validMachineConfigCondition validates stream/version/runc constraints - status.osImageStream inferred from node RHCOS version strings - runc guard prevents RHEL 10 selection when runc configs are present Co-Authored-By: Claude Opus 4.6 <[email protected]>
Extends the ignition server to generate stream-specific ignition payloads by: - Adding osStream parameter to IgnitionProvider.GetPayload interface - Reading os-stream from token secret in TokenSecretReconciler - Generating 99_osimagestream.yaml CR in the MCC manifest directory between runMCO and runMCC, so MCC bootstrap discovers the requested stream via OCI labels and overrides baseOSContainerImage accordingly - Extracting resolveClusterConfigComponent to reduce GetPayload cyclomatic complexity When osStream is empty (legacy payloads < 5.0), no OSImageStream CR is generated and MCC uses the default BaseOSContainerImage. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Covers the full dual-stream implementation: - getRHELStream decision matrix (explicit/implicit streams, runc guard, version constraints) with 12 test cases - DeserializeImageMetadataMultiStream for legacy and multi-stream ConfigMap formats - inferOSStreamFromNodeInfo RHCOS version string parsing - writeOSImageStreamCR YAML generation including empty-stream no-op - Updated fakeIgnitionProvider to match new GetPayload signature Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add the Claude Code skill definition for the jira:solve command that automates JIRA issue resolution. Co-Authored-By: Claude Opus 4.6 <[email protected]>
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-3612
branch
from
e089955 to
cf44fbe
Compare
openshift-ci
Bot
added
the
area/ai
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@hypershift-jira-solve-ci[bot]: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Re: #8714 (comment) Done.
AI-assisted response via Claude Code |
2 participants
What this PR does / why we need it:
Implements dual-stream RHEL 9/10 NodePool support for OpenShift 5.0+ payloads.
Starting with OpenShift 5.0, release payloads carry both RHEL 9 and RHEL 10 boot images. This PR threads the OS stream selection end-to-end: from the NodePool spec through the controller, token secret, ignition server, and into MCC bootstrap manifests.
Key changes:
Boot image metadata parsing (
support/releaseinfo):DeserializeImageMetadatanow parses both the legacy single-stream"stream"key and the new multi-stream"streams"key from the boot image ConfigMap. AStreamMetadataForStreamhelper resolves stream-specific metadata with fallback to legacy.RHEL stream resolution (
nodepool/rhel_stream.go): Pure functiongetRHELStream()resolves the target RHEL stream based onspec.osImageStream.name, release version, and runc usage. RHEL 10 is the default for >= 5.0; runc forces RHEL 9 (RHEL 10 does not ship runc). Pre-5.0 releases use legacy single-stream behavior.Controller plumbing (
nodepool/): The resolved stream flows intorolloutConfigfor hash computation (only explicit stream changes trigger rollouts), intodefaultNodePoolAMI/defaultNodePoolGCPImagefor stream-aware boot image selection, and into the token secret asos-streamfor the ignition server.Ignition server (
ignition-server/):GetPayload()accepts the OS stream and writes anOSImageStreamCR (99_osimagestream.yaml) to the MCC manifest directory, telling MCC bootstrap which RHEL stream to use when rendering MachineConfigs.Status reporting (
nodepool/version.go):status.osImageStreamis populated by detecting the RHEL stream from CAPI MachineNodeInfo.OSImage(RHCOS 4xx = RHEL 9, 5xx = RHEL 10).Validation conditions:
rhel-10on release < 5.0 andrhel-10+ runc are rejected with clear condition messages.Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/CNTRLPLANE-3612
Special notes for your reviewer:
rhelStreamfield is included inrolloutConfig.Hash()but only populated from the explicitspec.osImageStream.name(not the resolved default). This avoids fleet-wide rollouts when upgrading to 5.0 — existing NodePools silently pick up the default stream without a rollout.ContainerRuntimeConfigwithdefaultRuntime=runc, the controller automatically falls back to RHEL 9 even on >= 5.0 releases."stream"key is absent, ensuring stable behavior.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-3612](https://redhat.atlassian.net/browse/CNTRLPLANE-3612)Summary by CodeRabbit
New Features
Bug Fixes
Improvements