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 |
| 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.
pablintino
force-pushed
the
mco-2181
branch
3 times, most recently
from
e984a41 to
7c3c1d0
Compare
|
/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-upgrade periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3 |
|
/retest-required |
|
I think the azure jobs are failing because we don't have the rhcos-10 marketplace stream populated. I have opened #10556 to populate it. It uses rhcos9 images because rhcos10 images have not been published yet. |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
Good to know. Thanks for taking care of that fix. |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
Ah but the test is triggered. |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
I can't make sense of the build failure in the previous testwith. I just rebased my other PR, so let's see if that helps: /testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
/testwith openshift/installer/main/e2e-azure-ovn #10556 |
|
If we set the install-config value in a single place as in #10570, then this could be as simple as zaneb@osimagestream-rhel10-default |
| defer cancel() | ||
|
|
||
| st, err := rhcos.FetchCoreOSBuild(ctx, rhcos.DefaultOSImageStream) | ||
| st, err := rhcos.FetchCoreOSBuild(ctx, types.OSImageStreamRHCOS9) |
There was a problem hiding this comment.
Is this just waiting for MCO-2200 to land?
Note that this affects not only ABI, but also the baremetal IPI bootstrap VM.
There was a problem hiding this comment.
Is this just waiting for MCO-2200 to land?
Note that this affects not only ABI, but also the baremetal IPI bootstrap VM.
Yes, and it's something I knew and identified.
We have been already booting with RHEL 9 and pivoting to RHEL 10 during the MCD firstboot so I don't think having this for a short period of time (if the ABI change doesn't land before) is problematic.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
It occurred to me that in pkg/types/validation/featuregates.go:
Condition: c.OSImageStream != rhcos.DefaultOSImageStream,
is not going to do the Right Thing on 4.23, where rhcos.DefaultOSImageStream will be rhel-10, but the actual default in the install-config will be rhel-9, and it is setting it to rhel-10 that requires the dual-stream gate to be enabled.
| return fmt.Errorf("invalid value %q for --stream; must be one of %v", streamFlag, types.OSImageStreamValues) | ||
| } | ||
| osImageStream = s | ||
| } else if v, err := version.GetVersionInfo(); err == nil && v.Major < 5 { |
There was a problem hiding this comment.
As mentioned on Slack, I would be +1 on having a separate feature gate for rhel-10-by-default, and basing this decision on the feature gate.
That way we would have only one place to look at the binary version. (The feature gate code already needs to - but does not yet - look at the version.)
There was a problem hiding this comment.
I've opened a PR for the RHEL10 featuregate here: openshift/api#2860
There was a problem hiding this comment.
Yes, I have an eye on that. I'll incorporate it as soon as it lands.
| ) | ||
|
|
||
| // VersionInfo represents a parsed semantic version. | ||
| type VersionInfo struct { |
There was a problem hiding this comment.
suggestion (non-blocking): This repo has the following semver libraries already vendored which could be a more robust implementation:
There was a problem hiding this comment.
I known, and I've seen them. Unfortunately, some of them are strict with semver format and requires the version to start with v, others don't. Given that not only the Makefile is in charge of injecting the version, I prefer to use this hand-crafted methods that at least is safe and should do the job to get major, minor and patch fields.
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. |
6 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