Bug 1830390: pkg/operator: clear Degraded on task's success #1598
Conversation
The MCO runs a bunch of sync functions during its sync. Each of these functions have a name (a task name). When we loop through the sync functions and fail on any of them, the MCO breaks and reports the Degraded status for that task as Reason: TaskNameFailed. It then goes and retry all the sync functions (no change in behavior till here). During some debugging and testing I've noticed that the RenderConfig task failed with the version mismatch error for osImageURL. That is good, and also right. The MCO went ahead and set the Degraded condition to RenderConfigFailed (correctly!). The MCO then went again and re-tried all the sync functions. Now the RenderConfig task succeeded (the version mismatch error can indeed be temporary). What happened from now on is that the MCO kept rolling and eventually finishes the upgrade I was running but the RenderConfigFailed Degraded status was never cleared back to False even if the task _did_ succeed. This patch introduces a new function which we call after every sync function in order to clear any previous degraded status when it instaed succeeded on later syncs. Signed-off-by: Antonio Murdaca <runcom@linux.com>
openshift-ci-robot
added
the
approved
|
/retest |
|
@runcom: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/skip |
|
looks sane, will let other MCO folks for second opinion |
| return nil | ||
| } | ||
| if degradedStatusCondition.Reason != task+"Failed" { | ||
| return nil |
There was a problem hiding this comment.
is there anyway a legit degraded message will not have Failed at the end?
There was a problem hiding this comment.
would it be better to just check that task is contained in Reason? (I dunno, I'm wondering?)
There was a problem hiding this comment.
We can only clear out Degraded conditions coming from TaskName+Failed as we standardize that naming. The other errors cannot be realistically cleared here - maybe I can tackle that next but I'm not optimistic in general, this will make the trick for conditions we know about for now
There was a problem hiding this comment.
Gotcha! Thanks for explaining =)
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kikisdeliveryservice, runcom, sinnykumari The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kikisdeliveryservice
changed the title
|
@runcom: All pull requests linked via external trackers have merged: . Bugzilla bug 1830390 has been moved to the MODIFIED state. In 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. |
The MCO runs a bunch of sync functions during its sync. Each of these functions has
a name (a task name). When we loop through the sync functions and fail on any of them,
the MCO breaks and reports the Degraded status for that task as Reason: TaskNameFailed.
It then goes and retries all the sync functions (no change in behavior till here).
During some debugging and testing I've noticed that the RenderConfig task failed with the
version mismatch error for osImageURL. That is good, and also right. The MCO went ahead
and set the Degraded condition to RenderConfigFailed (correctly!). The MCO then went again
and re-tried all the sync functions. Now the RenderConfig task succeeded (the version mismatch error
can indeed be temporary). What happened from now on is that the MCO kept rolling and eventually finishes
the upgrade I was running but the RenderConfigFailed Degraded status was never cleared back to False
even if the task did succeed (I had to wait for all the sync functions to finish as expected, and it takes a while still reporting degraded).
This patch introduces a new function which we call after every sync function in order to clear
any previous degraded status when it instead succeeded in later syncs.
Signed-off-by: Antonio Murdaca runcom@linux.com