CNTRLPLANE-3022: Add osImageStream to NodePool spec and status#8675
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
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@sdminonne: This pull request references CNTRLPLANE-3022 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. |
|
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 ignored due to path filters (6)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds support for OS image stream selection in HyperShift node pools. The change introduces a new Sequence Diagram(s)sequenceDiagram
participant ClusterAdmin
participant FeatureGateConfig
participant NodePoolAPI
ClusterAdmin->>FeatureGateConfig: add/enable "OSStreams" entry (profiles)
ClusterAdmin->>NodePoolAPI: create/update NodePool with osImageStream
NodePoolAPI->>FeatureGateConfig: read "OSStreams" feature gate
Note right of NodePoolAPI: apply CEL validation (prevent rhel-10 -> rhel-9)
NodePoolAPI-->>ClusterAdmin: NodePoolStatus.osImageStream observed or defaulted
🚥 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/api
![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
🤖 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 `@api/hypershift/v1beta1/nodepool_types.go`:
- Around line 242-261: The CEL XValidation currently allows removing
osImageStream (self.name unset) to bypass the rhel-10 -> rhel-9 downgrade block;
update the validation expression on the OSImageStream field (OSImageStream in
nodepool_types.go) to require that when oldSelf.name == 'rhel-10' the new
self.name must exist and not equal 'rhel-9' (e.g. replace the rule with:
"!has(oldSelf.name) || oldSelf.name != 'rhel-10' || (has(self.name) && self.name
!= 'rhel-9')") so an unset self.name no longer permits a downgrade escape hatch.
🪄 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: 38e6fd98-eec7-43ca-b03a-e9f00daa8428
⛔ Files ignored due to path filters (19)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OSStreams.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/nodepoolspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/nodepoolstatus.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/osimagestreamreference.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-Hypershift-Default.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/featuregated.nodepools.osimagestream.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/nodepool_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*
📒 Files selected for processing (5)
api/hypershift/v1beta1/featuregates/featureGate-Hypershift-Default.yamlapi/hypershift/v1beta1/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlapi/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-Default.yamlapi/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlapi/hypershift/v1beta1/nodepool_types.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8675 +/- ##
==========================================
+ Coverage 41.43% 41.49% +0.05%
==========================================
Files 756 756
Lines 93647 93648 +1
==========================================
+ Hits 38802 38855 +53
+ Misses 52124 52057 -67
- Partials 2721 2736 +15 see 2 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:
|
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe failure is a stale-base false positive in Codecov's project-level coverage gate. The specific chain:
Recommendations
Evidence
|
Add OSImageStreamReference type and osImageStream fields to NodePoolSpec and NodePoolStatus, gated behind the OSStreams feature gate (TechPreview). This is Phase 0 of the Dual-Stream RHEL 9/10 NodePool enhancement, enabling per-pool OS stream selection in HyperShift. The spec field allows users to override the default OS images for a NodePool by referencing a known OSImageStream (rhel-9 or rhel-10). A field-level CEL validation rule prevents downgrades from rhel-10 to rhel-9. The status field reports the observed OS stream on nodes. Signed-off-by: Salvatore Dario Minonne <sminonne@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
sdminonne
force-pushed
the
CNTRLPLANE-3022/Dual-Stream-NodePool-API-add-osImageStream-to-spec-status
branch
from
0b89acd to
ee12e05
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| // When omitted, the pool is using the release version's default OS images. | ||
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +optional | ||
| OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"` |
There was a problem hiding this comment.
please use only omitzero, omitempty does nothing on structs
There was a problem hiding this comment.
Thanks going to do.
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!!
nit: I'd split the feature gate registration (OSStreams in the 4 featureGate YAMLs) into its own commit, separate from the new API type changes in nodepool_types.go. Easier to review and revert independently.
| // in-place OS downgrades are not supported. | ||
| // | ||
| // +openshift:enable:FeatureGate=OSStreams | ||
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.name) || oldSelf.name != 'rhel-10' || self.name != 'rhel-9'",message="OS stream downgrade from rhel-10 to rhel-9 is not allowed; create a new NodePool instead" |
There was a problem hiding this comment.
CEL downgrade rule can be bypassed via field removal
The current CEL rule prevents the direct rhel-10 → rhel-9 transition, which is great. But since the field is +optional, a user can bypass it in two steps:
- Remove
osImageStreamentirely (passes — field is optional) - Re-add it with
name: rhel-9(passes —!has(oldSelf.name)is true on the zero-value struct)
This is the "immutable + optional two-step bypass" pattern documented in api/AGENTS.md. The arch field handles this with a parent-level guard:
!has(oldSelf.arch) || has(self.arch)
Could you clarify the design intent? If removal should be blocked once set, we'd need something like this on NodePoolSpec:
+kubebuilder:validation:XValidation:rule="!has(oldSelf.osImageStream) || !has(oldSelf.osImageStream.name) || has(self.osImageStream)",message="osImageStream cannot be removed once set"
If removal is intentionally allowed (revert to release default), that's also a valid design — but it should be documented explicitly and tested.
There was a problem hiding this comment.
No I think removal should be blocked once set.
| // +optional | ||
| Arch string `json:"arch,omitempty"` | ||
|
|
||
| // osImageStream specifies an OS stream to be used for nodes in this pool. |
There was a problem hiding this comment.
Per the "Required Tests" section in api/AGENTS.md, API type changes should include serialization compatibility tests that verify N-1/N+1 round-trip behavior. Could you add a test in nodepool_types_test.go following the existing patterns (e.g., TestNodePoolAutoScalingSerializationCompatibility)? It should verify that a NodePoolSpec with OSImageStream set serializes correctly and deserializes into an N-1 struct without the field (and vice versa).
There was a problem hiding this comment.
there's no need to add this for a new optional field. Those tests are valuable when e.g. changing a filed type pointer <-> value
| id: "subnet-any" | ||
| type: AWS | ||
| osImageStream: | ||
| name: "rhel-10" |
There was a problem hiding this comment.
The test coverage looks solid for the core scenarios. One gap I'd suggest adding: a test for removing osImageStream from an existing NodePool that had it set. Something like:
- name: When removing osImageStream from a NodePool that had rhel-10 it should [succeed/fail]The expected outcome depends on the design intent (see my other comment about the CEL bypass), but either way the behavior should be explicitly tested and documented.
There was a problem hiding this comment.
I'll add a test case for this. Removal should be prevented once is set
- Remove omitempty from OSImageStream struct fields (spec and status), keeping only omitzero. omitempty has no effect on Go structs. - Tighten the field-level CEL downgrade rule to also require has(self.name) when oldSelf.name is rhel-10, closing an escape hatch where unsetting self.name could bypass the downgrade check. A parent-level CEL guard on NodePoolSpec to prevent osImageStream removal once set was considered but cannot be implemented via CEL. Because osImageStream is feature-gated (+openshift:enable:FeatureGate= OSStreams), it only exists in the TechPreview/CustomNoUpgrade CRD schema. A parent-level rule referencing osImageStream would be included in the Default CRD variant where the field does not exist, causing a CEL compilation error (undefined field) at CRD installation time. This is a fundamental constraint of the feature-gate CRD generation architecture: non-feature-gated CEL rules on a struct cannot reference feature-gated fields. The "once set, cannot be removed" constraint will need to be enforced at the controller level in a follow-up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Regarding the "once set, cannot be removed" guard for I investigated adding a parent-level CEL rule on This doesn't work because This is a fundamental constraint: non-feature-gated CEL rules on a parent struct cannot reference feature-gated child fields. The I've tightened the field-level CEL rule to close the |
enxebre
added
the
tide/merge-method-squash
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, sdminonne 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 |
openshift-ci
Bot
added
the
approved
|
/cc @cblecker @JoelSpeed for approval |
|
@sdminonne: GitHub didn't allow me to request PR reviews from the following users: for, approval. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by @sdminonne |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
|
new field is working |
Test Resultse2e-aws
e2e-aks
|
|
/uncc |
|
@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. |
7 participants
What this PR does / why we need it:
Phase 0 of the Dual-Stream RHEL 9/10 NodePool enhancement (OCPSTRAT-3014 / CNTRLPLANE-3022). Adds
osImageStreamfields to NodePool spec and status, gated behind theOSStreamsfeature gate (TechPreview).OSImageStreamReferencewithnamefield restricted torhel-9orrhel-10via Enum validationNodePoolSpec.osImageStream— allows users to override the default OS images for a NodePool by referencing a known OSImageStream. A field-level CEL rule prevents downgrades fromrhel-10torhel-9NodePoolStatus.osImageStream— reports the observed OS stream on nodes in the poolOSStreamsfeature gate registered in all 4 feature gate files (disabled in Default, enabled in TechPreview)Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3022
Special notes for your reviewer:
The CEL downgrade-prevention rule is placed at the field level (on
osImageStream) rather than the struct level (onNodePoolSpec). This ensures the rule is co-gated with the field itself and avoids CRD installation failures in the Default variant where the field doesn't exist in the schema.Checklist:
Test plan
make api— CRDs regenerated correctly (field present only in TechPreview/CustomNoUpgrade)make update— Full dependency sync passedmake verify— Linting and generation passedmake test-envtest-ocp— All envtests passed across K8s 1.30–1.35make test— All unit tests passedmake run-gitlint— Commit message validated🤖 Generated with Claude Code
Summary by CodeRabbit