MCO-1961: Allow multiple machine-os versions - #2157
Conversation
|
Skipping CI for Draft Pull Request. |
WalkthroughAllow the "machine-os" component to differ across tags: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test images unit |
|
@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. 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. |
| 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" { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
We should also add some tests BTW. |
|
/close |
|
@sdodson: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
Picking that up. |
|
/reopen Thanks @jbtrystram for testing this out, I was never able to get to that point. |
|
@sdodson: Reopened this PR. 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 kubernetes-sigs/prow repository. |
|
@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. 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. |
It is difficult to see the implications of this change (maybe there isn't any). But oc/pkg/cli/admin/release/new.go Lines 55 to 58 in 05fa7bb oc/pkg/cli/admin/release/info.go Line 2113 in 05fa7bb In my opinion, by allowing any arbitrary stream we would permanently lose the control. Would it be too difficult to embed all possible streams in oc and do not allow the others?. That would bring about some maintenance burden, but at least we intentionally update the list. |
|
Also cc'ing @wking @bradmwilliams (please ignore if you are not related to this topic) |
In the past few months its become clear that we're likely to see a proliferation of streams. It's probably right on the tipping point as to whether or not it will be manageable to do this with a lot of cross repo coordination, right now the MCO, Installer, and RHCOS are all oriented toward dynamic discovery so that we don't have to coordinate. |
|
@sdodson here is a patch to remove the warning when creating the payload : |
|
I'll defer the decision to @sdodson about relaxing the validation |
Thanks, I wasn't able to |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cli/admin/release/info.go (1)
1062-1062: Consider extracting the"machine-os"string to a constant.The literal
"machine-os"is used in multiple locations across this file andimage_mapper.go. A shared constant would reduce the risk of typos and make future updates easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/release/info.go` at line 1062, The literal "machine-os" is duplicated; define a shared constant (e.g., MachineOSKey or MACHINE_OS) and replace all occurrences of the string in this file and in image_mapper.go (locations like the conditional using k == "machine-os" and any map lookups or key comparisons) to use that constant; update imports/visibility as needed so both files can reference the constant (place it in a common package or exported const) and run tests to ensure no typos remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cli/admin/release/info.go`:
- Line 1062: The literal "machine-os" is duplicated; define a shared constant
(e.g., MachineOSKey or MACHINE_OS) and replace all occurrences of the string in
this file and in image_mapper.go (locations like the conditional using k ==
"machine-os" and any map lookups or key comparisons) to use that constant;
update imports/visibility as needed so both files can reference the constant
(place it in a common package or exported const) and run tests to ensure no
typos remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29b7d6bd-fc19-48f8-8a64-638452406180
📒 Files selected for processing (2)
pkg/cli/admin/release/image_mapper.gopkg/cli/admin/release/info.go
|
I'd like to move forward with this so we can make sure that this works properly in time for branch cut. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, jbtrystram, sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@sdodson: 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. |
|
/verified later |
|
@sdodson: 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. |
|
/verified later @sdodson |
|
@sdodson: This PR has been marked to be verified later by 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. |
Since we merged openshift/oc#2157 `oc` supports multiple `machine-os` images, so we can write those labels. See openshift/oc#2157
|
/cherry-pick release-4.21 |
|
@sdodson: new pull request created: #2244 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 kubernetes-sigs/prow repository. |
Allow multiple machine-os versions in component version checks to support dual stream efforts. This mirrors the same change in openshift/oc#2157. Refs: openshift/oc#2157 Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow multiple machine-os versions in component version checks to support dual stream efforts. This mirrors the same change in openshift/oc#2157. Refs: openshift/oc#2157 Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow multiple machine-os versions in component version checks to support dual stream efforts. This mirrors the same change in openshift/oc#2157. Refs: openshift/oc#2157 Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allow multiple machine-os versions in component version checks to support dual stream efforts. This mirrors the same change in openshift/oc#2157. Refs: openshift/oc#2157 Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
No description provided.