OCPBUGS-8247: return error when --to-multi-arch is used on a multi-arch cluster - #2340
OCPBUGS-8247: return error when --to-multi-arch is used on a multi-arch cluster#2340savio87 wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@savio87: This pull request references Jira Issue OCPBUGS-8247, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
WalkthroughThe ChangesMulti-architecture upgrade validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: savio87 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 |
|
Hi @savio87. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Adds a check on status.desired.architecture to detect clusters already running multi-architecture, returning a clear error instead of attempting a redundant patch. Could a maintainer update the target version on OCPBUGS-8247 and run /ok-to-test? Thanks! |
|
|
||
| case o.ToMultiArch: | ||
| if cv.Status.Desired.Architecture == configv1.ClusterVersionArchitectureMulti { | ||
| return fmt.Errorf("Cluster is already multi-architecture") |
There was a problem hiding this comment.
I am not sure the message is precise, the docs say:
// desired is the version that the cluster is reconciling towards.
// If the cluster is not yet fully initialized desired will be set
// with the information available, which may be an image or a tag.
// +required
Desired Release `json:"desired"`So it may be still progressing, I guess.
There was a problem hiding this comment.
I have to say I don't know exactly how this works, it's just based on the docs.
|
Good point @tchap — |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/upgrade.go (1)
248-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for both validation guards.
Use
upgrade_test.goto cover these cases:
Status.Desired.Architecture == Multireturns the already-multi-architecture error.Spec.DesiredUpdate.Architecture == Multireturns the already-requested error.- Both fields set confirm that the status guard runs first.
Assert the exact error messages.
🤖 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/cli/admin/upgrade/upgrade.go` around lines 248 - 252, Add regression tests in upgrade_test.go for the validation logic surrounding Status.Desired.Architecture and Spec.DesiredUpdate.Architecture: verify each Multi case returns its exact expected error message, and verify when both are Multi the status guard’s error is returned first. Use the existing upgrade test setup and invoke the relevant upgrade validation flow.
🤖 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.
Nitpick comments:
In `@pkg/cli/admin/upgrade/upgrade.go`:
- Around line 248-252: Add regression tests in upgrade_test.go for the
validation logic surrounding Status.Desired.Architecture and
Spec.DesiredUpdate.Architecture: verify each Multi case returns its exact
expected error message, and verify when both are Multi the status guard’s error
is returned first. Use the existing upgrade test setup and invoke the relevant
upgrade validation flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ff753571-639f-4549-b4c3-9be44e36060d
📒 Files selected for processing (1)
pkg/cli/admin/upgrade/upgrade.go
3 participants
Summary
Running
oc adm upgrade --to-multi-archon a cluster that is already multi-architecture did not return an error. The existing check only caught the case where a migration was requested (viaspec.desiredUpdate.architecture) but not yet completed. After completion, CVO clears that field, so the command would attempt the patch again.This adds a check on
status.desired.architectureto detect clusters that are already running multi-architecture and return a clear error message.Bug: https://redhat.atlassian.net/browse/OCPBUGS-8247
Changes
cv.Status.Desired.Architecture == ClusterVersionArchitectureMulti, return"Cluster is already multi-architecture"spec.desiredUpdatecheck, which catches in-progress migrationsSummary by CodeRabbit