Skip to content

[enterprise-4.12] Revert "OCPBUGS#9448: Add a note regarding checking clusterversion"#67259

Merged
michaelryanpeter merged 1 commit intoopenshift:enterprise-4.12from
skopacz1:65812_4.12CP
Nov 1, 2023
Merged

[enterprise-4.12] Revert "OCPBUGS#9448: Add a note regarding checking clusterversion"#67259
michaelryanpeter merged 1 commit intoopenshift:enterprise-4.12from
skopacz1:65812_4.12CP

Conversation

@skopacz1
Copy link
Copy Markdown
Contributor

@skopacz1 skopacz1 commented Nov 1, 2023

Cherry picked from c71ce8f xref:#65812.

This reverts commit fb0d246, openshift#57136.

The commit message did not explain the motivation, but [1] has:

  One note is:
  "
  After the update completes, you can confirm that the cluster version has updated to the new version:
  $ oc get clusterversion
  "

  The thing is that if the upgrade didn't complete, this output may include errors, for example:

  " oc get clusterversion
  NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
  version 4.10.26 True True 24m Unable to apply 4.11.0-rc.7: an unknown error has occurred: MultipleErrors
  "

  We should include a note to disregard these errors as long as the "Progressing" is in True.

But 1143cbe (Removed jq commands and replaced with oc adm upgrade,
2023-01-20, openshift#55005) was moving us from 'oc get clusterversion' towards
'oc adm upgrade' for "show my current context".  Both get
ClusterVersion and print a summary of its current status to the
console.  'oc get ...' is generic CustomResourceDefinition rendering
and the only knobs are things like additionalPrinterColumns, because
'get' needs to work if there are multiple matching resources and fit
them each into one line of text.  We have more control over the
rendering in 'oc adm upgrade', where we know we only care about the
ClusterVersion named 'cluster', and can take as many lines as we need
to explain the details of the current cluster state.  So basically
there's no upside to using 'oc get clusterversion', unless you are
using '-o json' or something and piping the results into a robot for
structured consumption.  For direct human consumption, 'oc adm
upgrade' will always be better, and in this commit, I'm moving us back
to using 'oc adm upgrade'.  This also gets the intervening 'Cluster
version is <version>' example output back to making sense, since
fb0d246 failed to update that example output when it pivoted
the suggested command.

I'm also dropping the "you can ignore the error" line.  While there
are certainly cases when cluster components ask for admin intervention
(e.g. by setting Available=False in a ClusterOperator) despite admin
intervention not actually being needed, it is absolutely not a good
idea to ignore those in general.  Cases:

* Cluster claims it is happy...
  * ... and it actually is happy.  Hooray!

  * ... but it is actually unhealthy.  Worth a bug to the overly
    optimistic component about more accurately reporting that it is
    unhealthy, and should be calling for admin intervention.

* Cluster claims it is unhealthy...
  * ... and it actually is unhealthy.  Sad.  But at least it is
    appropriately calling for help.  Error messages should be
    actionable, so the admin can quickly identify and resolve the
    issue, and if not, it is worth a bug to the opaquely-failing
    component about more helpfully guiding the admin through
    triage and resolution.

  * ... but it is actually pretty healthy.  This is the case where the
    admin could ignore the error message.  But it is hard to
    distinguish from the "actually unhealthy" cases where the admin
    should not ignore the error message.  Certainly:

      an unknown error has occurred: MultipleErrors

    is insufficient data to decide that the error report is
    false-positive noise.  Worth digging into the source of the error
    report, and a bug to the noisy component about not calling for
    admin assistance when no intervention is actually needed.

There will probably always be some corner cases where the cluster does
not ask for help despite needing help, or the cluster asks for help
despite not needing help, but as the controllers get smarter, those
cases should become more and more rare, to the point where we should
not need to discuss them in docs beyond generic comments pointing out
that no real-world diagnostic system is completely free of
false-positive and false-negative risk.

[1]: https://issues.redhat.com/browse/OCPBUGS-9448
@openshift-ci openshift-ci Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 1, 2023
@ocpdocs-previewbot
Copy link
Copy Markdown

@michaelryanpeter michaelryanpeter changed the title [enterprise-4.12] Manual CP of #65812 [enterprise-4.12] Revert "OCPBUGS#9448: Add a note regarding checking clusterversion" Nov 1, 2023
@michaelryanpeter michaelryanpeter merged commit 193f037 into openshift:enterprise-4.12 Nov 1, 2023
@skopacz1 skopacz1 deleted the 65812_4.12CP branch March 11, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants