panic: Session already closed in provisioner tests

[Aaron Bentley]

This may be related to 1305014. We have seen it in two recent test runs:
http://reports.vapour.ws/releases/issue/55565034749a5650fa59f34c

[Cheryl Jennings (cherylj)]

The latest matches in CI are in state/presence and are covered in bug #1588574

[Cheryl Jennings (cherylj)]

The latest matches are for a different failure (see comment #2). Can the regex for the outcome analyzer be updated?

[Aaron Bentley (abentley)]

It's not clear to me that there's a useful difference in the output. Looking at two examples:

http://reports.vapour.ws/releases/2506/job/run-unit-tests-trusty-ppc64el/attempt/2701#highlight
http://reports.vapour.ws/releases/4031/job/run-unit-tests-centos7-amd64/attempt/952#highlight

Both say "panic: Session already closed", and seem to trace back to NewWatcher. How would a regex tell them apart?

[Cheryl Jennings (cherylj)]

Both of the examples you gave are for the state/presence failure. The title of this bug implies the original failures were seen in worker/provisioner, and I see that in this run, the failure was in worker/provisioner: http://reports.vapour.ws/releases/2601/job/run-unit-tests-win2012-amd64/attempt/338

TestProvisionerRetriesTransientErrors is implicated in the trace back on that run.

Maybe also specifying the failing package would help differentiate?

[John A Meinel (jameinel)]

I looked at the failure from:
http://10.125.0.203:8080/job/RunUnittests-s390x/277/console

Which appears to have the fix that was landed for:
https://bugs.launchpad.net/juju/+bug/1754021

namely, we wait for 'hackyGoroutineDone' before we check m4.InstanceStatus which should mean that we won't tear down the connection for m3 until that goroutine is done.
However, a panic() could be obscuring the original error. Imagine that checkStartInstance failed with an error, which would then raise an exception, which would the never call close(thatsAllFolks) and never waits for hackyGoroutineDone.

[John A Meinel (jameinel)]

https://github.com/juju/juju/pull/8474

see also https://bugs.launchpad.net/juju/+bug/1754021

The above patch should make it so that we get to see the actual failure, rather than seeing a panic() during teardown.

[John A Meinel (jameinel)]

We should now not get the panic() though the underlying test failure is probably still there.

[John A Meinel (jameinel)]

https://github.com/juju/juju/pull/8482

[Anastasia (anastasia-macmood)]

Since this code landed in 2.3.5, I am pretty sure it is also present in 2.4 and subsequent branches. As this is also a test fix, I'll mark this as Fix Released.