diff --git a/pkg/cli/admin/upgrade/recommend/recommend.go b/pkg/cli/admin/upgrade/recommend/recommend.go index 89c54ea00f..c99f963046 100644 --- a/pkg/cli/admin/upgrade/recommend/recommend.go +++ b/pkg/cli/admin/upgrade/recommend/recommend.go @@ -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 ")) + } } } } @@ -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 { + return true + } + } + } + return false +} + func notRecommendedCondition(update configv1.ConditionalUpdate) *metav1.Condition { if len(update.Risks) == 0 { return nil diff --git a/pkg/cli/admin/upgrade/recommend/recommend_test.go b/pkg/cli/admin/upgrade/recommend/recommend_test.go index 6df6cda3a4..fd746710b7 100644 --- a/pkg/cli/admin/upgrade/recommend/recommend_test.go +++ b/pkg/cli/admin/upgrade/recommend/recommend_test.go @@ -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) { @@ -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) + } + }) + } +}