OCPBUGS-3714: pkg/cli/admin/upgrade: Report on Failing!=False conditions#900
Conversation
|
@wking: This pull request references Bugzilla bug 1992680, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 kubernetes/test-infra repository. |
f42948c to
004261b
Compare
| } | ||
| 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", ClusterStatusFailing) |
There was a problem hiding this comment.
Why is this a warning? Also it should be s/No current %s info/ No information on current %s/
There was a problem hiding this comment.
It's a warning, because we should always be setting Failing. Ideally to Failing=False, but sometimes to Failing=True. A ClusterVersion with no Failing at all is a sign that something is probably wrong with the CVO.
...No current Failing info, see... sounds pretty clear to me. I'd be fine rephrasing to something like ...No current conditions with type Failing, see... here and below for Progressing if you prefer.
|
/bugzilla refresh The requirements for Bugzilla bugs have changed, recalculating validity. |
|
@openshift-merge-robot: This pull request references Bugzilla bug 1992680, which is invalid:
Comment 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 kubernetes/test-infra repository. |
004261b to
304931d
Compare
|
/bugzilla refresh |
|
@wking: This pull request references Bugzilla bug 1992680, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 kubernetes/test-infra repository. |
|
/bugzilla refresh The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity. |
|
@openshift-bot: This pull request references Bugzilla bug 1992680, which is invalid:
Comment 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 kubernetes/test-infra repository. |
304931d to
fa302a7
Compare
|
@wking Do you have a sample output of old code vs this PR? |
fa302a7 to
f95fea0
Compare
|
Before this PR, With this PR, it will look like: |
|
@wking: No Bugzilla bug is referenced in the title of this pull request. 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 kubernetes/test-infra repository. |
|
@wking: This pull request references Jira Issue OCPBUGS-3714, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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 kubernetes/test-infra repository. |
a1d6433 to
ea3dd70
Compare
|
pre-merge verified in https://issues.redhat.com/browse/OCPBUGS-3714#comment-21273892 |
ea3dd70 to
1900577
Compare
b6e8e43 to
13196b1
Compare
cae0b5e (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]: openshift/api#287 [2]: openshift/cluster-version-operator#191 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1988576#c30
13196b1 to
d071b82
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, wking 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 |
|
@wking: 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/test-infra repository. I understand the commands that are listed here. |
|
/test e2e-agnostic-cmd |
|
@wking: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-3714 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
Spun out of this comment.
cae0b5e (openshift/origin#22644) moved this code from Failing to Degraded, likely inspired by openshift/api#287. But Degraded is only used in ClusterOperator. ClusterVersion kept using Failing, as seen in openshift/cluster-version-operator#191. 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=Truemay 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 (one example).