Skip to content

MCO-1961: Allow multiple machine-os versions#2157

Open
sdodson wants to merge 1 commit intoopenshift:mainfrom
sdodson:main
Open

MCO-1961: Allow multiple machine-os versions#2157
sdodson wants to merge 1 commit intoopenshift:mainfrom
sdodson:main

Conversation

@sdodson
Copy link
Member

@sdodson sdodson commented Dec 1, 2025

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 1, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link

coderabbitai bot commented Dec 1, 2025

Walkthrough

Modified the version merging logic in loadImageStreamTransforms to allow the "machine-os" component to have multiple versions across tags, while all other components maintain the strict single-version requirement. Error handling for non-machine-os components and error messaging remain unchanged.

Changes

Cohort / File(s) Summary
Version merging exception for machine-os
pkg/cli/admin/release/image_mapper.go
Modified loadImageStreamTransforms to bypass the single-version check specifically for "machine-os" component, allowing multiple versions across tags. Other components retain strict version checking. Error paths preserved with clarifying comment added.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify the logic correctly identifies and exempts "machine-os" component while preserving checks for other components
  • Confirm error messaging and error paths for non-machine-os components remain functionally correct
  • Validate the justification for the "machine-os" exception and its scope
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 345800d and 0ef0058.

📒 Files selected for processing (1)
  • pkg/cli/admin/release/image_mapper.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • pkg/cli/admin/release/image_mapper.go
🔇 Additional comments (1)
pkg/cli/admin/release/image_mapper.go (1)

201-202: The exception for machine-os is intentional and correct. Machine-os versions are allowed to differ across tags to support dual stream efforts, and machine-os is never referenced in manifests for version substitution (only for component lookups), so the conflict detection in NewComponentVersionsMapper is not triggered for this component. The logic is consistent with actual usage patterns.

Regarding extracting "machine-os" to a constant: while this would improve code clarity and reduce magic strings, it's a minor style preference rather than a critical maintainability issue, as the component name appears infrequently and its purpose is clear from context.

Likely an incorrect or invalid review comment.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sdodson
Once this PR has been reviewed and has the lgtm label, please assign hongkailiu for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sdodson
Copy link
Member Author

sdodson commented Dec 2, 2025

/test images unit

@sdodson sdodson changed the title Allow multiple machine-os versions MCO-1961: Allow multiple machine-os versions Dec 3, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 3, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 3, 2025

@sdodson: This pull request references MCO-1961 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 "4.21.0" version, but no target version was set.

Details

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

@sdodson sdodson marked this pull request as ready for review December 3, 2025 03:36
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2025
@openshift-ci openshift-ci bot requested review from ardaguclu and tchap December 3, 2025 03:37
if ok {
if existing.Version != v.Version {
// we allow multiple machine-os versions due to dual stream efforts
if existing.Version != v.Version && k != "machine-os" {
Copy link
Member

@ardaguclu ardaguclu Dec 3, 2025

Choose a reason for hiding this comment

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

This disables component version skew check entirely for machine-os component. Would it be better if we only allow 2 different machine-os versions and fail if there are 3 (to prevent any divergence) as we did #1662 (and #1656).

Additionally, same changes need to go Hypershift https://github.com/openshift/hypershift/blob/02f528cd5364b03168500ada33c9a0d9d0593025/support/releaseinfo/releaseinfo.go#L185

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out the hypershift use case. For the time being we're simply dropping the machine-os version from the rhel-coreos-10 images but I think we need to figure out how to effectively re-introduce this. Ideally for 4.22 but perhaps deferred to the next release after 4.22. I'm going to close this for the time being but open up a ticket for the RHCOS team to dive into this across the problem space.

Choose a reason for hiding this comment

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

Limiting to two values here will probably backfire later because we may have more streams coming up : the nvidia kernel, confidential clusters.. Now that this can of worms is open, more may even be added

@tchap
Copy link
Contributor

tchap commented Dec 3, 2025

We should also add some tests BTW.

@sdodson
Copy link
Member Author

sdodson commented Dec 5, 2025

/close
for now, RHCOS team to take a ticket to chase this across the problem space to ensure that we can have multiple unique versions for RHCOS in the release payload.

@openshift-ci openshift-ci bot closed this Dec 5, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 5, 2025

@sdodson: Closed this PR.

Details

In response to this:

/close
for now, RHCOS team to take a ticket to chase this across the problem space to ensure that we can have multiple unique versions for RHCOS in the release payload.

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.

@jbtrystram
Copy link

Picking that up.
So i did build a payload with two machine-os images, and with this patch, oc does not complain.

oc adm release info quay.io:443/jbtrystramtestimages/rhel-coreos:4.22.0-ec.3-full-payload                                
Name:           4.22.0-ec.3
Digest:         sha256:d6514d0ec05b7e09e8f0dedcc68fe23d3afdd3a899dba391258210cc5399efa2
Created:        2026-03-11T20:08:31Z
OS/Arch:        linux/amd64
Manifests:      859
Metadata files: 2

Pull From: quay.io:443/jbtrystramtestimages/rhel-coreos@sha256:d6514d0ec05b7e09e8f0dedcc68fe23d3afdd3a899dba391258210cc5399efa2

Release Metadata:
  Version:  4.22.0-ec.3
  Upgrades: 4.21.0, 4.21.1, 4.21.2, 4.21.3, 4.21.4, 4.22.0-ec.0, 4.22.0-ec.1, 4.22.0-ec.2
  Metadata:

Component Versions:
  kubectl          1.34.1        
  kubernetes       1.34.2        
  kubernetes-tests 1.34.1        
  machine-os       10.2.20260311 Red Hat Enterprise Linux CoreOS

Images:
  NAME                                           DIGEST
  agent-installer-api-server               
<...> 
  rhel-coreos                                    sha256:42e2e4d11674c603398469770700ef5a30432294ffa245ed1c67852e93fc380e
  rhel-coreos-10                                 quay.io:443/jbtrystramtestimages/rhel-coreos:4.22-10.2-tagged
  rhel-coreos-10-extensions                      sha256:ee90288a9dfc801da76f890438957d38702c09c94ecaaaeef0f56b5cd2eb19a9
  rhel-coreos-extensions                         sha256:b7d9bb1dee4383e0e64baaabb915c194deebf9cbe19726aaea7e3498eaa2c59e
<...>

Warnings:
* multiple versions or display names reported for the following component(s): machine-os

@sdodson
Copy link
Member Author

sdodson commented Mar 17, 2026

/reopen
@ardaguclu We're now looking at potentially having multiple streams which means we'll have N number of images labeled with machine-os version. Can you think of any issues that will arise in product if we allow that?

Thanks @jbtrystram for testing this out, I was never able to get to that point.

@openshift-ci openshift-ci bot reopened this Mar 17, 2026
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2026

@sdodson: Reopened this PR.

Details

In response to this:

/reopen
@ardaguclu We're now looking at potentially having multiple streams which means we'll have N number of images labeled with machine-os version. Can you think of any issues that will arise in product if we allow that?

Thanks @jbtrystram for testing this out, I was never able to get to that point.

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.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 17, 2026

@sdodson: This pull request references MCO-1961 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 "4.22.0" version, but no target version was set.

Details

In 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
Copy link
Contributor

openshift-ci bot commented Mar 17, 2026

@sdodson: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants