From d071b82684f1aa4ee7d8216c660aa2069563e4fb Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 16 Aug 2021 17:29:58 -0700 Subject: [PATCH] pkg/cli/admin/upgrade: Report on Failing!=False conditions cae0b5e096 (React to degraded condition change, 2019-04-23, openshift/origin#22644) moved this code from Failing to Degraded, likely inspired by [1]. But Degraded is only used in ClusterOperator. ClusterVersion kept using Failing, as seen in [2]. This commit returns us to watching for Failing (the condition the CVO has been setting the whole time), and informing the caller for any non-happy statuses (or the lack of a Failing condition at all). Even though the issue causing `Failing=True` may block the current update from progressing, it should not block admins from requesting a new update target. For some bugs, retargeting is the recommended way to resolve the issue that is currently sticking the update [3]. [1]: https://github.com/openshift/api/pull/287 [2]: https://github.com/openshift/cluster-version-operator/pull/191 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1988576#c30 --- pkg/cli/admin/upgrade/upgrade.go | 41 +++++++++++++++++---------- pkg/cli/admin/upgrade/upgrade_test.go | 17 +++++++---- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/pkg/cli/admin/upgrade/upgrade.go b/pkg/cli/admin/upgrade/upgrade.go index 04f0170117..74c2f50081 100644 --- a/pkg/cli/admin/upgrade/upgrade.go +++ b/pkg/cli/admin/upgrade/upgrade.go @@ -28,6 +28,13 @@ import ( "github.com/openshift/oc/pkg/cli/admin/upgrade/channel" ) +const ( + // clusterStatusFailing is set on the ClusterVersion status when a cluster + // cannot reach the desired state. It is considered more serious than Degraded + // and indicates the cluster is not healthy. + clusterStatusFailing = configv1.ClusterStatusConditionType("Failing") +) + var upgradeExample = templates.Examples(` # Review the available cluster updates oc adm upgrade @@ -340,15 +347,12 @@ func (o *Options) Run() error { return nil default: - if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorDegraded); c != nil && c.Status == configv1.ConditionTrue { - prefix := "No upgrade is possible due to an error" - if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); c != nil && c.Status == configv1.ConditionTrue && len(c.Message) > 0 { - prefix = c.Message - } - if len(c.Message) > 0 { - return fmt.Errorf("%s:\n\n Reason: %s\n Message: %s\n\n", prefix, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + if c := findClusterOperatorStatusCondition(cv.Status.Conditions, clusterStatusFailing); c != nil { + if c.Status != configv1.ConditionFalse { + 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 ")) } - return fmt.Errorf("The cluster can't be upgraded, see `oc describe clusterversion`") + } else { + fmt.Fprintf(o.ErrOut, "warning: No current %s info, see `oc describe clusterversion` for more details.\n", clusterStatusFailing) } if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); c != nil && len(c.Message) > 0 { @@ -358,12 +362,12 @@ func (o *Options) Run() error { fmt.Fprintln(o.Out, c.Message) } } else { - fmt.Fprintln(o.ErrOut, "warning: No current status info, see `oc describe clusterversion` for more details") + fmt.Fprintf(o.ErrOut, "warning: No current %s info, see `oc describe clusterversion` for more details.\n", configv1.OperatorProgressing) } fmt.Fprintln(o.Out) if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); c != nil && c.Status == configv1.ConditionFalse { - fmt.Fprintf(o.Out, "Upgradeable=False\n\n Reason: %s\n Message: %s\n\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) + 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 ")) } if c := findClusterOperatorStatusCondition(cv.Status.Conditions, "ReleaseAccepted"); c != nil && c.Status != configv1.ConditionTrue { @@ -560,20 +564,27 @@ func findClusterOperatorStatusCondition(conditions []configv1.ClusterOperatorSta func checkForUpgrade(cv *configv1.ClusterVersion) error { results := []string{} if c := findClusterOperatorStatusCondition(cv.Status.Conditions, "Invalid"); c != nil && c.Status == configv1.ConditionTrue { - results = append(results, fmt.Sprintf("the cluster version object is invalid, you must correct the invalid state first:\n\n Reason: %s\n Message: %s\n\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) + results = append(results, fmt.Sprintf("the cluster version object is invalid, you must correct the invalid state first:\n\n Reason: %s\n Message: %s", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) } - if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorDegraded); c != nil && c.Status == configv1.ConditionTrue { - results = append(results, fmt.Sprintf("the cluster is experiencing an upgrade-blocking error:\n\n Reason: %s\n Message: %s\n\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) + if c := findClusterOperatorStatusCondition(cv.Status.Conditions, clusterStatusFailing); c != nil && c.Status == configv1.ConditionTrue { + target := cv.Status.Desired.Version + if target == "" { + target = cv.Status.Desired.Image + if target == "" { + target = "ClusterVersion status.desired has neither a version nor image; please open a bug" + } + } + results = append(results, fmt.Sprintf("the cluster is experiencing an error reconciling %q:\n\n Reason: %s\n Message: %s", target, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) } if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); c != nil && c.Status == configv1.ConditionTrue { - results = append(results, fmt.Sprintf("the cluster is already upgrading:\n\n Reason: %s\n Message: %s\n\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) + results = append(results, fmt.Sprintf("the cluster is already upgrading:\n\n Reason: %s\n Message: %s", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n "))) } if len(results) == 0 { return nil } - return errors.New(strings.Join(results, "")) + return errors.New(strings.Join(results, "\n\n")) } // targetMatch returns true if the target release matches the target diff --git a/pkg/cli/admin/upgrade/upgrade_test.go b/pkg/cli/admin/upgrade/upgrade_test.go index 6c8d85ef30..0c3a919917 100644 --- a/pkg/cli/admin/upgrade/upgrade_test.go +++ b/pkg/cli/admin/upgrade/upgrade_test.go @@ -48,26 +48,33 @@ func TestCheckForUpgrade(t *testing.T) { Reason: "SomeReason", Message: "Some message.", }}, - expected: errors.New("the cluster version object is invalid, you must correct the invalid state first:\n\n Reason: SomeReason\n Message: Some message.\n\n"), + expected: errors.New("the cluster version object is invalid, you must correct the invalid state first:\n\n Reason: SomeReason\n Message: Some message."), }, { - name: "degraded and progressing", + name: "failing and progressing", conditions: []configv1.ClusterOperatorStatusCondition{{ Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Reason: "RollingOut", Message: "Updating to v2.", }, { - Type: configv1.OperatorDegraded, + Type: clusterStatusFailing, Status: configv1.ConditionTrue, Reason: "BadStuff", Message: "The widgets are slow.", }}, - expected: errors.New("the cluster is experiencing an upgrade-blocking error:\n\n Reason: BadStuff\n Message: The widgets are slow.\n\nthe cluster is already upgrading:\n\n Reason: RollingOut\n Message: Updating to v2.\n\n"), + expected: errors.New("the cluster is experiencing an error reconciling \"4.1.0\":\n\n Reason: BadStuff\n Message: The widgets are slow.\n\nthe cluster is already upgrading:\n\n Reason: RollingOut\n Message: Updating to v2."), }, } { t.Run(testCase.name, func(t *testing.T) { - clusterVersion := &configv1.ClusterVersion{} + clusterVersion := &configv1.ClusterVersion{ + Status: configv1.ClusterVersionStatus{ + Desired: configv1.Release{ + Version: "4.1.0", + Image: "quay.io/openshift-release-dev/ocp-release@sha256:b8307ac0f3ec4ac86c3f3b52846425205022da52c16f56ec31cbe428501001d6", + }, + }, + } clusterVersion.Status.Conditions = testCase.conditions actual := checkForUpgrade(clusterVersion) if !reflect.DeepEqual(actual, testCase.expected) {