OTA-1814: fix(alerts): Remove duplicate information in the recommend command#2279
OTA-1814: fix(alerts): Remove duplicate information in the recommend command#2279nbottari9 wants to merge 2 commits into
Conversation
|
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 alerts function now detects when the ClusterUpdateAcceptRisks feature gate is enabled and hypershift is not in use; under those conditions, it returns early to skip client-side alert evaluation. Two helper functions inspect cluster FeatureGate and Infrastructure state to determine these conditions, and new unit tests validate the helper behavior. ChangesConditional alert checking based on feature gates and hypershift
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 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/recommend.go`:
- Around line 187-190: The call to o.Client.ConfigV1().FeatureGates().Get(ctx,
"cluster", metav1.GetOptions{}) is ignoring its error return; capture the error
(e.g., err) instead of using `_` and log it with klog.V(4).Infof() (including
context like that the FeatureGate fetch failed) while keeping the existing nil
fallback behavior for featureGate; update the block that references featureGate
and o.Client to handle the captured error and log it appropriately.
🪄 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: 260e5e72-5faa-490c-859c-5290a98e1dc7
📒 Files selected for processing (1)
pkg/cli/admin/upgrade/recommend/recommend.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/recommend/recommend_test.go (1)
34-67: ⚡ Quick winExpand test coverage with table-driven tests.
These tests only cover the positive cases. Critical edge cases are missing: nil inputs, version mismatches, and feature-not-enabled scenarios. Table-driven tests would improve coverage and align with guidelines.
As per coding guidelines: "Unit tests should use co-located
*_test.gofiles with table-driven tests."♻️ Proposed table-driven tests
-func TestIsAcceptRisksEnabled(t *testing.T) { - featureGate := &configv1.FeatureGate{ - Status: configv1.FeatureGateStatus{ - FeatureGates: []configv1.FeatureGateDetails{ - { - Version: "4.22.0", - Enabled: []configv1.FeatureGateAttributes{ - { - Name: features.FeatureGateClusterUpdateAcceptRisks, - }, - }, - }, - }, - }, - } - - result := isAcceptRisksEnabled(featureGate, "4.22.0") - if result != true { - t.Errorf("ClusterUpdateAcceptRisks feature gate should report as enabled") - } -} - -func TestIsHypershiftEnabled(t *testing.T) { - infra := &configv1.Infrastructure{ - Status: configv1.InfrastructureStatus{ - ControlPlaneTopology: configv1.ExternalTopologyMode, - }, - } - - result := isHypershiftEnabled(infra) - if result != true { - t.Errorf("Hypershift should report as enabled") - } -} +func TestIsAcceptRisksEnabled(t *testing.T) { + tests := []struct { + name string + featureGate *configv1.FeatureGate + clusterVersion string + want bool + }{ + { + name: "nil feature gate", + featureGate: nil, + clusterVersion: "4.22.0", + want: false, + }, + { + name: "feature gate enabled for matching version", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.22.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: features.FeatureGateClusterUpdateAcceptRisks}, + }, + }, + }, + }, + }, + clusterVersion: "4.22.0", + want: true, + }, + { + name: "feature gate enabled for different version", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.21.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: features.FeatureGateClusterUpdateAcceptRisks}, + }, + }, + }, + }, + }, + clusterVersion: "4.22.0", + want: false, + }, + { + name: "feature gate not in enabled list", + featureGate: &configv1.FeatureGate{ + Status: configv1.FeatureGateStatus{ + FeatureGates: []configv1.FeatureGateDetails{ + { + Version: "4.22.0", + Enabled: []configv1.FeatureGateAttributes{ + {Name: "SomeOtherFeature"}, + }, + }, + }, + }, + }, + clusterVersion: "4.22.0", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isAcceptRisksEnabled(tt.featureGate, tt.clusterVersion) + if got != tt.want { + t.Errorf("isAcceptRisksEnabled() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestIsHypershiftEnabled(t *testing.T) { + tests := []struct { + name string + infra *configv1.Infrastructure + want bool + }{ + { + name: "nil infrastructure", + infra: nil, + want: false, + }, + { + name: "external topology mode (hypershift)", + infra: &configv1.Infrastructure{ + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.ExternalTopologyMode, + }, + }, + want: true, + }, + { + name: "high availability mode", + infra: &configv1.Infrastructure{ + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.HighlyAvailableTopologyMode, + }, + }, + want: false, + }, + { + name: "single replica mode", + infra: &configv1.Infrastructure{ + Status: configv1.InfrastructureStatus{ + ControlPlaneTopology: configv1.SingleReplicaTopologyMode, + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isHypershiftEnabled(tt.infra) + if got != tt.want { + t.Errorf("isHypershiftEnabled() = %v, want %v", got, tt.want) + } + }) + } +}🤖 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_test.go` around lines 34 - 67, Refactor the TestIsAcceptRisksEnabled and TestIsHypershiftEnabled test functions to use table-driven tests instead of single-case tests. The current tests only cover positive scenarios and miss critical edge cases. For TestIsAcceptRisksEnabled, add test cases for nil featureGate input, version mismatches, disabled features, and empty feature gates. For TestIsHypershiftEnabled, add test cases for nil infra input, different control plane topology modes, and other edge conditions. Use a table structure with test case names, input parameters, and expected results, then iterate through the table to run each scenario.Source: Coding guidelines
🤖 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 33-35: The condition checking cv == nil then attempting to
dereference cv.Status.Desired.Version will cause a nil pointer panic because cv
cannot be dereferenced when it is nil. Change the condition from cv == nil to cv
!= nil in the if statement at line 33 to ensure the ClusterVersion object is not
nil before accessing its Status.Desired.Version field in the
isAcceptRisksEnabled function call.
- Around line 27-29: The three API calls FeatureGates().Get(),
Infrastructures().Get(), and ClusterVersions().Get() are silently discarding
their error returns using the blank identifier. Instead of ignoring errors with
_, capture the error return values from each of these Get() calls and handle
them appropriately. Check each error and either log it, return it, or handle the
failure case explicitly so that network issues, RBAC denials, and other failures
are properly distinguished from successful operations rather than being silently
ignored.
---
Nitpick comments:
In `@pkg/cli/admin/upgrade/recommend/recommend_test.go`:
- Around line 34-67: Refactor the TestIsAcceptRisksEnabled and
TestIsHypershiftEnabled test functions to use table-driven tests instead of
single-case tests. The current tests only cover positive scenarios and miss
critical edge cases. For TestIsAcceptRisksEnabled, add test cases for nil
featureGate input, version mismatches, disabled features, and empty feature
gates. For TestIsHypershiftEnabled, add test cases for nil infra input,
different control plane topology modes, and other edge conditions. Use a table
structure with test case names, input parameters, and expected results, then
iterate through the table to run each scenario.
🪄 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: a2529d24-e031-4cd3-ad50-33dc89d0cd7f
📒 Files selected for processing (3)
pkg/cli/admin/upgrade/recommend/alerts.gopkg/cli/admin/upgrade/recommend/recommend.gopkg/cli/admin/upgrade/recommend/recommend_test.go
nbottari9
force-pushed
the
1814-duplicate-warning
branch
2 times, most recently
from
d9a7f2a to
9cb58aa
Compare
nbottari9
force-pushed
the
1814-duplicate-warning
branch
from
b6b3fb2 to
05b6aaa
Compare
hongkailiu
left a comment
hongkailiu
left a comment
There was a problem hiding this comment.
I believe it is pretty close.
Only a few of code refactoring for simplicity.
We are ready to take mock data from a cluster (HowTo is discussed over slack) and add them into examples and see if we do the output correctly.
| skip, err := o.alertsEvaluatedByCVO(ctx) | ||
| if err != nil { | ||
| klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err) | ||
| } | ||
| if skip { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
nit: check skip only if err==nil.
If err!=nil (i.e., fail to determine if CVO does it already), oc does it anyway.
| skip, err := o.alertsEvaluatedByCVO(ctx) | |
| if err != nil { | |
| klog.Warningf("An error occured while determining if the CVO is evaluating alerts, so the client will check. %v", err) | |
| } | |
| if skip { | |
| return nil, nil | |
| } | |
| 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 | |
| } |
| }{ | ||
| { | ||
| name: "no infrastructure", | ||
| expected: false, |
There was a problem hiding this comment.
nit: no need to set up zero value explicitly. It applies to other cases too.
| expected: false, |
|
/retitle OTA-1814: fix(alerts): Remove duplicate information in the recommend command |
openshift-ci
Bot
changed the title
|
@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. |
openshift-ci-robot
added
the
jira/valid-reference
| @@ -0,0 +1,14 @@ | |||
| Failing=True: | |||
|
|
|||
| Reason: ClusterOperatorDegraded | |||
There was a problem hiding this comment.
This is a bit of noise for our test. 🤷
| "examples/5.0.0-cvo-handling-risks-cv.yaml": "1.2.3-not-important", | ||
| "examples/5.0.0-cvo-not-handling-risks-cv.yaml": "1.2.3-not-important", |
There was a problem hiding this comment.
| "examples/5.0.0-cvo-handling-risks-cv.yaml": "1.2.3-not-important", | |
| "examples/5.0.0-cvo-not-handling-risks-cv.yaml": "1.2.3-not-important", | |
| "examples/5.0.0-cvo-handling-risks-cv.yaml": "5.0.0-ec.3", | |
| "examples/5.0.0-cvo-not-handling-risks-cv.yaml": "5.0.0-ec.3", |
I missed this in the previous rounds.
We will do some clean up manually after UPDATE=TRUE go test.
Then you will find out we need to handle issues that apply to this cluster but which were not included in from
| Reason: PodDisruptionBudgetAtLimit | ||
| Message: The pod disruption budget is preventing further disruption to pods. https://github.com/openshift/runbooks/blob/master/alerts/cluster-kube-controller-manager-operator/PodDisruptionBudgetAtLimit.md | ||
|
|
||
| error: issues that apply to this cluster but which were not included in --accept: ConditionalUpdateRisk,Failing |
There was a problem hiding this comment.
I expect this to go to the other branch.
| if err != nil { | ||
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | ||
| } | ||
| fmt.Fprintf(o.Out, "Cluster update risks are being handled by the Cluster Version Operator (CVO). Please use `oc adm upgrade accept ...` to accept risks.\n") |
There was a problem hiding this comment.
Perhaps error fits better. What do you think?
| fmt.Fprintf(o.Out, "Cluster update risks are being handled by the Cluster Version Operator (CVO). Please use `oc adm upgrade accept ...` to accept risks.\n") | |
| return fmt.Errorf("issues that apply to this cluster and are not accepted. The command `oc adm upgrade accept` can be used to accept them: %s", strings.Join(sets.List(unaccepted), ",")) |
There was a problem hiding this comment.
Not sure, personally I think nothing wrong necessarily occurred in the code, more informing the user they should use the other command.
| if err != nil { | ||
| return fmt.Errorf("failed to determine if CVO is checking alerts: %v", err) | ||
| } | ||
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. Cluster update risks are being handled by the Cluster Version Operator (CVO). Please use `oc adm upgrade accept ...` to accept them: %s\n", strings.Join(sets.List(issues), ",")) |
There was a problem hiding this comment.
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. Cluster update risks are being handled by the Cluster Version Operator (CVO). Please use `oc adm upgrade accept ...` to accept them: %s\n", strings.Join(sets.List(issues), ",")) | |
| return fmt.Errorf("There are issues that apply to this cluster and have not been accepted. Cluster update risks are being handled by the Cluster Version Operator (CVO). Please use `oc adm upgrade accept` to accept them if the associated risks are acceptable: %s\n", strings.Join(sets.List(issues), ",")) |
nbottari9
force-pushed
the
1814-duplicate-warning
branch
from
ef53167 to
4470fbe
Compare
…recommend` command Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
nbottari9
force-pushed
the
1814-duplicate-warning
branch
from
ce17dc8 to
ef559e5
Compare
| Reason: TestAlert | ||
| Message: Test alert for updates. https://github.com/openshift/runbooks/tree/master/alerts?runbook=notfound | ||
|
|
||
| error: There are issues that apply to this cluster and have not been accepted. `oc adm upgrade accept` can be used to accept them: ConditionalUpdateRisk |
There was a problem hiding this comment.
Why is TestAlert missing here?
We probably need a bigger effort here and it is totally fine if you want to do it with a follow-up pull.
The effort would be working on the calculation of unaccepted when CVO handles the alert risks.
There was a problem hiding this comment.
recommend.go - line 345, should be unaccepted not issues
|
@nbottari9: The following test 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. |
…VO-handling case Signed-off-by: Nicholas Bottari <nbottari9@gmail.com>
|
/lgtm We will fix #2279 (comment) with a follow up. |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, nbottari9 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, nbottari9 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 |
openshift-ci
Bot
added
the
approved
4 participants
Context
If the
ClusterUpdateAcceptRisksfeature gate is enabled on the cluster, the CVO will include alerts in the conditional update risks. The client also checks for alerts, which can cause warnings or info messages to be printed twice.Description
We can detect if the server (CVO) is already including alerts by checking if
Recommended=Falsein theClusterVersionconditional updates. If this is set, we know that the feature gate is enabled, therefore the server is checking alerts and we don't need to on the client.Changes
ifstatement before callingprecheck()to check if the server is including alertsshouldSkipClientAlertChecking()- loops over theConditionalUpdatesand checks to see if any of them haveRecommended=False. If so, return true to indicate the server IS checking for alerts and we don't need to on the client.TestServerSideAlertRiskstest suite - Added new tests to test new functionalitySummary by CodeRabbit
Bug Fixes
Tests