Retry initialization error conditions #2979
Conversation
tmshort
force-pushed
the
OCPBUGS-13128
branch
4 times, most recently
from
48f8cdc
to
3de9dc2
Compare
| @@ -78,7 +78,7 @@ func TestOperatorRunChannelClosure(t *testing.T) { | |||
|
|
|||
| o.Run(ctx) | |||
|
|
|||
| timeout := time.After(time.Second) | |||
| timeout := time.After(2 * time.Minute) | |||
There was a problem hiding this comment.
is this meant to represent 2*defaultServerVersionInterval ?
There was a problem hiding this comment.
Yeah, it probably should be.
| select { | ||
| case <-time.After(defaultProbeInterval): | ||
| case <-stopCh: | ||
| return | ||
| } |
There was a problem hiding this comment.
Do we want to try initialization indefinitely?
There was a problem hiding this comment.
If this fails, then OLM is unable to monitor and update the packageserver cluster operator. After chatting with some people, it was decided to keep on retrying, rather than terminate the OLM process (which would restart the pod), which could look worse in metrics.
The other retry was added, but is restricted because it impacts unit tests.
| v, err := o.serverVersion.ServerVersion() | ||
| if err == nil { | ||
| o.logger.Infof("connection established. cluster-version: %v", v) | ||
| return | ||
| } | ||
| select { | ||
| case <-time.After(defaultServerVersionInterval): | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| v, err = o.serverVersion.ServerVersion() |
There was a problem hiding this comment.
So if I'm reading this correctly, we attempt to get the server version twice. If the first attempt fails, we retry after the defaultServerVersionInterval. If we ever succeed we return the serverVersion. Could you share your reasoning why we make two attempts here but infinite attempts above?
There was a problem hiding this comment.
Unit tests... the unit tests actually wait for this initialization to complete (success or failure), which is guaranteed to fail.
There was a problem hiding this comment.
(I would've liked to have had an infinite wait, but then the unit-tests timeout)
When the api server is flakey (e.g. during a cluster install), it is possible for some of the OLM initialization to fail. When this happens, OLM gets into a bad state (e.g. a monitoring go routine terminates) and can't recover without a restart. There were at least two places I found where a retry mechanism is needed to handle intialization errors. This was as far as I peeled the onion. It's not an exponential backoff retry, but a 1 minute retry interval should be sufficient (no other backoffs are exponential). The ServerVersion only retries once with a minute in between. This required fixing a unit-test to take the retry into account. Signed-off-by: Todd Short <todd.short@me.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ankitathomas, tmshort The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of the change:
When the api server is flakey (e.g. during a cluster install), it is possible for some of the OLM initialization to fail. When this happens, OLM gets into a bad state (e.g. a monitoring go routine terminates) and can't recover without a restart.
There were at least two places I found where a retry mechanism is needed to handle intialization errors. This was as far as I peeled the onion. It's not an exponential backoff retry, but a 1 minute retry interval should be sufficient (no other backoffs are exponential).
Motivation for the change:
Downstream bug report.
Architectural changes:
None.
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue