Skip to content

CNTRLPLANE-3022: Add osImageStream to NodePool spec and status #8675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 2 commits into from
Jun 9, 2026
Merged

CNTRLPLANE-3022: Add osImageStream to NodePool spec and status #8675

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee12e05
feat(api): add osImageStream to NodePool spec and status
sdminonne Jun 4, 2026
e6eb131
fix(api): address review feedback on osImageStream CEL and serialization
sdminonne Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/hypershift/v1beta1/featuregates/featureGate-Hypershift-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
},
{
"name": "HCPEtcdBackup"
},
{
"name": "OSStreams"
}
],
"enabled": [
Expand Down
3 changes: 3 additions & 0 deletions api/hypershift/v1beta1/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
},
{
"name": "HCPEtcdBackup"
},
{
"name": "OSStreams"
}
],
"version": ""
Expand Down
3 changes: 3 additions & 0 deletions api/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
},
{
"name": "HCPEtcdBackup"
},
{
"name": "OSStreams"
}
],
"enabled": [
Expand Down
3 changes: 3 additions & 0 deletions api/hypershift/v1beta1/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
},
{
"name": "HCPEtcdBackup"
},
{
"name": "OSStreams"
}
],
"version": ""
Expand Down
37 changes: 37 additions & 0 deletions api/hypershift/v1beta1/nodepool_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,36 @@ type NodePoolSpec struct {
// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="Arch is immutable"
// +optional
Arch string `json:"arch,omitempty"`

// osImageStream specifies an OS stream to be used for nodes in this pool.

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@enxebre enxebre Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// This field can be optionally set to a known OSImageStream name to change
// the OS and Extension images with a well-known, tested, release-provided
// set of images. This enables a streamlined way of switching the pool's
// node OS to a different version than the cluster default, such as
// transitioning to a major RHEL version.
//
// When set, the referenced stream overrides the default OS images for the
// pool. When omitted, the pool uses the release version's default stream
// (rhel-9 for OCP < 5.0, rhel-10 for OCP >= 5.0).
// Changing this field triggers a rollout. Forward transitions
// (rhel-9 -> rhel-10) are allowed; backward transitions
// (rhel-10 -> rhel-9) are rejected by CEL validation because
// 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"

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Remove osImageStream entirely (passes — field is optional)
  2. 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.

@sdminonne sdminonne Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think removal should be blocked once set.

// +optional
OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"`
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}

// OSImageStreamReference references an OSImageStream by name.
type OSImageStreamReference struct {
// name is a required reference to an OSImageStream to be used for the pool.
//
// +required
// +kubebuilder:validation:Enum=rhel-9;rhel-10
Name string `json:"name,omitempty"`
}

// NodePoolStatus is the latest observed status of a NodePool.
Expand All @@ -263,6 +293,13 @@ type NodePoolStatus struct {
// +optional
Platform *NodePoolPlatformStatus `json:"platform,omitempty"`

// osImageStream reports the OS stream observed on the nodes in this pool.
//
// When omitted, the pool is using the release version's default OS images.
// +openshift:enable:FeatureGate=OSStreams
// +optional
OSImageStream OSImageStreamReference `json:"osImageStream,omitempty,omitzero"`

@enxebre enxebre Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use only omitzero, omitempty does nothing on structs

@sdminonne sdminonne Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks going to do.


// conditions represents the latest available observations of the node pool's
// current state.
// +kubebuilder:validation:MaxItems=100
Expand Down
17 changes: 17 additions & 0 deletions api/hypershift/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading