2.5 worker lease 1815471

[jameinel]

Description of change

Fix at least 5 flaky tests around Lease behavior.

• The Manager.setupInitialTimer was allowing the test loops to think that setup had finished, but really it was only that setup had started and had done the initial NewTimer() call.
• Timer.Reset() is unsafe to call without first Stopping the timer according to the time docs. (race condition with the trigger firing.)
• Several tests were only passing because they exited the main loop 'fast enough'. They wanted to test that we weren't expiring a claim that got a longer-than-requested duration. However, because Claim doesn't actually tell you how long the Lease was given for, unless we did an extra lookup we can't actually set the timeout trivially. And we want to avoid lookups on every Extend. (And in practice we don't give out arbitrarily longer claims.) Update the tests to sync with the expected Refresh and note that nothing gets expired.
• Also test the fact that it will expire the Lease at the appropriate times.

QA steps

To trigger the race in setupInitialTimer you could use this patch:

--- a/worker/lease/manager.go
+++ b/worker/lease/manager.go
@@ -437,6 +437,7 @@ func (manager *Manager) setupInitialTimer() {
        manager.muNextTimeout.Lock()
        manager.timer = manager.config.Clock.NewTimer(manager.config.MaxSleep)
        manager.muNextTimeout.Unlock()
+       time.Sleep(0 * time.Millisecond)
        // lastTick has never happened, so pass in the epoch time
        manager.computeNextTimeout(time.Time{}, manager.config.Store.Leases())
 }

That extends the chance that the test loop will sync on the NewTimer and not on the timer.Reset that is immediately called after that.

For all the tests, the easiest way to see the flakiness is:

$ go test -race -count=2000 -failfast -test.timeout=20m -check.v
$ go test -race -count=2000 -failfast -test.timeout=20m -check.v -check.f Expiry

With this patch, I didn't get any failures after 2000 loops. (though, as always, there could still be issues.)

Documentation changes

None.

Bug reference

https://bugs.launchpad.net/juju/+bug/1815471

[SimonRichardson]

Tested with the old code with time.Sleep and can induce race, then tested new code and can't induce race.

[SimonRichardson]

nice 👏

[SimonRichardson]

Good change, the docs read like a minefield.

[jameinel]

$$merge$$

[jameinel]

$$merge$$
fix another flaky test around SlowPeriodSuite.TestWatchRemoveBeforeKnown.
It seemed to think that you would never see the command that was just executed, but I don't think that is guaranteed in any way.