MCO-2181: Default version switchover#10533
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@pablintino: This pull request references MCO-2181 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
do-not-merge/work-in-progress
WalkthroughCentralizes RHCOS OS image stream selection into new rhcos helpers and updates callsites to use the effective/default stream (instead of reading raw config.OSImageStream or rhcos.DefaultOSImageStream), plus small related filename/error handling and call-signature adjustments. ChangesOS Image Stream Centralization
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Installer
participant rhcos
participant Assets
participant Cloud
CLI->>Installer: invoke (e.g., coreoscli / install)
Installer->>rhcos: GetDefaultStreamForInstallConfig(nil) or GetEffectiveOSImageStream(ic)
rhcos-->>Installer: effective OS image stream
Installer->>Assets: FetchCoreOSBuild(stream)
Assets-->>Installer: build metadata / AMI regions
Installer->>Cloud: validate/create images/resources using stream-derived artifact info
Cloud-->>Installer: infra responses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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
🤖 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 `@pkg/rhcos/builds.go`:
- Around line 119-123: Update the SCOS build variant functions getStreamFileName
and getMarketplaceStreamFileName to match the main signatures by returning
(string, error) instead of just string; modify their definitions to return the
filename string and a nil error (e.g., return "<filename>", nil) so callers like
the code in builds.go that expect (string, error) compile correctly.
In `@pkg/rhcos/stream.go`:
- Around line 55-63: The function GetDefaultStreamForInstallConfig currently
falls back to getDefaultOSImageStreamForBuildVersion only when ic is nil; change
it to base the fallback on whether the install-config actually configures the
OSStreams feature gate. Update GetDefaultStreamForInstallConfig to: if ic is nil
OR ic.FeatureGate is nil (i.e. the gate is not present) then return
getDefaultOSImageStreamForBuildVersion(), otherwise use
ic.Enabled(features.FeatureGateOSStreams) to choose between
types.OSImageStreamRHCOS10 and types.OSImageStreamRHCOS9. Reference:
GetDefaultStreamForInstallConfig, ic.FeatureGate, ic.Enabled,
features.FeatureGateOSStreams, getDefaultOSImageStreamForBuildVersion.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8aa1aa2e-c457-4716-9eaf-6e98ecfba95d
📒 Files selected for processing (12)
pkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/manifests/osimagestream.gopkg/asset/rhcos/image.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
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 `@pkg/asset/imagebased/image/baseiso.go`:
- Around line 29-31: The function defaultCoreOSStreamGetter currently hardcodes
types.OSImageStreamRHCOS9 when calling rhcos.FetchCoreOSBuild; replace that
hardcoded stream with the version-driven default stream determination used
elsewhere in the PR (i.e., call the project's helper that computes the default
stream based on build/version metadata and use its result as the stream argument
to rhcos.FetchCoreOSBuild), and only fall back to types.OSImageStreamRHCOS9 if
that helper yields no result; update defaultCoreOSStreamGetter to call that
helper, pass its output into rhcos.FetchCoreOSBuild, and preserve existing error
propagation.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 606a94a6-2a53-441a-92d4-c6cb99f7a4d1
📒 Files selected for processing (15)
pkg/asset/ignition/bootstrap/common.gopkg/asset/imagebased/image/baseiso.gopkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/manifests/osimagestream.gopkg/asset/rhcos/image.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.go
✅ Files skipped from review due to trivial changes (1)
- pkg/asset/rhcos/iso.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/infrastructure/azure/azure.go
- pkg/asset/rhcos/release.go
- pkg/asset/installconfig/aws/platform.go
- pkg/rhcos/builds.go
- pkg/asset/installconfig/vsphere/validation.go
- pkg/asset/installconfig/nutanix/validation.go
- pkg/rhcos/stream.go
- pkg/rhcos/stream_scos.go
| func defaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) { | ||
| return rhcos.FetchCoreOSBuild(ctx, rhcos.DefaultOSImageStream) | ||
| // TODO @pablintino: Pending removal of the getter concept | ||
| return rhcos.FetchCoreOSBuild(ctx, types.OSImageStreamRHCOS9) |
There was a problem hiding this comment.
Avoid hard-coding the default stream to RHCOS9.
Line 31 forces types.OSImageStreamRHCOS9, which bypasses the version-driven default stream switchover logic described in this PR and can fetch the wrong base ISO metadata on newer builds.
Suggested fix
func defaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) {
// TODO `@pablintino`: Pending removal of the getter concept
- return rhcos.FetchCoreOSBuild(ctx, types.OSImageStreamRHCOS9)
+ return rhcos.FetchCoreOSBuild(ctx, rhcos.GetDefaultStreamForInstallConfig(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.
| func defaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) { | |
| return rhcos.FetchCoreOSBuild(ctx, rhcos.DefaultOSImageStream) | |
| // TODO @pablintino: Pending removal of the getter concept | |
| return rhcos.FetchCoreOSBuild(ctx, types.OSImageStreamRHCOS9) | |
| func defaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) { | |
| // TODO `@pablintino`: Pending removal of the getter concept | |
| return rhcos.FetchCoreOSBuild(ctx, rhcos.GetDefaultStreamForInstallConfig(nil)) | |
| } |
🤖 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 `@pkg/asset/imagebased/image/baseiso.go` around lines 29 - 31, The function
defaultCoreOSStreamGetter currently hardcodes types.OSImageStreamRHCOS9 when
calling rhcos.FetchCoreOSBuild; replace that hardcoded stream with the
version-driven default stream determination used elsewhere in the PR (i.e., call
the project's helper that computes the default stream based on build/version
metadata and use its result as the stream argument to rhcos.FetchCoreOSBuild),
and only fall back to types.OSImageStreamRHCOS9 if that helper yields no result;
update defaultCoreOSStreamGetter to call that helper, pass its output into
rhcos.FetchCoreOSBuild, and preserve existing error propagation.
There was a problem hiding this comment.
This is known. The code you are referencing is going to be removed later by #10537.
That said, picking rhel 9 in the meantime is a safe solution. If the user picks RHEL 10 the node will pivot later, like it's doing now.
There was a problem hiding this comment.
@pablintino, thanks for the context! That makes sense — since defaultCoreOSStreamGetter is slated for removal in #10537 and RHCOS9 serves as a safe interim default (with RHEL 10 nodes pivoting as they do today), keeping the hardcoded value is a reasonable short-term approach. The existing TODO comment in the code also makes the transient nature clear. I'll withdraw the concern.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
pablintino
force-pushed
the
mco-2181
branch
2 times, most recently
from
00da3d4 to
f57f60f
Compare
|
/test gofmt golint images shellcheck |
This change makes the installer pick the proper bootimages, ISOs and OS Image Stream depending on FeatureGate presence or build version if there's no FeatureGate at hand. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@pablintino: The following tests failed, say
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. |
| if err != nil { | ||
| return err | ||
| } | ||
| var osImageStream types.OSImageStream |
There was a problem hiding this comment.
Nit: osImageStream := rhcos.GetDefaultStreamForInstallConfig(nil)
| return fmt.Errorf("invalid value %q for --stream; must be one of %v", streamFlag, types.OSImageStreamValues) | ||
| } | ||
| osImageStream = s | ||
| } else { |
There was a problem hiding this comment.
Nit: we won't need this if we initialize osImageStream to rhcos.GetDefaultStreamForInstallConfig(nil) and overwrite it only when streamFlag != "".
| file, err := data.Assets.Open(getStreamFileName(osImageStream)) | ||
| jsonFile, err := getStreamFileName(osImageStream) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Nit:
| return nil, err | |
| return nil, fmt.Errorf("failed to get CoreOS stream file name: %w", err) |
| } | ||
| return types.OSImageStreamRHCOS10 | ||
| } | ||
| return getDefaultOSImageStreamForBuildVersion() |
There was a problem hiding this comment.
When FeatureGate is enabled, default stream is types.OSImageStreamRHCOS10. For every other case, I would think we would want to return getDefaultOSImageStreamForBuildVersion().
| if versionNumber >= 5 { | ||
| return types.OSImageStreamRHCOS10 | ||
| } | ||
| return types.OSImageStreamRHCOS9 |
There was a problem hiding this comment.
I am unclear why the OS stream value here is different from the start of the method where we are not able to determine versionString and return types.OSImageStreamRHCOS10.
| } | ||
|
|
||
| func getDefaultOSImageStreamForBuildVersion() types.OSImageStream { | ||
| versionString, err := version.Version() |
There was a problem hiding this comment.
BUILD_VERSION is not being set in presubmits, so the version we're getting is being derived from the git tags, which is not what we want:
time="2026-05-08T11:43:29Z" level=debug msg="OpenShift Installer v1.4.22-ec5-76-ga2f36db268f29ead7cd3ffe3b09b425d8ed0b297-dirty"
I think we need to get BUILD_VERSION set before we can proceed with this PR. I'm floating openshift/release#79061 to see if that would be a quick way of achieving this. Otherwise, we probably need to talk to ART or test platform team.
4 participants
This change makes the installer pick the proper bootimages, ISOs and OS Image Stream depending on FeatureGate presence or build version if there's no FeatureGate at hand.
Summary by CodeRabbit
Bug Fixes
Enhancements