OTA-1814: fix(alerts): populate CVO-sourced risks in issues#2319
OTA-1814: fix(alerts): populate CVO-sourced risks in issues#2319nbottari9 wants to merge 15 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@nbottari9: This pull request references OTA-1814 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 bug 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. |
|
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:
WalkthroughThe upgrade recommendation command now loads related mock data, evaluates conditional risks from ClusterVersion status when CVO checks alerts, and provides context-specific acceptance guidance. The example output also lists ChangesCVO risk handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
![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
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/recommend/alerts.go (1)
263-290: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the result to avoid redundant API calls.
This method is called twice during
oc adm upgrade recommend --version <version>(once fromprecheck->alertsand once directly inRun). Since it makes up to three API calls each time when not using mock data, consider caching the boolean result or the retrieved API objects on theoptionsstruct to avoid duplicate requests.🤖 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/recommend/alerts.go` around lines 263 - 290, Update options and alertsEvaluatedByCVO to cache the computed result or retrieved FeatureGates, Infrastructure, and ClusterVersion objects after the first non-mock evaluation. Reuse that cached state on subsequent calls so precheck and Run do not repeat the API requests, while preserving mock-data behavior and the existing boolean result.
🤖 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/cli/admin/upgrade/recommend/alerts.go`:
- Around line 26-31: Correct the spelling of “occured” to “occurred” in the
klog.Warningf message within the alertsEvaluatedByCVO error-handling block,
without changing the surrounding behavior.
In `@pkg/cli/admin/upgrade/recommend/recommend.go`:
- Around line 340-354: Update the alertsEvaluatedByCVO handling in the
recommendation flow to match alerts()/precheck behavior: log the API error as a
warning and continue with client-side checks instead of returning a hard
failure, while preserving CVO-specific conditional-risk handling only when the
check succeeds. Also revise the unaccepted-issues error message to start
lowercase and omit the trailing newline; if hard failure is retained, wrap the
original error with %w.
---
Nitpick comments:
In `@pkg/cli/admin/upgrade/recommend/alerts.go`:
- Around line 263-290: Update options and alertsEvaluatedByCVO to cache the
computed result or retrieved FeatureGates, Infrastructure, and ClusterVersion
objects after the first non-mock evaluation. Reuse that cached state on
subsequent calls so precheck and Run do not repeat the API requests, while
preserving mock-data behavior and the existing boolean result.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6699e1c3-d867-4a04-832d-8f4aaea5a760
📒 Files selected for processing (19)
pkg/cli/admin/upgrade/recommend/alerts.gopkg/cli/admin/upgrade/recommend/alerts_test.gopkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-cv.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-featuregate.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks-infrastructure.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-handling-risks.version-5.0.0-ec.3-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-alerts.jsonpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-cv.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-featuregate.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks-infrastructure.yamlpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.show-outdated-releases-outputpkg/cli/admin/upgrade/recommend/examples/5.0.0-cvo-not-handling-risks.version-5.0.0-ec.3-outputpkg/cli/admin/upgrade/recommend/examples_test.gopkg/cli/admin/upgrade/recommend/mockresources.gopkg/cli/admin/upgrade/recommend/recommend.go
| if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil { | ||
| klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err) | ||
| } else if skip { | ||
| return nil, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the spelling in the log message.
There is a typo in the warning log: "occured" should be "occurred".
✏️ Proposed fix
- if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil {
- klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err)
- } else if skip {
+ if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil {
+ klog.Warningf("An error occurred while determining if the CVO is evaluating alerts, so the client will check. %v", err)
+ } else if skip {📝 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.
| if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil { | |
| klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err) | |
| } else if skip { | |
| return nil, nil | |
| } | |
| if skip, err := o.alertsEvaluatedByCVO(ctx); err != nil { | |
| klog.Warningf("An error occurred while determining if the CVO is evaluating alerts, so the client will check. %v", err) | |
| } else if skip { | |
| return nil, 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/cli/admin/upgrade/recommend/alerts.go` around lines 26 - 31, Correct the
spelling of “occured” to “occurred” in the klog.Warningf message within the
alertsEvaluatedByCVO error-handling block, without changing the surrounding
behavior.
| cvoChecking, err := o.alertsEvaluatedByCVO(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | ||
| } | ||
| if cvoChecking { | ||
| for _, name := range conditionalRisksFromCVO(cv) { | ||
| unaccepted.Insert(name) | ||
| } | ||
| } | ||
| if unaccepted.Len() > 0 { | ||
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | ||
| if cvoChecking { | ||
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | ||
| } else { | ||
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle API errors gracefully and format error strings idiomatically.
There are two concerns here:
- Inconsistent error handling: If
alertsEvaluatedByCVOreturns an error here, the command fails entirely. However, when the same method is called earlier inalerts()(viaprecheck), the error is gracefully logged as a warning and the CLI falls back to client-side checks. Handling the error gracefully here would ensure consistent command behavior. - Go error string conventions: The new error string starts with a capital letter and ends with a newline (
\n), which violates standard Go error conventions (e.g.,Effective Goguidelines). Error strings should be lowercase and avoid trailing punctuation or newlines.
As per coding guidelines, if you choose to keep the hard failure rather than falling back gracefully, you should wrap the error with fmt.Errorf("context: %w", err) instead of %v.
🛠️ Proposed fix (graceful fallback + style corrections)
- cvoChecking, err := o.alertsEvaluatedByCVO(ctx)
- if err != nil {
- return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err)
- }
+ cvoChecking, err := o.alertsEvaluatedByCVO(ctx)
+ if err != nil {
+ klog.Warningf("An error occurred while determining if the CVO is evaluating alerts, assuming client-side checks: %v", err)
+ cvoChecking = false
+ }
if cvoChecking {
for _, name := range conditionalRisksFromCVO(cv) {
unaccepted.Insert(name)
}
}
if unaccepted.Len() > 0 {
if cvoChecking {
- return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ","))
+ return fmt.Errorf("there are issues that apply to this cluster and have not been accepted; `oc adm upgrade accept` can be used to accept them: %s", strings.Join(sets.List(unaccepted), ","))
} else {
return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ","))
}📝 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.
| cvoChecking, err := o.alertsEvaluatedByCVO(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | |
| } | |
| if cvoChecking { | |
| for _, name := range conditionalRisksFromCVO(cv) { | |
| unaccepted.Insert(name) | |
| } | |
| } | |
| if unaccepted.Len() > 0 { | |
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | |
| if cvoChecking { | |
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | |
| } else { | |
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | |
| } | |
| cvoChecking, err := o.alertsEvaluatedByCVO(ctx) | |
| if err != nil { | |
| klog.Warningf("An error occurred while determining if the CVO is evaluating alerts, assuming client-side checks: %v", err) | |
| cvoChecking = false | |
| } | |
| if cvoChecking { | |
| for _, name := range conditionalRisksFromCVO(cv) { | |
| unaccepted.Insert(name) | |
| } | |
| } | |
| if unaccepted.Len() > 0 { | |
| if cvoChecking { | |
| return fmt.Errorf("there are issues that apply to this cluster and have not been accepted; `oc adm upgrade accept` can be used to accept them: %s", strings.Join(sets.List(unaccepted), ",")) | |
| } else { | |
| return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ",")) | |
| } |
🤖 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/recommend/recommend.go` around lines 340 - 354, Update
the alertsEvaluatedByCVO handling in the recommendation flow to match
alerts()/precheck behavior: log the API error as a warning and continue with
client-side checks instead of returning a hard failure, while preserving
CVO-specific conditional-risk handling only when the check succeeds. Also revise
the unaccepted-issues error message to start lowercase and omit the trailing
newline; if hard failure is retained, wrap the original error with %w.
Source: Coding guidelines
|
LGTM. I will do the labels after rebase. |
6e34130 to
273b9a3
Compare
Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
273b9a3 to
e9139f9
Compare
|
/lgtm |
openshift-ci
Bot
added
approved
|
Scheduling required tests: |
|
/verified by TestExamples @nbottari9 |
openshift-ci-robot
added
the
verified
|
@nbottari9: 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. |
…2296) * docs(alerts): add missing inputs for alerts, add new inputs for fg and infra. add more context Signed-off-by: Nicholas Bottari <nbottari9@gmail.com> * docs(alerts): address feedback in review. Signed-off-by: Nicholas Bottari <nbottari9@gmail.com> --------- Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
|
/retest |
NO-JIRA: Bump k8s dependencies to 1.36 along with openshift
|
/test e2e-aws-ovn-serial-1of2 |
…14-duplicate-warning" This reverts commit 54b7c12, reversing changes made to ae0477d. PR openshift#2279 introduced a regression: alertsEvaluatedByCVO() returns nil from alerts() when the ClusterUpdateAcceptRisks feature gate (TechPreview-only) is enabled, breaking `oc adm upgrade recommend` output. This is blocking the 5.0 nightly payload.
|
/verified remove |
openshift-ci-robot
removed
the
verified
|
@nbottari9: The 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 jobs fails with /hold TRT policy is to land a revert #2320 to restore payload health |
openshift-ci
Bot
added
the
do-not-merge/hold
TRT-2817: Revert "Merge pull request openshift#2279 from nbottari9/1814-duplicate-warning"
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: %s\n", strings.Join(sets.List(unaccepted), ",")) | ||
| } | ||
| } | ||
| if cv.Spec.DesiredUpdate != nil { |
There was a problem hiding this comment.
This should be inside the cvoChecking branch.
See https://github.com/openshift/oc/pull/2319/changes#r3624785682
I think that causes the failure of serial e2e.
/lgtm cancel
openshift-ci
Bot
added
needs-rebase
Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
|
@nbottari9: This pull request references OTA-1814 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 bug 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. |
Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
…ottari9/oc into 1814-cvo-enabled-accept-risks
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nbottari9 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 |
|
@nbottari9: 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
After fixing duplicate printing (see), CVO-sourced risks would not appear in list of unaccepted risks. Fixed by populating issues list with CVO-sourced risks
Summary by CodeRabbit