CNTRLPLANE-3553: feat(nodepool): thread streamName through all platform callers#8834
CNTRLPLANE-3553: feat(nodepool): thread streamName through all platform callers#8834sdminonne wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughNode pool image resolution now accepts an explicit RHEL stream across Azure, KubeVirt, OpenStack, and PowerVS paths. Platform helpers resolve architecture and release metadata through Sequence Diagram(s)sequenceDiagram
participant NodePoolController
participant PlatformHelper
participant ReleaseImage
participant MachineTemplate
NodePoolController->>PlatformHelper: pass resolved RHEL stream
PlatformHelper->>ReleaseImage: resolve stream metadata
ReleaseImage-->>PlatformHelper: architecture and release data
PlatformHelper->>MachineTemplate: construct stream-specific image spec
MachineTemplate-->>NodePoolController: return platform template
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8834 +/- ##
==========================================
+ Coverage 43.72% 43.76% +0.03%
==========================================
Files 772 772
Lines 95933 95966 +33
==========================================
+ Hits 41944 41995 +51
+ Misses 51077 51053 -24
- Partials 2912 2918 +6
... and 3 files with indirect coverage changes
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.
🧹 Nitpick comments (2)
hypershift-operator/controllers/nodepool/openstack/openstack_test.go (1)
293-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a named-stream test case.
All updated call sites pass
"", which routes through the legacyStreamMetadatafallback inStreamForName. The newOSStreams/named-stream branch (e.g."rhel-9"/"rhel-10") — the actual purpose of this PR — isn't exercised here. A table entry that populatesOSStreamsand passes a concrete stream name would close that gap.Also applies to: 358-358, 472-472
🤖 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_test.go` at line 293, Add a table-driven named-stream test case for OpenstackDefaultImage so the new OSStreams path is covered instead of only the legacy StreamMetadata fallback. Update the relevant test cases in openstack_test.go to populate OSStreams and pass a concrete stream name like “rhel-9” or “rhel-10” through OpenstackDefaultImage / StreamForName, and keep the existing empty-string cases as fallback coverage. Ensure the added case exercises the branch introduced by this PR at the updated call sites.hypershift-operator/controllers/nodepool/config.go (1)
98-121: 📐 Maintainability & Code Quality | 🔵 TrivialFactor out the default-stream derivation.
config.gore-parses the release version and recomputes the default RHEL stream withGetRHELStream("", version, false), duplicating the same version-aware logic used bygetRHELStream. A shared helper, or returning the default alongside the resolved stream, would keep hash normalization and stream selection aligned ifusesRunchandling changes later.🤖 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/config.go` around lines 98 - 121, The config hash normalization in config.go duplicates the version-based default RHEL stream logic by re-parsing the release version and calling GetRHELStream("", version, false) separately from getRHELStream. Refactor this so the default stream is derived through a shared helper or returned together with the resolved stream from getRHELStream, and update the normalization logic in the same Config function to use that shared result so hash behavior stays aligned if usesRunc handling changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 98-121: The config hash normalization in config.go duplicates the
version-based default RHEL stream logic by re-parsing the release version and
calling GetRHELStream("", version, false) separately from getRHELStream.
Refactor this so the default stream is derived through a shared helper or
returned together with the resolved stream from getRHELStream, and update the
normalization logic in the same Config function to use that shared result so
hash behavior stays aligned if usesRunc handling changes.
In `@hypershift-operator/controllers/nodepool/openstack/openstack_test.go`:
- Line 293: Add a table-driven named-stream test case for OpenstackDefaultImage
so the new OSStreams path is covered instead of only the legacy StreamMetadata
fallback. Update the relevant test cases in openstack_test.go to populate
OSStreams and pass a concrete stream name like “rhel-9” or “rhel-10” through
OpenstackDefaultImage / StreamForName, and keep the existing empty-string cases
as fallback coverage. Ensure the added case exercises the branch introduced by
this PR at the updated call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 28adccd3-0671-4206-ac2d-917bb3007eb9
📒 Files selected for processing (27)
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/capi_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/osstream.gohypershift-operator/controllers/nodepool/osstream_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/stream.gohypershift-operator/controllers/nodepool/stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.go
sdminonne
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-3553 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
ee9c4de to
55f1400
Compare
openshift-ci
Bot
added
area/api
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, "", releaseImage) | ||
| // Default behavior for Linux/RHCOS AMIs. | ||
| // TODO(CNTRLPLANE-3553): hardcode to rhel-9 until the MCO can install | ||
| // rhel-10 OS images. Use getRHELStreamForBootImage once MCO support lands. |
There was a problem hiding this comment.
I notice setAWSConditions hardcodes StreamRHEL9 here, but the other platform condition-setters (setKubevirtConditions, setOpenStackConditions, setPowerVSconditions) all call getRHELStreamForBootImage() for version-aware resolution. Is there a reason AWS is different? If the intent is to hardcode everywhere until MCO rhel-10 lands, the others should match. If the others should be version-aware, AWS should too.
There was a problem hiding this comment.
No there's no reason it's the status of MCO that enforce us to hardcode to rhel-9. Need to be done across all the platforms
There was a problem hiding this comment.
Agreed with this observation. Worth noting that the inconsistency goes beyond cross-platform: within KubeVirt, OpenStack, and PowerVS themselves, the condition-setter path calls getRHELStreamForBootImage() (which resolves to rhel-10 for OCP 5.x) while the CAPI machine-template path uses c.resolvedRHELStreamForBootImage (hardcoded to rhel-9). So the condition can validate against rhel-10 metadata while machines actually boot with rhel-9. Hardcoding StreamRHEL9 in all condition-setters (matching AWS) would keep both paths consistent per-platform until MCO rhel-10 support lands.
There was a problem hiding this comment.
Good catch. Agreed with both your and @cblecker's observation — the inconsistency between condition-setter and CAPI machine-template paths within the same platform was the real
issue. Fixed in 5c532dc: all condition-setters (setKubevirtConditions, setOpenStackConditions, setPowerVSconditions) now hardcode StreamRHEL9 matching the AWS pattern, with the same
TODO(CNTRLPLANE-3553) marker so they all get unwound together once MCO rhel-10 support lands.
| // | ||
| // This field is intentionally outside rolloutConfig because it does not | ||
| // participate in the config hash that drives rollouts. | ||
| resolvedRHELStreamForBootImage string |
There was a problem hiding this comment.
Having two stream-related fields (resolvedRHELStreamForBootImage on ConfigGenerator and rhelStream on rolloutConfig) could trip up future contributors. The comment here explains the separation well, but rhelStream in rolloutConfig doesn't point back. A short "see also resolvedRHELStreamForBootImage" there would help.
There was a problem hiding this comment.
Done — added a // See also ConfigGenerator.resolvedRHELStreamForBootImage comment on the rhelStream field in rolloutConfig.
|
|
||
| const ( | ||
| // OSImageStreamRHEL9 is the OS image stream name for RHEL 9. | ||
| // This value must match support/releaseinfo.StreamRHEL9. |
There was a problem hiding this comment.
These "must match" cross-reference comments are a manual invariant that'll rot. Could we add a compile-time check instead? Something like:
func TestStreamConstantsMatch(t *testing.T) {
assert.Equal(t, hyperv1.OSImageStreamRHEL9, releaseinfo.StreamRHEL9)
assert.Equal(t, hyperv1.OSImageStreamRHEL10, releaseinfo.StreamRHEL10)
}That way the comment isn't the only thing keeping them in sync.
There was a problem hiding this comment.
Added an init() assertion in stream.go that panics immediately at startup if hyperv1.OSImageStreamRHEL9 != releaseinfo.StreamRHEL9 (or RHEL10).
Go doesn't support true compile-time
string assertions, but init() runs before any controller code and will crash the binary if the constants diverge. Also added TestStreamConstantsMatch as a belt-and-suspenders check
in CI.
|
|
||
| if !isOCP5Plus { | ||
| return "", nil | ||
| return StreamRHEL9, nil |
There was a problem hiding this comment.
There was a problem hiding this comment.
This is safe for the parent PRs. StreamForName("rhel-9") falls back to StreamMetadata for pre-5.0 payloads where OSStreams is nil, so the resolution path is the same as before. The
only difference is the name passed is "rhel-9" instead of "", and StreamForName handles both — empty string returns StreamMetadata directly, and a named string on a nil OSStreams
also falls back to StreamMetadata. Verified against #8730 and #8709.
| g := NewGomegaWithT(t) | ||
|
|
||
| err := defaultAzureNodePoolImage(tc.nodePool, tc.releaseImage) | ||
| err := defaultAzureNodePoolImage(tc.nodePool, tc.releaseImage, "") |
There was a problem hiding this comment.
The source changes in azure.go touch 3 functions (+8/-7), but the test only passes "" as stream. Could we add at least one case with a named stream and multi-stream ReleaseImage? The AWS tests do this well — same coverage here would be great.
There was a problem hiding this comment.
Added a test case "When named stream is used with multi-stream ReleaseImage it should resolve marketplace from the named stream" that populates OSStreams with a "rhel-9" entry and
passes it as streamName.
| testRI = ri | ||
| } | ||
| img, digest, err := defaultImage(tt.arch, testRI) | ||
| img, digest, err := defaultImage(tt.arch, testRI, "") |
There was a problem hiding this comment.
Same — defaultImage, GetImage, and MachineTemplateSpec all gained a streamName parameter but tests only pass "". One test with a named stream + OSStreams would confirm the wiring works for KubeVirt.
There was a problem hiding this comment.
Added "When named stream is used with multi-stream ReleaseImage it should resolve from the named stream" with OSStreams and streamName: "rhel-9".
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| url, hash, err := OpenstackDefaultImage(tc.releaseImage) | ||
| url, hash, err := OpenstackDefaultImage(tc.releaseImage, "") |
There was a problem hiding this comment.
All five OpenStack functions only pass "" for the new parameter. Given OpenStack had the most changes (+19/-11), at least one happy-path test with a named stream would be reassuring.
There was a problem hiding this comment.
Added named-stream test cases to both TestOpenstackDefaultImage and TestOpenStackReleaseImage.
| expectedCondType: string(hyperv1.NodePoolValidPlatformImageType), | ||
| expectedCondValue: corev1.ConditionFalse, | ||
| }, | ||
| // TODO(CNTRLPLANE-3553): re-enable once getRHELStreamForBootImage is |
There was a problem hiding this comment.
nit: Commented-out test code is easy to forget. Maybe worth adding a // TODO(CNTRLPLANE-3553): re-enable once hardcoding is removed on the commented block so a future grep picks it up — otherwise this will silently stay dead.
There was a problem hiding this comment.
Already has the grep-friendly TODO(CNTRLPLANE-3553) marker — left as-is.
Wire the resolved RHEL stream name through Azure, PowerVS, OpenStack, and KubeVirt platform resolvers so that each platform selects boot image metadata from the correct OS stream (rhel-9 or rhel-10) instead of always using the legacy default. CAPI paths use c.resolvedRHELStream from ConfigGenerator. Condition setter paths resolve the stream via getRHELStream(). All functions that previously accessed releaseImage.StreamMetadata directly now go through releaseImage.StreamForName(streamName). Ref: CNTRLPLANE-3553 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Hardcode StreamRHEL9 in setKubevirtConditions, setOpenStackConditions, and setPowerVSconditions to match the AWS pattern, keeping condition- setter and CAPI machine-template paths consistent until MCO rhel-10 support lands. - Add init() assertion and TestStreamConstantsMatch to enforce that API constants (hyperv1.OSImageStreamRHEL9/10) match releaseinfo constants (releaseinfo.StreamRHEL9/10). - Add cross-reference comment on rolloutConfig.rhelStream pointing to ConfigGenerator.resolvedRHELStreamForBootImage. - Add named-stream test cases for Azure, KubeVirt, and OpenStack that exercise the OSStreams/StreamForName path with a concrete stream name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-rebase
55f1400 to
5c532dc
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
|
@sdminonne: 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. |
|
I now have a complete picture. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis is a code coverage threshold failure, not a test execution failure. PR #8834 threads a Root CauseThe coverage shortfall is structural, not a missing-test oversight. The 44 uncovered lines fall into three categories: 1. Reconciler/CAPI methods (0% file-level coverage, ~22 lines): These are methods on
2. Error-path branches in library functions (~16 lines): The new
3. Intentionally-unreachable panic guardrails (~6 lines): The
The pure-logic functions that the PR actually modifies — Recommendations
Evidence
|
|
Closing in favor of #8832, which includes all commits from this PR plus runc detection. |
- Hardcode StreamRHEL9 in setKubevirtConditions, setOpenStackConditions, and setPowerVSconditions to match the AWS pattern, keeping condition- setter and CAPI machine-template paths consistent until MCO rhel-10 support lands. - Add init() assertion and TestStreamConstantsMatch to enforce that API constants (hyperv1.OSImageStreamRHEL9/10) match releaseinfo constants (releaseinfo.StreamRHEL9/10). - Add cross-reference comment on rolloutConfig.rhelStream pointing to ConfigGenerator.resolvedRHELStreamForBootImage. - Add named-stream test cases for Azure, KubeVirt, and OpenStack that exercise the OSStreams/StreamForName path with a concrete stream name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 participants
Summary
Wire the resolved RHEL stream name (
streamName) through Azure, PowerVS, OpenStack, and KubeVirt platform resolvers so each platform selects boot image metadata from the correct OS stream (rhel-9 or rhel-10) instead of always using the legacy defaultStreamMetadata.c.resolvedRHELStreamForBootImagefromConfigGenerator; condition-setter paths resolve viagetRHELStreamForBootImage().releaseImage.StreamMetadatadirectly now go throughreleaseImage.StreamForName(streamName).PR Dependency Graph
OSImageStreamfield to NodePoolGetRHELStream+ stream constantsPlatforms Updated
defaultAzureNodePoolImage,getAzureMarketplaceMetadatagetPowerVSImage,ibmPowerVSMachineTemplateSpec,setPowerVSconditionsOpenstackDefaultImage,OpenStackReleaseImage,MachineTemplateSpec,ReconcileOpenStackImageSpec,PrefixedClusterImageName,setOpenStackConditions,reconcileOpenStackImageCRdefaultImage,GetImage,MachineTemplateSpec,setKubevirtConditionsTest plan
""(empty/default) stream nameOSStreamspopulated onReleaseImage(integration)e2e-aws-upgrade-hypershift-operatorto validate no spurious rolloutsRef: CNTRLPLANE-3553
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes