Prevent infinite transition loops; more aggressive validate_state()

[crusaderky]

Partially closes #6305

#6305 introduces an infinite loop in the worker transitions.
Such loop never releases the GIL,
which means that the @gen_cluster timeout doesn't work,
which means that after 5 minutes pytest_timeout kicks in,
which means that you get no logs and no cluster dump whatsoever.

This PR:

• Runs validate_state() at the end of every test for all workers, in addition to the scheduler. This excludes workers wrapped by nannies and workers not started by gen_cluster itself.
• Runs validate_state() on Scheduler and Workers in case of TimeoutError, hoping to get an error about invalid state instead of the much more opaque timeout message
• Implements a limit for the number of transitions a scheduler or a worker can go through before it breaks gracefully after at most 30s (gen_cluster timeout). When it happens, pytest records all logs and a full cluster dump is generated.

[github-actions]

Unit Test Results

       15 files   -        1         15 suites   - 1   7h 2m 0s ⏱️ - 27m 11s
  2 774 tests +       3    2 695 ✔️ +       4    78 💤  -     1  1 ❌ ±0 
20 580 runs   - 1 550  19 671 ✔️  - 1 439  908 💤  - 111  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 35a5568. ± Comparison against base commit 9f02e7a.

♻️ This comment has been updated with latest results.

[crusaderky]

https://github.com/dask/distributed/runs/6386832455?check_suite_focus=true now shows the issue in #6305
The other two test failures are unrelated.

Note: this PR is likely to increase flakiness, and I think it's a good thing, as it will crop up a lot of previously undetected issues which are likely to cause deadlocks somewhere else.

[gjoseph92]

Though a little odd to expose in top-level config, this seems like a good thing to have and great to have in all tests.

To be clear, this doesn't actually fix #6248 (comment), just causes it to be caught more obviously in tests?

[gjoseph92]

Might be nice if this raised something like a TransitionCounterMaxExceeded error, to be consistent with workers

[crusaderky]

yeah, but scheduler doesn't have anything like InvalidTransitionError on the worker

[crusaderky]

To be clear, this doesn't actually fix #6248 (comment), just causes it to be caught more obviously in tests?

Correct

[gjoseph92]

@crusaderky can we make the 1-line fix for #6305 now, or do you want to see this take effect in CI first to confirm it's working?

[crusaderky]

@crusaderky can we make the 1-line fix for #6305 now, or do you want to see this take effect in CI first to confirm it's working?

See #6327

[fjetter]

I would like to have a conversation about this. I think this limit should be removed again.

I don't see a huge value in it and would prefer having fewer configuration options, even for tests alone. Whenever we may write stress tests, etc. we'd need to adjust this limit (we can already see a few cases where the "default" limit appears to not be sufficient).

[fjetter]

I think we should try to not clutter this file with stuff we only use for internal tests. Apart from tests I don't see the usefulness of having a global limit on the number of transitions

[crusaderky]

Indeed, this is for tests only.