Only set 5s connect timeout in gen_cluster tests

[gjoseph92]

This subtly refactors (and renames) the use of the _reconfigure contextmanager, which is used to set some dask config defaults. Rather than calling _reconfigure within clean, we separate it out (since its job is different) and call it manually in the gen_cluster, gen_test, and loop fixtures.

We also remove the "distributed.comm.timeouts.connect": "5s" default timeout, except for in gen_cluster, since that's the only test setup where we can be confident the scheduler isn't running in a subprocess (which could take >5s to start). I'm not even sure what the value is in keeping this timeout for gen_cluster, but it shouldn't hurt either, so I left it for now.

See explanation in #6731 (comment).

• Passes pre-commit run --all-files

Closes #6731

• Closes Flaky test_dashboard #6751
• Closes Flaky test_dashboard_non_standard_ports #6752
• Closes distributed/cli/tests/test_dask_scheduler.py::test_dashboard_port_zero #6395
• Closes Flaky test_defaults #6753
• Closes Flaky test_multiple_workers #6754
• Closes Flaky test_multiple_workers_2 #6755
• Closes Flaky test_signal_handling[Signals.SIGTERM] #6759
• Closes Flaky test_signal_handling[Signals.SIGTERM] #6760
• Closes Flaky test_error_during_startup[--nanny] #6761
• Closes Flaky test_error_during_startup[--no-nanny] #6762
• Closes Flaky test_scheduler_address_env #6763
• Closes Flaky test_nanny #6764
• Closes Flaky test_separate_key_cert #6765
• Closes Flaky test_queue_in_task #6773

[github-actions]

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 40m 30s ⏱️ + 6m 23s
  2 989 tests +1    2 899 ✔️ +1       88 💤 ±0  2 ❌ ±0 
22 165 runs  +8  21 118 ✔️ +7  1 044 💤 ±0  3 ❌ +1 

For more details on these failures, see this check.

Results for commit 5303f90. ± Comparison against base commit 4f6960a.

♻️ This comment has been updated with latest results.

[hendrikmakait]

LGTM, great detective work!

[hendrikmakait]

nit: I'm not a fan of this name since it sounds like this would be the default for basically all tests, but then again I currently lack a better one. If you happen to have an idea for a more descriptive name, I'd be happy, otherwise, leave it as is.

[gjoseph92]

@hendrikmakait I renamed to config_for_cluster_tests and added a little docstring, what do you think?

[gjoseph92]

I'm looking into test_submit_after_failed_worker_sync. The test itself seems to be passing, but it's failing exiting the cluster contextmanager.

[fjetter]

FYI I was working on a similar change before w/out final conclusion #5791 I originally proposed to just lower the timeout for local test execution.

We took this as an opportunity to test the extreme case of large timeouts to configure tests better. I never finished this, though

[gjoseph92]

These changes have made test_submit_after_failed_worker_sync flaky. I think they would do the same for test_gather_after_failed_worker.

The problem is that the RPC's timeout is now longer than the asyncio.wait_for timeout in

distributed/distributed/utils_test.py

Lines 789 to 802 in 1db1d9f

	async def do_disconnect():
	logger.info(f"Disconnecting {addr}")
	async with rpc(addr, **rpc_kwargs) as w:
	logger.info(f"Disconnecting {addr} - RPC connected")
	# If the worker was killed hard (e.g. sigterm) during test runtime,
	# we do not know at this point and may not be able to connect
	with suppress(EnvironmentError, CommClosedError):
	# Do not request a reply since comms will be closed by the
	# worker before a reply can be made and we will always trigger
	# the timeout
	await w.terminate(reply=False)
	logger.info(f"Disconnecting {addr} - sent terminate")
	await asyncio.wait_for(do_disconnect(), timeout=timeout)

Before, the RPC would time out after 5s, and be ignored by the suppress(CommClosedError). Now, the wait_for times out after 20s, but the RPC is still trying to connect for 30s. So an asyncio.TimeoutError comes out of the wait_for, failing the test.

I could fix this in this PR by just passing the timeout into the RPC:

diff --git a/distributed/utils_test.py b/distributed/utils_test.py
index 8c7ddd27..50c3044c 100644
--- a/distributed/utils_test.py
+++ b/distributed/utils_test.py
@@ -788,7 +788,7 @@ async def disconnect(addr, timeout=3, rpc_kwargs=None):
 
     async def do_disconnect():
         logger.info(f"Disconnecting {addr}")
-        async with rpc(addr, **rpc_kwargs) as w:
+        async with rpc(addr, timeout=timeout * 0.9, **rpc_kwargs) as w:
             logger.info(f"Disconnecting {addr} - RPC connected")
             # If the worker was killed hard (e.g. sigterm) during test runtime,
             # we do not know at this point and may not be able to connect

However, IMO trying to open an RPC while the subprocess may be shutting down is silly, when we have POSIX signals at our disposal. I'm removing the RPC approach entirely in #6829.

(With the debug logs I added, you can see that cluster is trying to shut down both workers, including the one that just got SIGTERM'd. The RPC keeps trying to connect to the dead worker, but the asyncio.wait_for times out before the RPC times out.)

---

So (after removing some debug code) I'd like to merge this, then #6829 separately (in any order).

[gjoseph92]

Looking at test_worker_who_has_clears_after_failed_connection now

[gjoseph92]

That test has a race condition, fixed in #6832 and unrelated to this PR.

I think this is ready to go. Any final 👍 👎 ?

[fjetter]

Thanks @gjoseph92! I hope this will do the trick! 🤞

[graingert]

there are tests that depended on def test_example(cleanup): also calling _reconfigure

what should these tests do instead?