CNTRLPLANE-3871: resolve RHEL stream dynamically for boot images#9099
CNTRLPLANE-3871: resolve RHEL stream dynamically for boot images#9099jparrill wants to merge 3 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
|
@jparrill: This pull request references CNTRLPLANE-3871 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:
📝 WalkthroughWalkthroughNodePool reconciliation now resolves the RHEL stream for the boot image and passes it to platform condition handling, configuration generation, token creation, and secret janitor reconciliation. AWS, KubeVirt, PowerVS, and OpenStack image selection use the resolved stream instead of hardcoded RHEL 9. Resolution failures return wrapped errors, and tests cover stream resolution and invalid OS image stream validation. Sequence Diagram(s)sequenceDiagram
participant NodePoolReconciler
participant GetRHELStreamForBootImage
participant PlatformImageDiscovery
participant NewConfigGenerator
NodePoolReconciler->>GetRHELStreamForBootImage: resolve boot-image RHEL stream
GetRHELStreamForBootImage-->>NodePoolReconciler: resolved stream or error
NodePoolReconciler->>PlatformImageDiscovery: select image with resolved stream
NodePoolReconciler->>NewConfigGenerator: generate config with resolved stream
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
approved
|
/hold Until this PR got merged: |
openshift-ci
Bot
added
the
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/openstack.go (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid introducing an outer
errthat downstream code shadows.Use a name such as
resolveErrfor this lookup. The new function-scopeerrmakes the existing short declarations inside the conditional shadow error variables, contrary to the Go shadowing guideline.🤖 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.go` around lines 53 - 56, Rename the error variable returned by getRHELStreamForBootImage in the surrounding function from err to resolveErr, and update the immediately following error check to use resolveErr. Leave downstream err declarations and handling unchanged.Source: Coding guidelines
🤖 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.go`:
- Around line 364-367: Set the NodePoolValidPlatformImageType condition to false
before returning the stream-resolution error in the resolver error branches of
hypershift-operator/controllers/nodepool/aws.go:364-367,
hypershift-operator/controllers/nodepool/kubevirt.go:68-71, and
hypershift-operator/controllers/nodepool/openstack.go:53-56. Preserve the
existing wrapped error returns and match the adjacent image-discovery failure
handling.
In `@hypershift-operator/controllers/nodepool/openstack.go`:
- Around line 53-56: Update the error path after getRHELStreamForBootImage in
the OpenStack reconciliation flow to preserve the invalid-image status update,
setting NodePoolValidPlatformImageType=False before returning the wrapped error.
Keep the existing error context and ensure status-update failures are handled
consistently with the surrounding reconciliation logic.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/openstack.go`:
- Around line 53-56: Rename the error variable returned by
getRHELStreamForBootImage in the surrounding function from err to resolveErr,
and update the immediately following error check to use resolveErr. Leave
downstream err declarations and handling unchanged.
🪄 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: c8cbd061-1f36-4461-824a-b23dd16eb83b
📒 Files selected for processing (4)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/openstack.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9099 +/- ##
==========================================
+ Coverage 44.51% 44.59% +0.07%
==========================================
Files 774 774
Lines 96997 96997
==========================================
+ Hits 43179 43256 +77
+ Misses 50830 50746 -84
- Partials 2988 2995 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@jparrill shouldn't modify this The e2e-v2-gke and e2e-v2-aws run OCP5 |
sdminonne
left a comment
sdminonne
left a comment
There was a problem hiding this comment.
Duplicate getRHELStreamForBootImage call per reconcile
This PR introduces two calls to getRHELStreamForBootImage in the same reconcile loop:
setPlatformConditions→ e.g.setAWSConditions→getRHELStreamForBootImage(nodepool_controller.go:367)NewConfigGenerator→getRHELStreamForBootImage(nodepool_controller.go:380)
Before this PR there was no duplication — both sites used the hardcoded StreamRHEL9 constant. Now each call parses the release version, does client.Get for every ConfigMap in nodePool.Spec.Config to detect runc, then calls GetRHELStream.
It's not expensive per se, but it's unnecessary redundancy. Consider computing the resolved stream once and threading it through (e.g. passing it to NewConfigGenerator or storing it on the reconciler context), or at minimum noting it as a follow-up.
76adca8 to
ef073d3
Compare
…boot images Replace hardcoded StreamRHEL9 with dynamic resolution via getRHELStreamForBootImage across all platform controllers (AWS, OpenStack, KubeVirt) and the central ConfigGenerator. With MCO PR openshift/machine-config-operator#6308 merged and the OSStreams feature gate promoted to Default for Hypershift (openshift/api#2950), the MCO can now process OSImageStream during bootstrap. NodePools can resolve the correct RHEL stream (9 or 10) based on release version, explicit spec.osImageStream, and container runtime configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
ef073d3 to
3409314
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/aws_test.go`:
- Around line 1167-1184: Update the invalid osImageStream test case to set
expectedCondType and the corresponding expected condition reason/message for
ValidPlatformImage, so it verifies the image-invalid condition rather than only
expectError. Adjust setAWSConditions to emit that condition consistently when
the release image is incompatible with the requested stream.
🪄 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: 34a9184f-0551-44d1-97ff-8d43f2385eec
📒 Files selected for processing (5)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/openstack.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/controllers/nodepool/openstack.go
- hypershift-operator/controllers/nodepool/kubevirt.go
- hypershift-operator/controllers/nodepool/aws.go
- hypershift-operator/controllers/nodepool/config.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/kubevirt.go`:
- Around line 68-79: The KubeVirt reconciliation path currently resolves the
RHEL stream before determining whether it is needed. Update the flow around
getRHELStreamForBootImage and kubevirt.GetImage to skip stream resolution for
explicit KubeVirt container images, while preserving resolution for the
default-image path; ensure valid explicit images are not marked invalid when
resolution would fail. Add a regression test covering an explicit image with a
resolver failure.
🪄 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: 93871049-168c-4cff-8b28-fa26190818c6
📒 Files selected for processing (5)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/openstack.go
🚧 Files skipped from review as they are similar to previous changes (4)
- hypershift-operator/controllers/nodepool/openstack.go
- hypershift-operator/controllers/nodepool/aws_test.go
- hypershift-operator/controllers/nodepool/config.go
- hypershift-operator/controllers/nodepool/aws.go
openshift-ci
Bot
added
the
area/platform/powervs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/conditions.go`:
- Around line 387-390: When getRHELStreamForBootImage returns an error in the
condition-building flow, set NodePoolValidPlatformImageType to False before
returning the wrapped error so any previous True condition is cleared; add a
regression test covering this failure path and asserting the condition state.
In `@hypershift-operator/controllers/nodepool/config_test.go`:
- Around line 466-474: Update the successful test cases in the config generator
test around NewConfigGenerator to assert that cg.resolvedRHELStreamForBootImage
equals the resolvedStream value passed to the generator. Keep the assertion
focused on propagation and retain the existing hash-based checks.
🪄 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: 16dd596c-4fdc-4043-90e3-91b5faf68222
📒 Files selected for processing (10)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/secret_janitor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/nodepool/aws_test.go
c898dcb to
fa81589
Compare
openshift-ci
Bot
added
the
area/karpenter-operator
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 367-372: Update the reconciliation flow around
GetRHELStreamForBootImage and setPlatformConditions so stream-resolution
failures still invoke setPlatformConditions and preserve the
NodePoolValidPlatformImageType=False status update. Retain the resolution error
for the final reconciliation result while ensuring successful resolution
continues passing resolvedRHELStream to downstream generation.
🪄 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: 3f8809ff-f5b9-4c35-bcaf-e9b5e6f7e947
📒 Files selected for processing (13)
hypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/kubevirt.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/openstack.gohypershift-operator/controllers/nodepool/osstream.gohypershift-operator/controllers/nodepool/osstream_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/secret_janitor.gokarpenter-operator/controllers/karpenterignition/karpenterignition_controller.go
🚧 Files skipped from review as they are similar to previous changes (8)
- hypershift-operator/controllers/nodepool/kubevirt.go
- hypershift-operator/controllers/nodepool/powervs.go
- hypershift-operator/controllers/nodepool/secret_janitor.go
- hypershift-operator/controllers/nodepool/aws.go
- hypershift-operator/controllers/nodepool/conditions.go
- hypershift-operator/controllers/nodepool/config_test.go
- hypershift-operator/controllers/nodepool/aws_test.go
- hypershift-operator/controllers/nodepool/config.go
fa81589 to
6960551
Compare
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
Test Resultse2e-aws
Failed TestsTotal failed tests: 5
e2e-aks
|
|
/retest |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
|
/retest-required |
b7a1ff3 to
aa157d6
Compare
openshift-ci-robot
removed
the
verified
|
@jparrill: This pull request references CNTRLPLANE-3871 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. |
…icated TechPreview job - Add `lifecycle` label to osImageStream Status tests so they only run in the dedicated e2e-v2-aws-techpreview-osimagestream job where the hosted cluster has TechPreview (TODO: remove after FG graduation) - Move osImageStream post-upgrade verification from nodepool_lifecycle_test.go into nodepool_osimagestream_test.go as a dedicated upgrade test - Fix NodePoolOSImageStreamExplicitDefaultNoRolloutTest: create a dedicated NodePool instead of mutating the default one (osImageStream is immutable once set — CEL validation prevents removal) - Use version-aware stream expectations (rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0) - Remove all TODO(CNTRLPLANE-3553) and TODO(CNTRLPLANE-3032) comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
aa157d6 to
344c616
Compare
|
@jparrill: 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. |
|
Scheduling tests matching the |
|
/pipeline required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling tests matching the |
|
/verified by e2e passing |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
5 participants
Summary
StreamRHEL9with dynamic resolution viaGetRHELStreamForBootImageacross all platform controllers (AWS, OpenStack, KubeVirt, PowerVS) and the centralConfigGeneratorNewConfigGenerator, eliminating redundant calls fromtoken(),secretJanitor,conditions, andkarpenterGetRHELStreamForBootImagefor cross-package use (karpenter-operator)NodePoolValidPlatformImageType=Falseon stream resolution errors to prevent stale conditionsnodepool-osimagestreamlabel withlifecycleso they only run in the dedicatede2e-v2-aws-techpreview-osimagestreamjob (TODO: remove after FG graduation)nodepool_osimagestream_test.goNodePoolOSImageStreamExplicitDefaultNoRolloutTest: create dedicated NodePool instead of mutating default (osImageStream is immutable once set — CEL prevents removal)setPlatformConditions,setOpenStackConditions,setPowerVSconditions,setKubevirtConditions, andvalidMachineConfigConditionTODO([CNTRLPLANE-3553](https://redhat.atlassian.net/browse/CNTRLPLANE-3553))andTODO([CNTRLPLANE-3032](https://redhat.atlassian.net/browse/CNTRLPLANE-3032))commentsDependency chain
e2e-v2-aws-techpreview-osimagestreamjob (must merge first)lifecyclelabel, temporary CI job, and move tests back toe2e-v2-awsAlready merged
Fixes
🤖 Generated with Claude Code