Skip to content

Commit

Permalink
upgrade/adminack: simplify polling and unblock "guaranteed" post-upgr…
Browse files Browse the repository at this point in the history
…ade check

openshift#27645 intended to add a guaranteed post-upgrade check but I have overlooked how exactly the polling is implemented and terminated, leading to the post-upgrade check never actually execute.

Previously the test used `PollImmediateWithContext` for the each-10-minutes check. The `ConditionFunc` never actually returned `true` or non-nil `err`, so the `PollImmediateWithContext` never terminated by the means of `ConditionFunc`: it was always terminated by the `ctx.Done()` that the framework does on finished upgrade (or a test timeout). This means that `PollImmediateWithContext` always terminated with `err=wait.ErrWaitTimeout` and the `Test` method immediately returned, so the "guaranteed" check code is never reached.

Given our `ConditionFunc` never terminates the polling, we can simplify and use the `wait.UntilWithContext` instead, which is a simpler version that precisely implements the desired loop (poll until context is done).
  • Loading branch information
petr-muller committed Jan 23, 2023
1 parent 2154d73 commit a1bd387
Showing 1 changed file with 2 additions and 7 deletions.
Expand Up @@ -51,19 +51,14 @@ func (t *AdminAckTest) Test(ctx context.Context) {
exercisedVersions := sets.NewString()
success := false
var lastError error
if err := wait.PollImmediateUntilWithContext(ctx, t.Poll, func(ctx context.Context) (bool, error) {
wait.UntilWithContext(ctx, func(ctx context.Context) {
if err := t.test(ctx, exercisedGates, exercisedVersions); err != nil {
framework.Logf("Retriable failure to evaluate admin acks: %v", err)
lastError = err
} else {
success = true
}
return false, nil
}); err == nil || err == wait.ErrWaitTimeout {
return
} else {
framework.Fail(err.Error())
}
}, t.Poll)

if !success {
framework.Failf("Never able to evaluate admin acks. Most recent failure: %v", lastError)
Expand Down

0 comments on commit a1bd387

Please sign in to comment.