DNM: CNTRLPLANE-3612: Add dual-stream RHEL support in HCP#8734
DNM: CNTRLPLANE-3612: Add dual-stream RHEL support in HCP#8734hypershift-jira-solve-ci[bot] wants to merge 9 commits into
Conversation
…stream support Implements the core getRHELStream() function that resolves the RHEL OS stream based on the user's explicit spec.osImageStream.name selection, the release version, and whether the NodePool configuration uses runc. Resolution rules: - Explicit rhel-10 on pre-5.0 releases returns an error - Explicit rhel-10 with runc config returns an error (RHEL 10 drops runc) - No explicit stream + pre-5.0 returns empty (legacy behavior) - No explicit stream + >=5.0 defaults to rhel-10 - No explicit stream + >=5.0 + runc falls back to rhel-9 Also adds usesRunc detection to ConfigGenerator by inspecting ContainerRuntimeConfig manifests during config parsing, and adds rhelStream to rolloutConfig for config hash computation (populated from spec only, not resolved value, to prevent fleet-wide rollouts when the default stream changes across releases). CNTRLPLANE-3612 Co-Authored-By: Claude Opus 4.6 <[email protected]>
…vider Adds osStream field to Token struct, resolved via getRHELStream() during NewToken(). The resolved stream is stored in the token secret under a new "os-stream" data key so the ignition server can use it during payload generation. Updates the IgnitionProvider.GetPayload() interface to accept an osStream parameter. The TokenSecretReconciler reads the os-stream key from the token secret and passes it through to the ignition provider. This is the plumbing layer that connects the NodePool controller's stream resolution to the ignition server's payload generation pipeline. CNTRLPLANE-3612 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Writes a 99_osimagestream.yaml manifest into the MCC input directory when the osStream parameter is non-empty. This manifest contains an OSImageStream CR with the resolved defaultStream, which MCC bootstrap uses to discover available OS streams via OCI label inspection and render MachineConfigs with the correct osImageURL. The CR is written at the end of runMCO (after copyMCOOutputToMCC) so it is available when runMCC processes the MCC input directory. When osStream is empty (pre-5.0 legacy behavior), no CR is written. CNTRLPLANE-3612 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Extends validMachineConfigCondition to validate the OS stream selection before proceeding with token creation. This catches invalid configurations early (e.g., rhel-10 on pre-5.0 releases, runc + rhel-10) and surfaces them as NodePool conditions. Adds OS stream inference from CAPI Machine NodeInfo: RHCOS version strings starting with "4" indicate RHEL 9, while versions starting with "5" indicate RHEL 10. The inferred stream is written to status.osImageStream to give users visibility into which RHEL version their nodes are actually running. CNTRLPLANE-3612 Co-Authored-By: Claude Opus 4.6 <[email protected]>
Covers: - getRHELStream(): all resolution paths including defaults, explicit selections, runc fallback, and error cases - inferStreamFromOSImage(): RHCOS version string parsing for RHEL 9/10 - inferOSStreamFromMachines(): aggregation across multiple CAPI Machines - writeOSImageStreamCR(): OSImageStream manifest generation - fakeIgnitionProvider: updated to match new GetPayload signature CNTRLPLANE-3612 Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add rhelStream to HashWithoutVersion() so pure OS stream changes
trigger config update detection and CAPI reconciliation
- Fix %q to %s in writeOSImageStreamCR() to produce idiomatic YAML
- Deduplicate TokenSecretOSStreamKey constant (define in ignserver only)
- Make error handling consistent in validMachineConfigCondition (both
version parse and stream validation now return Result{}, nil)
- Surface implicit rhel-9 fallback as a status condition message
- Anchor inferStreamFromOSImage() pattern to require dot-separated
version format, reducing false-positive risk
- Fix tie-break doc comment to say "plurality" instead of "majority"
- Fix inconsistent NewGomegaWithT usage in ignition-server tests
- Add tests: rhelStream in Hash/HashWithoutVersion, usesRunc detection,
TokenSecretOSStreamKey in token secret, empty-string no-op for
writeOSImageStreamCR, mixed-stream tie-break for inferOSStreamFromMachines
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-3612 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 (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements dual-stream RHEL OS support (RHEL 9/10) for HyperShift NodePools. A new Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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
the
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] 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 |
openshift-ci
Bot
added
area/hypershift-operator
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8734 +/- ##
==========================================
+ Coverage 41.66% 41.69% +0.03%
==========================================
Files 758 759 +1
Lines 93929 94083 +154
==========================================
+ Hits 39135 39232 +97
- Misses 52046 52096 +50
- Partials 2748 2755 +7
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
hypershift-operator/controllers/nodepool/version_test.go (2)
329-365: 💤 Low valueUse Gomega for test assertions to maintain consistency.
This test uses
t.Errorffor assertions, while the rest of the file uses Gomega (g.Expect). For consistency and better error messages, consider switching to Gomega:♻️ Suggested refactor
func TestInferStreamFromOSImage(t *testing.T) { + g := NewWithT(t) + tests := []struct { name string osImage string want string }{ // ... test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := inferStreamFromOSImage(tt.osImage) - if got != tt.want { - t.Errorf("inferStreamFromOSImage(%q) = %v, want %v", tt.osImage, got, tt.want) - } + g.Expect(got).To(Equal(tt.want)) }) } }🤖 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/version_test.go` around lines 329 - 365, The TestInferStreamFromOSImage function uses manual t.Errorf assertions instead of Gomega for consistency with the rest of the test file. Replace the if statement that checks if got != tt.want with a Gomega g.Expect assertion to match the testing style used elsewhere in the file. The assertion should compare the result of inferStreamFromOSImage(tt.osImage) against tt.want using Gomega's assertion methods.
367-427: 💤 Low valueUse Gomega for test assertions to maintain consistency.
This test uses
t.Errorffor assertions, while the rest of the file uses Gomega (g.Expect). For consistency and better error messages, consider switching to Gomega:♻️ Suggested refactor
func TestInferOSStreamFromMachines(t *testing.T) { + g := NewWithT(t) + tests := []struct { name string machines []*v1beta1.Machine want string }{ // ... test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := inferOSStreamFromMachines(tt.machines) - if got != tt.want { - t.Errorf("inferOSStreamFromMachines() = %v, want %v", got, tt.want) - } + g.Expect(got).To(Equal(tt.want)) }) } }🤖 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/version_test.go` around lines 367 - 427, The TestInferOSStreamFromMachines function currently uses t.Errorf for assertions instead of Gomeka assertions like the rest of the file. Replace the t.Errorf assertion inside the test's inner function with a Gomega assertion using g.Expect and To(Equal()) pattern to maintain consistency with the rest of the test file and provide better error messages when assertions fail.ignition-server/controllers/local_ignitionprovider.go (1)
797-819: ⚡ Quick winConsider validating
osStreambefore YAML interpolation.The
osStreamvalue is directly interpolated into the YAML string without validation. While in practice it's constrained to constants like"rhel-9"or"rhel-10"from upstreamgetRHELStream(), adding defensive validation would make the code more robust against future changes:🛡️ Suggested validation
func writeOSImageStreamCR(mccDir, osStream string) error { if osStream == "" { return nil } + // Validate osStream to prevent YAML injection + if osStream != "rhel-9" && osStream != "rhel-10" { + return fmt.Errorf("invalid osStream value: %q (expected rhel-9 or rhel-10)", osStream) + } + osImageStreamCR := fmt.Sprintf(`apiVersion: machineconfiguration.openshift.io/v1alpha1 kind: OSImageStream metadata: name: cluster spec: defaultStream: %s `, osStream)🤖 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 `@ignition-server/controllers/local_ignitionprovider.go` around lines 797 - 819, The writeOSImageStreamCR function directly interpolates the osStream parameter into YAML without validating its contents, which could be vulnerable if the function is called with unexpected values in the future. Add defensive validation before the fmt.Sprintf call to ensure osStream contains only expected characters (such as alphanumeric characters, hyphens, and underscores). If validation fails, return an error with a descriptive message indicating that the osStream value is invalid. This makes the code more robust against future changes while maintaining the current behavior for valid inputs like "rhel-9" and "rhel-10".
🤖 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/token.go`:
- Around line 120-129: Add a nil check for configGenerator.releaseImage before
calling the Version() method on it in the NewToken function. If
configGenerator.releaseImage is nil, return an error with a descriptive message
(similar to the existing error handling pattern used for parseErr) rather than
allowing a panic to occur when dereferencing the nil pointer.
In `@hypershift-operator/controllers/nodepool/version.go`:
- Around line 102-107: The OSImageStream status field is only updated when
inferOSStreamFromMachines returns a non-empty stream, causing stale data to
persist when the function returns an empty string. Remove the conditional check
and always set nodePool.Status.OSImageStream to ensure the status accurately
reflects the current inferred stream state, whether it contains a valid stream
name or is empty.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/version_test.go`:
- Around line 329-365: The TestInferStreamFromOSImage function uses manual
t.Errorf assertions instead of Gomega for consistency with the rest of the test
file. Replace the if statement that checks if got != tt.want with a Gomega
g.Expect assertion to match the testing style used elsewhere in the file. The
assertion should compare the result of inferStreamFromOSImage(tt.osImage)
against tt.want using Gomega's assertion methods.
- Around line 367-427: The TestInferOSStreamFromMachines function currently uses
t.Errorf for assertions instead of Gomeka assertions like the rest of the file.
Replace the t.Errorf assertion inside the test's inner function with a Gomega
assertion using g.Expect and To(Equal()) pattern to maintain consistency with
the rest of the test file and provide better error messages when assertions
fail.
In `@ignition-server/controllers/local_ignitionprovider.go`:
- Around line 797-819: The writeOSImageStreamCR function directly interpolates
the osStream parameter into YAML without validating its contents, which could be
vulnerable if the function is called with unexpected values in the future. Add
defensive validation before the fmt.Sprintf call to ensure osStream contains
only expected characters (such as alphanumeric characters, hyphens, and
underscores). If validation fails, return an error with a descriptive message
indicating that the osStream value is invalid. This makes the code more robust
against future changes while maintaining the current behavior for valid inputs
like "rhel-9" and "rhel-10".
🪄 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: 37db3fde-e3f9-426c-94f0-818e787d8440
📒 Files selected for processing (14)
hypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/rhel_stream.gohypershift-operator/controllers/nodepool/rhel_stream_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.goignition-server/cmd/run_local_ignitionprovider.goignition-server/controllers/local_ignitionprovider.goignition-server/controllers/local_ignitionprovider_test.goignition-server/controllers/tokensecret_controller.goignition-server/controllers/tokensecret_controller_test.go
| // Resolve OS stream based on spec, release version, and runc usage. | ||
| var osStream string | ||
| releaseVersion, parseErr := semver.Parse(configGenerator.releaseImage.Version()) | ||
| if parseErr != nil { | ||
| return nil, fmt.Errorf("failed to parse release version for OS stream resolution: %w", parseErr) | ||
| } | ||
| osStream, err = getRHELStream(configGenerator.nodePool.Spec.OSImageStream.Name, releaseVersion, configGenerator.usesRunc) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resolve OS stream: %w", err) | ||
| } |
There was a problem hiding this comment.
Guard NewToken against nil embedded rollout data before resolving OS stream.
NewToken dereferences configGenerator.releaseImage.Version() unconditionally. Because ConfigGenerator embeds *rolloutConfig, a nil rolloutConfig (or nil releaseImage) will panic instead of returning a validation error.
🐛 Proposed fix
func NewToken(ctx context.Context, configGenerator *ConfigGenerator, cpoCapabilities *CPOCapabilities) (*Token, error) {
if configGenerator == nil {
return nil, fmt.Errorf("configGenerator can't be nil")
}
+ if configGenerator.rolloutConfig == nil || configGenerator.releaseImage == nil {
+ return nil, fmt.Errorf("configGenerator release image can't be nil")
+ }
+ if configGenerator.nodePool == nil {
+ return nil, fmt.Errorf("configGenerator nodePool can't be nil")
+ }
if cpoCapabilities == nil {
return nil, fmt.Errorf("cpoCapabilities can't be nil")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Resolve OS stream based on spec, release version, and runc usage. | |
| var osStream string | |
| releaseVersion, parseErr := semver.Parse(configGenerator.releaseImage.Version()) | |
| if parseErr != nil { | |
| return nil, fmt.Errorf("failed to parse release version for OS stream resolution: %w", parseErr) | |
| } | |
| osStream, err = getRHELStream(configGenerator.nodePool.Spec.OSImageStream.Name, releaseVersion, configGenerator.usesRunc) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to resolve OS stream: %w", err) | |
| } | |
| func NewToken(ctx context.Context, configGenerator *ConfigGenerator, cpoCapabilities *CPOCapabilities) (*Token, error) { | |
| if configGenerator == nil { | |
| return nil, fmt.Errorf("configGenerator can't be nil") | |
| } | |
| if configGenerator.rolloutConfig == nil || configGenerator.releaseImage == nil { | |
| return nil, fmt.Errorf("configGenerator release image can't be nil") | |
| } | |
| if configGenerator.nodePool == nil { | |
| return nil, fmt.Errorf("configGenerator nodePool can't be nil") | |
| } | |
| if cpoCapabilities == nil { | |
| return nil, fmt.Errorf("cpoCapabilities can't be nil") | |
| } | |
| // Resolve OS stream based on spec, release version, and runc usage. | |
| var osStream string | |
| releaseVersion, parseErr := semver.Parse(configGenerator.releaseImage.Version()) | |
| if parseErr != nil { | |
| return nil, fmt.Errorf("failed to parse release version for OS stream resolution: %w", parseErr) | |
| } | |
| osStream, err = getRHELStream(configGenerator.nodePool.Spec.OSImageStream.Name, releaseVersion, configGenerator.usesRunc) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to resolve OS stream: %w", err) | |
| } |
🤖 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/token.go` around lines 120 - 129,
Add a nil check for configGenerator.releaseImage before calling the Version()
method on it in the NewToken function. If configGenerator.releaseImage is nil,
return an error with a descriptive message (similar to the existing error
handling pattern used for parseErr) rather than allowing a panic to occur when
dereferencing the nil pointer.
There was a problem hiding this comment.
The concern is valid in principle, but these nil cases are already guarded by NewConfigGenerator — the only constructor for ConfigGenerator:
releaseImage != nilis validated at config.go:80-82rolloutConfigis always initialized as part of the struct (it embedsreleaseImage), so it cannot be nil whenNewConfigGeneratorsucceedsnodePoolis a required constructor parameter and all three call sites (nodepool_controller.go:371,nodepool_controller.go:465,secret_janitor.go:122) provide a non-nil value obtained from the reconcile loop
Adding redundant nil checks in NewToken would duplicate existing validation without increasing safety, since NewToken is not called with a manually-constructed ConfigGenerator outside of tests (and tests already set these fields). Skipping this change to keep the validation in one place.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // Infer OS stream from node OSImage and set status.osImageStream. | ||
| if stream := inferOSStreamFromMachines(machines); stream != "" { | ||
| nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{ | ||
| Name: stream, | ||
| } | ||
| } |
There was a problem hiding this comment.
Status field is not cleared when OS stream inference returns empty, potentially leaving stale data.
When inferOSStreamFromMachines returns an empty string (e.g., no machines with NodeInfo, or all machines scaled to zero), the code skips updating nodePool.Status.OSImageStream. This leaves the previous value in place even though it no longer reflects the current machine state.
Consider always setting the field to ensure status accurately reflects the current inferred stream (or lack thereof):
- // Infer OS stream from node OSImage and set status.osImageStream.
- if stream := inferOSStreamFromMachines(machines); stream != "" {
- nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{
- Name: stream,
- }
- }
+ // Infer OS stream from node OSImage and set status.osImageStream.
+ stream := inferOSStreamFromMachines(machines)
+ nodePool.Status.OSImageStream = hyperv1.OSImageStreamReference{
+ Name: stream,
+ }🤖 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/version.go` around lines 102 - 107,
The OSImageStream status field is only updated when inferOSStreamFromMachines
returns a non-empty stream, causing stale data to persist when the function
returns an empty string. Remove the conditional check and always set
nodePool.Status.OSImageStream to ensure the status accurately reflects the
current inferred stream state, whether it contains a valid stream name or is
empty.
There was a problem hiding this comment.
The current behavior is intentional. Preserving the last-known OS stream during transient states is the correct design choice here:
- During initial provisioning: Before machines register
NodeInfo,inferOSStreamFromMachinesreturns empty. Clearing the status would remove useful observability data that was already set from the spec-driven resolution path. - During scale-to-zero: When all machines are scaled down, there's no NodeInfo to inspect. Retaining the last observed stream is more useful for operators than showing an empty field.
- The status field documents what was observed: The
status.osImageStreamcomment says "reports the OS stream observed on the nodes" — when no observation is possible, preserving the last known value is consistent with how other Kubernetes status fields behave (e.g.,status.versionisn't cleared during rollouts).
An empty inferOSStreamFromMachines result means "no data available," not "the stream changed to nothing." Unconditionally setting it would introduce misleading status transitions.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
enxebre
changed the title
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
add a new section in the automated-machine-management docs capturing the UX for the different dualstream permutations captured in the enhacement |
|
Confirmed. The base branch (before this PR) did not access Now I have everything needed for the report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe PR adds OS stream resolution logic to Root CauseThe PR inserts new code at // Resolve OS stream based on spec, release version, and runc usage.
var osStream string
releaseVersion, parseErr := semver.Parse(configGenerator.releaseImage.Version()) // LINE 122The type ConfigGenerator struct {
client.Client
hostedCluster *hyperv1.HostedCluster
nodePool *hyperv1.NodePool
controlplaneNamespace string
usesRunc bool
*rolloutConfig // <-- embedded pointer, nil by default
}And type rolloutConfig struct {
releaseImage *releaseinfo.ReleaseImage
// ... other fields
}The test at configGenerator: &ConfigGenerator{
hostedCluster: &hyperv1.HostedCluster{...},
nodePool: &hyperv1.NodePool{},
controlplaneNamespace: controlplaneNamespace,
},Before this PR, The fix requires updating the test to include a configGenerator: &ConfigGenerator{
hostedCluster: &hyperv1.HostedCluster{...},
nodePool: &hyperv1.NodePool{},
controlplaneNamespace: controlplaneNamespace,
rolloutConfig: &rolloutConfig{
releaseImage: &releaseinfo.ReleaseImage{
ImageStream: &imageapi.ImageStream{
ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"},
},
},
},
},Recommendations
Evidence
|
Document the UX for all dual-stream RHEL permutations including the OS stream resolution matrix, stream transitions, runc compatibility, observability via status fields and conditions, and common upgrade scenarios. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Re: #8734 (comment) Done. Added
Also added the page to the mkdocs nav under "Automated Machine Management". AI-assisted response via Claude Code |
…stTokenReconcile The dual-stream RHEL support added OS stream resolution to NewToken() which requires configGenerator.releaseImage to be set. Three TestNewToken cases (success, missing ignition endpoint, and missing CA cert) reached the new code path without a rolloutConfig, causing a nil pointer dereference on configGenerator.releaseImage.Version(). Additionally, TestTokenReconcile used ImageStream Name "4.17" which is not valid semver — semver.Parse requires Major.Minor.Patch format. Fix by adding rolloutConfig with a valid releaseImage to the affected TestNewToken cases and updating TestTokenReconcile to use "4.17.0". Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
openshift-ci
Bot
added
the
area/documentation
|
@hypershift-jira-solve-ci[bot]: 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. |
|
/uncc |
3 participants
What this PR does / why we need it:
Adds dual-stream RHEL (9/10) support to the NodePool and ignition server pipelines so that HCP clusters running OCP 5.0+ can resolve, validate, and propagate the correct OS stream for worker nodes.
Key changes:
getRHELStream): resolves the RHEL stream from the user's explicitspec.osImageStream.name, the release version, and whether the NodePool uses runc. Defaults to rhel-10 on OCP ≥5.0; falls back to rhel-9 when runc is configured (RHEL 10 drops runc support). Pre-5.0 releases retain legacy behavior (empty stream).ConfigGeneratorinspectsContainerRuntimeConfigmanifests during config parsing to detect runc usage, informing stream resolution.osStreamis stored in the token secret under a newos-streamdata key.TokenSecretReconcilerreads it and passes it to the ignition provider via an updatedGetPayload()interface.99_osimagestream.yamlmanifest (anOSImageStreamCR) into the MCC input directory whenosStreamis non-empty, enabling MCC bootstrap to discover available OS streams via OCI label inspection.validMachineConfigConditionvalidates the stream selection early (e.g., rhel-10 on pre-5.0, runc + rhel-10) and surfaces errors as NodePool conditions. OS stream inference from CAPI Machine NodeInfo writes tostatus.osImageStreamfor observability.rhelStreamis included inHashWithoutVersion()so stream-only changes trigger config update detection and CAPI reconciliation.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3612
Special notes for your reviewer:
rhelStreamfield inrolloutConfiguses the spec value (not the resolved value) to prevent fleet-wide rollouts when the default stream changes across releases.inferStreamFromOSImage()pattern is anchored to require dot-separated version format to reduce false-positive risk.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-3612](https://redhat.atlassian.net/browse/CNTRLPLANE-3612)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation