Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request references OTA-1548 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Cobra subcommand to manage ClusterVersion.spec.desiredUpdate.acceptRisks, wires it behind a feature gate, propagates AcceptRisks into upgrade/cancel flows, adds unit and e2e tests, extends test CLI env var support, tweaks rollback visibility and describer exceptions, and bumps two dependencies. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant CLI as "oc upgrade accept\n(Cobra cmd)"
participant Client as "ClusterVersion client"
participant API as "Kubernetes API / ClusterVersion"
User->>CLI: invoke command (tokens, --replace/--clear)
CLI->>CLI: parse flags, validate args
CLI->>Client: GET ClusterVersion
Client->>API: GET /clusterversions/{name}
API-->>Client: return ClusterVersion (includes spec.desiredUpdate.acceptRisks)
Client-->>CLI: return cv
CLI->>CLI: compute new acceptRisks (getAcceptRisks)
alt changes required
CLI->>Client: PATCH ClusterVersion (JSON Merge Patch)
Client->>API: PATCH /clusterversions/{name}
API-->>Client: patched cv
Client-->>CLI: success
CLI->>User: print updated acceptRisks
else no changes
CLI->>User: print "no changes"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
hongkailiu
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/accept/accept.go (1)
134-136: Placeholder logic pending API update.The hardcoded fake risks bypass actual ClusterVersion data. Ensure this is tracked for completion once the
o/apidependency is updated.Would you like me to open an issue to track this TODO?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cli/admin/upgrade/accept/accept.gopkg/cli/admin/upgrade/upgrade.go
🧰 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/upgrade/accept/accept.gopkg/cli/admin/upgrade/upgrade.go
🧬 Code graph analysis (2)
pkg/cli/admin/upgrade/accept/accept.go (1)
pkg/cli/admin/upgrade/upgrade.go (1)
New(56-132)
pkg/cli/admin/upgrade/upgrade.go (1)
pkg/cli/admin/upgrade/accept/accept.go (1)
New(29-57)
🔇 Additional comments (1)
pkg/cli/admin/upgrade/upgrade.go (1)
28-28: LGTM!The import and feature gate wiring follow the established pattern used for the
statusandrollbacksubcommands.Also applies to: 126-128
hongkailiu
force-pushed
the
accept-cmd
branch
2 times, most recently
from
b4dc31c to
1efbc2c
Compare
hongkailiu
force-pushed
the
accept-cmd
branch
from
1efbc2c to
7e52894
Compare
openshift-ci
bot
removed
the
approved
hongkailiu
force-pushed
the
accept-cmd
branch
from
7e52894 to
62e2d9a
Compare
|
I will wait a bit on |
hongkailiu
force-pushed
the
accept-cmd
branch
6 times, most recently
from
87f10fc to
583aa51
Compare
|
Cluster bot: Testing results with 583aa51: So we showed that the oc/pkg/cli/admin/upgrade/upgrade.go Line 688 in 90d7ae6 |
openshift-ci-robot
added
the
verified
|
@JianLi-RH: This PR has been marked as verified 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. |
|
/testwith openshift/cluster-version-operator/main/e2e-agnostic-ovn-techpreview-serial openshift/cluster-version-operator#1337 |
|
@hongkailiu, |
We will collect the data for the new case and remove the label if the data look good. This will avoid supprises from the failures in payload testing speically in the platforms that are not covered in pre-submits.
openshift-ci-robot
removed
the
verified
| if cv.Spec.DesiredUpdate != nil && len(cv.Spec.DesiredUpdate.AcceptRisks) > 0 { | ||
| backup := cv.DeepCopy() | ||
| backup.Spec.DesiredUpdate.AcceptRisks = nil | ||
| backup, err = configClient.ClusterVersions().Update(ctx, backup, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
I want to keep this one with a guard.
See b655628
That SpecStateSkipped guard protects the Update from running in clusters that don't have the acceptRisks property. Which is good. But I don't see how it protects the Update from clearing out spec.someNewProperty if someNewProperty is added to openshift/api and vendored into openshift/cluster-version-operator but folks forget to bump the vendored API in this oc repository.
There was a problem hiding this comment.
Oh. Now I see your point.
Your concerns is not AcceptRisk because we know oc bumps API already that we can reply on.
Instead, it is about a new field in the spec that happens in the future.
The backup is the whole CV, and oc might not recognize the field because of not yet bumping to the new API. Some component, e.g. CVO, fills that field in, my backup will destroy it.
What a case! 👍
Let me call the patch since I got your point now.
There was a problem hiding this comment.
https://redhat.atlassian.net/browse/OTA-1901
We will do it for 5.0.
Because new API is unlikely to happen before that.
| cli *CLI | ||
| verb string | ||
| args []string | ||
| addEnvVars map[string]string |
There was a problem hiding this comment.
note to self, this is catching the local utils up with openshift/origin@85cad81 (openshift/origin#29954), which added similar functionality to origin's CLI structure.
|
@hongkailiu: 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. |
|
/testwith openshift/cluster-version-operator/main/e2e-agnostic-ovn-techpreview-serial openshift/cluster-version-operator#1337 |
|
#2241 remerged the vendoring we depend on here. /retest-required |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
I'm still nervous about the use of Update, but whatever, it's just CI, and if it breaks CI in the future, we can pivot to patch at that point. Not a merge blocker for me. The actual shipped-to-users portions of this change look solid to me, so let's merge:
/lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hongkailiu, wking 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 |
|
/testwith openshift/cluster-version-operator/main/e2e-agnostic-ovn-techpreview-serial openshift/cluster-version-operator#1337 |
|
In this job, the case is skipped as TP is not enabled. $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oc/2170/pull-ci-openshift-oc-main-e2e-aws-ovn-serial-1of2/2036653227151200256/artifacts/e2e-aws-ovn-serial/openshift-e2e-test/artifacts/e2e.log | grep risks
started: 0/1/63 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
skipped: (200ms) 2026-03-25T06:27:37 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks" |
|
/retest-required |
|
Run the test (b655628) locally with a TP enabled cluster: |
|
/verified by @hongkailiu |
openshift-ci-robot
added
the
verified
|
@hongkailiu: This PR has been marked as verified 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. |
|
The multi-pr job failed on other tests (seem not relevant to this pull). Our e2e tests are green: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/multi-pr-openshift-oc-2170-openshift-cluster-version-operator-1337-e2e-agnostic-ovn-techpreview-serial/2036880026560892928/artifacts/e2e-agnostic-ovn-techpreview-serial/openshift-e2e-test/artifacts/e2e.log | rg risks
started: 0/1/112 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
passed: (30.8s) 2026-03-25T22:33:43 "[Jira:\"Cluster Version Operator\"] cluster-version-operator should work with accept risks"
started: 0/56/112 "[sig-cli][OCPFeatureGate:ClusterUpdateAcceptRisks] oc can operate accept risks [Serial]"
passed: (1.7s) 2026-03-25T23:25:20 "[sig-cli][OCPFeatureGate:ClusterUpdateAcceptRisks] oc can operate accept risks [Serial]" |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Dropped some comments but overall mechanism looks good to me. However, I think we need to see green OTE tests
| for _, risk := range acceptRisks { | ||
| names = append(names, risk.Name) | ||
| } | ||
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) |
There was a problem hiding this comment.
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) | |
| fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) //nolint:errcheck |
| } | ||
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are [%s]\n", strings.Join(names, ", ")) | ||
| } else { | ||
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are not changed\n") |
There was a problem hiding this comment.
| _, _ = fmt.Fprintf(o.Out, "info: Accept risks are not changed\n") | |
| fmt.Fprintf(o.Out, "info: Accept risks are not changed\n") //nolint:errcheck |
|
|
||
| func (o *Options) Run() error { | ||
| cv, err := o.Client.ConfigV1().ClusterVersions().Get(context.TODO(), "version", metav1.GetOptions{}) | ||
| func (o *Options) Run(ctx context.Context) error { |
There was a problem hiding this comment.
In my opinion, it is better to use context.TODO instead of wiring ctx which is almost always nil.
| configClient, err = configv1client.NewForConfig(config) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| skipIfMicroShift(oc) |
There was a problem hiding this comment.
Is skipping in BeforeEach effective for every test?. I think, it is better to skip in each test respectively.
|
In my opinion e2e-aws-oc-ote and e2e-aws-oc-ote-serial all should be green, especially since this PR define new tests. |
6 participants
With OC_ENABLE_CMD_UPGRADE_ACCEPT_RISKS=true, a new command
oc adm upgrade acceptis enabled. It accepts comma-separated risks exposed to an OpenShift release [1].The risks are stored in
clusterversion/version's.specs.desiredUpdate.acceptRisks.[1]. https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html-single/updating_clusters/index#understanding-clusterversion-conditiontypes_understanding-openshift-updates
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Style