Skip to content

fix(alerts): Remove duplicate information in the recommend command #2279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nbottari9 wants to merge 2 commits into
base: main
Choose a base branch
from
+190 −34
Open

fix(alerts): Remove duplicate information in the recommend command #2279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
881c169
fix(alerts): prevent duplication of warnings by disabling client-side…
nbottari9 Jun 2, 2026
fb98845
fix(alerts): add tests to test skipping client-side alert checking if…
nbottari9 Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 52 additions & 34 deletions pkg/cli/admin/upgrade/recommend/recommend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,45 +185,48 @@ func (o *options) Run(ctx context.Context) error {
issues.Insert(string(configv1.OperatorProgressing))
}

conditions, err := o.precheck(ctx)
if err != nil {
if !o.quiet {
fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err)
}
issues.Insert("FailedToCompletePrecheck")
}
var happyConditions []string
var acceptedConditions []string
var unhappyConditions []string
for _, condition := range conditions {
if condition.Status == metav1.ConditionTrue {
happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason))
} else {
issues.Insert(condition.acceptanceName)
if accept.Has(condition.acceptanceName) {
acceptedConditions = append(acceptedConditions, condition.Type)
// if server is already doing alert checking, we don't need to do it here
if !shouldSkipClientAlertChecking(cv) {
conditions, err := o.precheck(ctx)
if err != nil {
if !o.quiet {
fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err)
}
issues.Insert("FailedToCompletePrecheck")
}
var happyConditions []string
var acceptedConditions []string
var unhappyConditions []string
for _, condition := range conditions {
if condition.Status == metav1.ConditionTrue {
happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason))
} else {
unhappyConditions = append(unhappyConditions, condition.Type)
issues.Insert(condition.acceptanceName)
if accept.Has(condition.acceptanceName) {
acceptedConditions = append(acceptedConditions, condition.Type)
} else {
unhappyConditions = append(unhappyConditions, condition.Type)
}
}
}
}

if !o.quiet {
if len(happyConditions) > 0 {
sort.Strings(happyConditions)
fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", "))
}
if len(acceptedConditions) > 0 {
sort.Strings(acceptedConditions)
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", "))
}
if len(unhappyConditions) > 0 {
sort.Strings(unhappyConditions)
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", "))
if !o.quiet {
if len(happyConditions) > 0 {
sort.Strings(happyConditions)
fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", "))
}
if len(acceptedConditions) > 0 {
sort.Strings(acceptedConditions)
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", "))
}
if len(unhappyConditions) > 0 {
sort.Strings(unhappyConditions)
fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", "))

for _, c := range conditions {
if c.Status != metav1.ConditionTrue {
fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))
for _, c := range conditions {
if c.Status != metav1.ConditionTrue {
fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))
}
}
}
}
Expand Down Expand Up @@ -418,6 +421,21 @@ func (o *options) Run(ctx context.Context) error {
return nil
}

// This function tests if the server is already checking for alerts
// by seeing if the CVO is actively recommending against updates based on risks.
// If so, we can skip checking for these alerts on the client-side, and prevent
// duplication
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 {
Copy link
Copy Markdown
Member

@wking wking Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true
}
}
}
return false
}

func notRecommendedCondition(update configv1.ConditionalUpdate) *metav1.Condition {
if len(update.Risks) == 0 {
return nil
Expand Down
138 changes: 138 additions & 0 deletions pkg/cli/admin/upgrade/recommend/recommend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

configv1 "github.com/openshift/api/config/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestSortConditionalUpdatesBySemanticVersions(t *testing.T) {
Expand All @@ -29,3 +30,140 @@ func TestSortConditionalUpdatesBySemanticVersions(t *testing.T) {
t.Errorf("%v != %v", actual, expected)
}
}

func TestServerSideAlertRisks(t *testing.T) {
cases := []struct {
name string
cv *configv1.ClusterVersion
expected bool
}{
{
name: "server includes alert risks - single update with Recommend=False",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{
{
Release: configv1.Release{Version: "4.22.0"},
Conditions: []metav1.Condition{
{
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: "TestAlert",
},
},
},
},
},
},
expected: true,
},
{
name: "server includes alert risks - multiple updates with one Recommend=False",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{
{
Release: configv1.Release{Version: "4.22.0"},
Conditions: []metav1.Condition{
{
Type: "Recommended",
Status: metav1.ConditionFalse,
},
},
},
{
Release: configv1.Release{Version: "4.22.1"},
Conditions: []metav1.Condition{
{
Type: "Recommended",
Status: metav1.ConditionFalse,
Reason: "TestAlert",
},
},
},
},
},
},
expected: true,
},
{
name: "server does not include alert risks - all Recommended=True",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{
{
Release: configv1.Release{Version: "4.22.0"},
Conditions: []metav1.Condition{
{
Type: "Recommended",
Status: metav1.ConditionTrue,
},
},
},
},
},
},
expected: false,
},
{
name: "server does not include alert risks - no CondtionalUpdates",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{},
},
},
expected: false,
},
{
name: "server does not include alert risks - no Recommended condition",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{
{
Release: configv1.Release{Version: "4.22.0"},
Conditions: []metav1.Condition{
{
Type: "OtherCondition",
Status: metav1.ConditionFalse,
},
},
},
},
},
},
expected: false,
},
{
name: "server does not include alert risks - update has no conditions",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: []configv1.ConditionalUpdate{
{
Release: configv1.Release{Version: "4.22.0"},
Conditions: []metav1.Condition{},
},
},
},
},
expected: false,
},
{
name: "server does not include alert risks - ConditionalUpdates is nil",
cv: &configv1.ClusterVersion{
Status: configv1.ClusterVersionStatus{
ConditionalUpdates: nil,
},
},
expected: false,
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
actual := shouldSkipClientAlertChecking(c.cv)
if actual != c.expected {
t.Errorf("shoudSkipClientAlertChecking() = %v, expected = %v", actual, c.expected)
}
})
}
}