pkg/cvo/sync_worker: Consolidate all ClusterOperator errors by reason #577
Conversation
openshift-ci
bot
added
the
approved
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
8 times, most recently
from
4c92b1a to
a518161
Compare
|
/uncc |
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
2 times, most recently
from
6beb1d6 to
7e743a6
Compare
openshift-ci
bot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
LGTM but needs rebase. /remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
from
7e743a6 to
2c890a7
Compare
openshift-ci
bot
removed
the
needs-rebase
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
2 times, most recently
from
1807269 to
dd62b5b
Compare
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
Things like API-server connection errors and patch conflicts deserve some retries before we bubble them up into ClusterVersion conditions. But when we are able to retrieve in-cluster objects and determine that they are not happy, we should exit more quickly so we can complain about the resource state and start in on the next sync cycle. For example, see the recent e02d148 (pkg/cvo/internal/operatorstatus: Replace wait-for with single-shot "is it alive now?", 2021-05-12, openshift#560) and the older cc9292a (lib/resourcebuilder: Replace wait-for with single-shot "is it alive now?", 2020-07-07, openshift#400). This commit uses the presence of an UpdateError as a marker for "fail fast; no need to retry". The install-time backoff is from fee2d06 (sync: Completely parallelize the initial payload, 2019-03-11, openshift#136). I'm not sure if it really wants the same cap as reconcile and update modes, but I've left them the same for now, and future commits to pivot the backoff settings can focus on motivating those pivots. I'd tried dropping: backoff := st.Backoff and passing st.Backoff directly to ExponentialBackoffWithContext, but it turns out that Step() [1]: ... mutates the provided Backoff to update its Steps and Duration. Luckily, Backoff has no pointer properties, so storing as a local variable is sufficient to give us a fresh copy for the local mutations. [1]: https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#Backoff.Step
…ssing message For example, in a recent test cluster: $ oc get -o json clusterversion version | jq -r '.status.conditions[] | .lastTransitionTime + " " + .type + "=" + .status + " " + (.reason // "-") + ": " + (.message // "-")' 2021-05-26T20:18:35Z Available=True -: Done applying 4.8.0-0.ci-2021-05-26-172803 2021-05-26T21:58:16Z Failing=True ClusterOperatorNotAvailable: Cluster operator machine-config is not available 2021-05-26T21:42:31Z Progressing=False ClusterOperatorNotAvailable: Error while reconciling 4.8.0-0.ci-2021-05-26-172803: the cluster operator machine-config has not yet successfully rolled out 2021-05-26T19:53:47Z RetrievedUpdates=False NoChannel: The update channel has not been configured. 2021-05-26T21:26:31Z Upgradeable=False DegradedPool: Cluster operator machine-config cannot be upgraded between minor versions: One or more machine config pools are degraded, please see `oc get mcp` for further details and resolve before upgrading But the "has not yet successfully rolled out" we used for Progressing is much less direct than "is not available" we used for Failing. 6c7cd99 (pkg/payload/task: Fail fast for UpdateError, 2021-05-24, openshift#573) removed the last important abuse of ClusterOperatorNotAvailable (for failures to client.Get the target ClusterOperator). In this commit I'm moving the remaining abuser over to a new ClusterOperatorNoVersions. So the only remaining ClusterOperatorNotAvailable really means "not Available=True", and we can say that more directly.
To make it easier to understand what each case is about.
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
from
dd62b5b to
098a037
Compare
openshift-merge-robot
removed
the
needs-rebase
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
2 times, most recently
from
59b65d4 to
fe9cad3
Compare
newClusterOperatorsNotAvailable is from c2ac20f (status: Report the operators that have not yet deployed, 2019-04-09, openshift#158). And the not-available filtering is from bdd4545 (status: Hide generic operator status in favor of more specific errors, 2019-05-19, openshift#192). But in ce1eda1 (pkg/cvo/internal/operatorstatus: Change nested message, 2021-02-04, openshift#514), we moved "waiting on status.versions" from ClusterOperatorNotAvailable to ClusterOperatorUpdating. And we want to avoid uncollapsed errors like: Multiple errors are preventing progress: * Cluster operator machine-api is updating versions * Cluster operator openshift-apiserver is updating versions where we are waiting on multiple ClusterOperator which are in similar situations. This commit drops the filtering, because cluster operators are important. It does sort those errors to the end of the list though, so the first error is the non-ClusterOperator error. TestCVO_ParallelError no longer tests the consolidated error message, because the consolidation is now restricted to ClusterOperator resources. I tried moving the pkg/cvo/testdata/paralleltest/release-manifests manifests to ClusterOperator, but then the test struggled with: I0802 16:04:18.133935 2005 sync_worker.go:945] Unable to precreate resource clusteroperator so now TestCVO_ParallelError is excercising the fact that non-ClusterOperator failures are not aggregated.
wking
force-pushed
the
ClusterOperatorUpdating-aggregation
branch
from
fe9cad3 to
28df4d9
Compare
|
/test e2e-agnostic-upgrade |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, 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. |
|
/override ci/prow/e2e-agnostic-upgrade |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade 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. |
|
/hold I'm taking a closer look at the error. |
openshift-ci
bot
added
the
do-not-merge/hold
|
Looks like I didn't get my hold on in time. But same error in a parallel pull discussed here, so I'm back to being confident that I'm not introducing this failure mode with this PR. /hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
6 participants
Builds on #575; you may want to review that first.
newClusterOperatorsNotAvailableis from c2ac20f (#158). And the not-available filtering is from bdd4545 (#192). But in ce1eda1 (#514), we moved "waiting onstatus.versions" fromClusterOperatorNotAvailabletoClusterOperatorUpdating. And we want to avoid uncollapsed errors like:where we are waiting on multiple ClusterOperator which are in similar situations. This commit drops the filtering, because cluster operators are important. It does sort those errors to the end of the list though, so the first error is the non-ClusterOperator error.