fix(alerts): Remove duplicate information in the recommend command#2279
fix(alerts): Remove duplicate information in the recommend command#2279nbottari9 wants to merge 4 commits into
Conversation
… alert checking if the server is already doing it. Signed-off-by: Nick Bottari <nbottari@nbottari-thinkpadp1gen4i.boston.csb>
… the server is already checking for alerts Signed-off-by: Nick Bottari <nbottari9@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe recommend command now checks the cluster FeatureGate for ClusterUpdateAcceptRisks and the cluster Infrastructure; when accept-risks is enabled for the desired version or hypershift is enabled, it skips the client-side precheck, related issue insertions, and alert printing. ChangesConditional client-side alert checking for upgrade recommendations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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 |
|
[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 |
| func shouldSkipClientAlertChecking(cv *configv1.ClusterVersion) bool { | ||
| for _, update := range cv.Status.ConditionalUpdates { | ||
| for _, condition := range update.Conditions { | ||
| if condition.Type == "Recommended" && condition.Status == metav1.ConditionFalse { |
There was a problem hiding this comment.
There could be reasons for an update to be Recommended=False that aren't about the alert checks. And we want to be exhaustive in checking, because a cluster-admin should be able to say "oh, I'm ok with those issues" and be able to confidently update anyway. Maybe we can look in conditionalUpdateRisks for risk names that start with Alert? Or maybe the presence of conditionalUpdateRisks at all is enough to mark the relevant feature as enabled, and we don't need to build heuristics based on risk name? Either of those might double-check a cluster where the CVO checked and thought the cluster was clean, and then the client re-checked (and hopefully agreed the cluster was clean), but that seems ok during the transitional period, before we can drop the client-side checking entirely.
There was a problem hiding this comment.
(Sorry for late comments)
CVO works on the risks with the above guard, including the ones from alerts.
Could we do similar guard in oc to CVO, like this?
It checks the feature gate and HyperShift: oc has the tools to receive both.
…ing alerts Signed-off-by: Nick Bottari <nbottari9@gmail.com>
![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
Signed-off-by: Nick Bottari <nbottari9@gmail.com>
|
@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. |
| // if CVO is present in the cluster, | ||
| // and Hypershift is not enabled, | ||
| // we don't need to check for alerts here | ||
| if !isAcceptRisksEnabled(featureGate, cv.Status.Desired.Version) && !isHypershiftEnabled(infrastructure) { |
There was a problem hiding this comment.
In my mind, the guard will be inside the alert checking:
Something like if the gate is enabled and not Hypershift, then the function returns nil, nil like there were no alerts to check (because they are checked by CVO already and CVO will generate the Recommend condition if needed).
3 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