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 3 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. |
📝 WalkthroughWalkthroughChangesThis PR introduces explicit RHEL stream resolution ( Sequence Diagram(s)sequenceDiagram
participant Reconciler as NodePoolReconciler
participant Stream as getRHELStream
participant Platform as Platform Image Resolver
participant Machines as Machine List
participant Status as NodePool.Status
Reconciler->>Stream: resolve RHEL stream (NodePool, ReleaseImage)
Stream-->>Reconciler: resolved stream name
Reconciler->>Platform: resolveAMI/Image(streamName)
Platform-->>Reconciler: image reference
Reconciler->>Machines: list machines for NodePool
Machines-->>Reconciler: machine OS image data
Reconciler->>Reconciler: infer majority stream from OS images
Reconciler->>Status: set OSImageStream (if majority found)
Compact metadata
Related issues: None specified in the provided summary. Suggested labels: area/nodepool, area/api, needs-tests-review Suggested reviewers: hypershift-nodepool-maintainers Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
![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
|
Now I have the full picture. Let me compile the final analysis. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryNo Prow CI jobs ran. The Root CauseThe tide Conflicting PRs merged to
PR #8834 touches all of these files as part of threading Additional blocking factors:
Because of the merge conflict, tide cannot merge the PR and all Prow presubmit jobs remain gated in Recommendations
Evidence
|
Wire spec.osImageStream into the NodePool reconciliation loop: validation, config hash, boot image resolution, token secret propagation, and observation-based status reporting. Behavior matches the dual-stream RHEL NodePool enhancement: https://github.com/openshift/enhancements/blob/master/enhancements/hypershift/dual-stream-rhel-nodepool.md Stream resolution (osstream.go, stream.go): - getRHELStreamForBootImage delegates to GetRHELStream for version-aware default resolution: rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0 when spec.osImageStream is unset. On upgrade to OCP 5.0+, existing NodePools with unset spec.osImageStream will implicitly transition to rhel-10 boot images as intended by the enhancement. - GetRHELStream always returns a concrete stream name ("rhel-9" or "rhel-10") and is used for validation and default resolution. - validateOSImageStream delegates to GetRHELStream; runs before NewConfigGenerator in validMachineConfigCondition for fail-fast. Config hash (config.go): - Add rhelStream to rolloutConfig and Hash()/HashWithoutVersion(). Normalized so that setting the version-derived default (e.g. "rhel-9" on 4.x) keeps rhelStream empty — no spurious rollout. - resolvedRHELStreamForBootImage lives on ConfigGenerator outside rolloutConfig since it does not participate in the hash. Boot image resolution (aws.go, gcp.go): - AWS and GCP machine template paths use resolvedRHELStreamForBootImage via StreamForName for consistent AMI/image lookup. - setAWSConditions uses getRHELStreamForBootImage for consistency with the CAPI path. Token secret (token.go): - Write os-stream key with resolved stream for future ignition-server consumption (CNTRLPLANE-3553). Status reporting (version.go): - setOSImageStreamStatus infers the observed RHEL stream from Machine NodeInfo.OSImage (RHCOS 4xx → rhel-9, 5xx → rhel-10) using majority vote across pool machines. - setNodesInfoStatus aggregates node version and health from CAPI Machines into status.nodesInfo. Tests (osstream_test.go): - Test_getRHELStreamForBootImage covers all combinations of osImageStream.Name × release version per the enhancement behavior table. - TestGetRHELStreamForBootImage proves that on OCP 5.0+ multi-stream payloads, the resolved rhel-10 default produces the correct AMI and machine template hash, confirming the intended boot image transition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s rhel-10
The MCO cannot install rhel-10 OS images yet (the ExternalTopologyMode
guard in MCC bootstrap skips OSImageStream processing for HyperShift).
Hardcode resolvedRHELStreamForBootImage to StreamRHEL9 so that all
NodePools resolve rhel-9 boot AMIs regardless of release version.
Without this, NodePools on OCP 5.0+ payloads would boot a rhel-10 AMI
while the MCO installs rhel-9, and the HyperShift Operator upgrade test
would fail because the template hash changes from StreamForName("") to
StreamForName("rhel-10").
Once the MCO removes the ExternalTopologyMode guard (MCO PR openshift#5750),
getRHELStreamForBootImage should be wired back in for version-aware
stream resolution.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
sdminonne
force-pushed
the
CNTRLPLANE-3553-thread-streamName-all-platforms
branch
from
ee9c4de to
55f1400
Compare
openshift-ci
Bot
added
area/api
|
[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 |
2 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