Always pick an open port when running tests #6591

fjetter · 2022-06-16T17:31:56Z

This PR adds pytest fixtures that ensure a port being used in a test is free (no 100% guarantee)

I added logic to enable this even for concurrent runs using pytest-xdist, i.e. this is a step towards

github-actions · 2022-06-16T20:34:20Z

Unit Test Results

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

      15 files +      6       15 suites +6 6h 56m 15s ⏱️ + 3h 26m 4s
  2 882 tests +      3   2 795 ✔️ +    11   84 💤 -   11 3 ❌ +3
21 352 runs +8 648 20 384 ✔️ +8 227 965 💤 +418 3 ❌ +3

For more details on these failures, see this check.

Results for commit 9620479. ± Comparison against base commit 4239aed.

♻️ This comment has been updated with latest results.

distributed/cli/tests/test_dask_scheduler.py

distributed/utils_test.py

crusaderky · 2022-06-20T17:45:25Z

distributed/utils_test.py

+def free_port(global_port_lock, name_of_test):
+    with _get_open_port(global_port_lock) as port:
+        print(f"Using free port {port} for test {name_of_test}")
+        yield str(port)


why do you need to change it to str?

https://pypi.org/project/ephemeral-port-reserve/

crusaderky · 2022-06-20T17:46:12Z

+1 I like the idea behind this

graingert · 2022-06-21T15:01:48Z

distributed/utils_test.py

+def free_port(global_port_lock, name_of_test):
+    with _get_open_port(global_port_lock) as port:
+        print(f"Using free port {port} for test {name_of_test}")
+        yield str(port)


https://pypi.org/project/ephemeral-port-reserve/

fjetter

I removed the file locking nonsense again since it didn't work reliably, at least not with tmpfiles.
If we choose to go for pytest-xdist we might just want to skip the tests that require default ports or ensure they are all scheduled on the same xdist worker

fjetter · 2022-06-21T18:15:10Z

distributed/utils.py


-def open_port(host=""):
-    """Return a probably-open port
+def open_port(host="", port=0):


the version of ephemeral-port-reserve seems to be pretty close to what we had but much more sophisticated and likely better tested. Since open_port is something that might've been used elsewhere I decided to vendor the code.

That version didn't work on windows so I reverted to the old one.

fjetter · 2022-06-22T12:12:25Z

I replaced most occurrences of hard coded ports (there are still plenty in test_core.py)

distributed/utils.py

graingert · 2022-06-22T13:13:00Z

distributed/cli/tests/test_dask_scheduler.py

+            "--port",
+            str(port1),


nit: I find f-strings marginally neater here

Suggested change

"--port",

str(port1),

f"--port={port1}",

agreed. I was not entirely sure if our popen would handle this properly but it does 🎉

I changed most occurrences with some regex replacements. might have missed a few but will not go back for this

distributed/utils_test.py

fjetter · 2022-06-22T17:59:51Z

distributed/cli/tests/test_dask_scheduler.py

 @pytest.mark.skipif(WINDOWS, reason="POSIX only")
 @pytest.mark.parametrize("sig", [signal.SIGINT, signal.SIGTERM])
 def test_signal_handling(loop, sig):
+    port = open_port()
    with subprocess.Popen(
-        ["python", "-m", "distributed.cli.dask_scheduler"],
+        [
+            "python",
+            "-m",
+            "distributed.cli.dask_scheduler",
+            f"--port={port}",
+            "--dashboard-address=:0",
+        ],
        stdout=subprocess.PIPE,
        stderr=subprocess.STDOUT,
    ) as scheduler:
        # Wait for scheduler to start
-        with Client(f"127.0.0.1:{Scheduler.default_port}", loop=loop) as c:
+        with Client(f"127.0.0.1:{port}", loop=loop) as c:
            pass
        scheduler.send_signal(sig)
        stdout, stderr = scheduler.communicate()


I believe this test is causing a lot of cascading failures by blocking ports after not shutting down properly. I notice we're not using the custom popen (which I'm OK with) but I'm wondering why this is not shutting down properly. Anyhow, with these port changes this should be more robust

fjetter · 2022-06-23T09:52:13Z

On OSX 3.10 I still get a connection refused error but this is likely caused by a conflict in the dashboard port, see also #6612

FAILED distributed/cli/tests/test_dask_scheduler.py::test_defaults - OSError:...
FAILED distributed/cli/tests/test_dask_scheduler.py::test_dashboard - OSError...

fjetter requested review from crusaderky and gjoseph92 June 16, 2022 17:31

fjetter mentioned this pull request Jun 16, 2022

Do not log in signal handler #6590

Merged

fjetter force-pushed the consistent_ports_dask_schdeuler_cli branch from 012ba28 to 5b4e280 Compare June 16, 2022 18:07

crusaderky reviewed Jun 20, 2022

View reviewed changes

distributed/cli/tests/test_dask_scheduler.py Outdated Show resolved Hide resolved

crusaderky reviewed Jun 20, 2022

View reviewed changes

distributed/utils_test.py Outdated Show resolved Hide resolved

crusaderky reviewed Jun 20, 2022

View reviewed changes

fjetter requested a review from graingert June 21, 2022 15:00

graingert requested changes Jun 21, 2022

View reviewed changes

fjetter commented Jun 21, 2022

View reviewed changes

fjetter changed the title ~~WIP / RFC Add fixtures to ensure free ports are picked~~ Always pick an open port when running tests Jun 22, 2022

fjetter force-pushed the consistent_ports_dask_schdeuler_cli branch from 771342c to 0d9bb03 Compare June 22, 2022 09:30

fjetter marked this pull request as ready for review June 22, 2022 11:53

graingert reviewed Jun 22, 2022

View reviewed changes

distributed/utils.py Outdated Show resolved Hide resolved

graingert reviewed Jun 22, 2022

View reviewed changes

distributed/utils_test.py Outdated Show resolved Hide resolved

graingert approved these changes Jun 22, 2022

View reviewed changes

fjetter added 9 commits June 22, 2022 18:52

Add fixtures to ensure free ports are picked

28df483

use nullctx

768b816

Use ephemeral-port-reserve-code

3610b8f

Revert changes to open_port

6537d5c

Fix more tests

9a3ebad

Eliminate all occurences of hard-coded port 8786

f52d3d1

More fixes

8a3bc7d

Use contextlib.closing

cf914b1

code review

afe006d

fjetter force-pushed the consistent_ports_dask_schdeuler_cli branch from 42f1a57 to afe006d Compare June 22, 2022 16:53

Add fix for test_signal_handling

56b0073

fjetter mentioned this pull request Jun 22, 2022

Default dashboard address configurable #6612

Closed

fix linting

edbaac5

fjetter commented Jun 22, 2022

View reviewed changes

Fix test_listen_address_ipv6

9620479

fjetter mentioned this pull request Jun 23, 2022

Warn unreachable for scheduler.py #6611

Merged

fjetter merged commit 7a0649a into dask:main Jun 23, 2022

fjetter deleted the consistent_ports_dask_schdeuler_cli branch June 23, 2022 09:52

fjetter mentioned this pull request Jun 23, 2022

Remove unused __started Event in Server #6615

Merged

crusaderky mentioned this pull request Jun 24, 2022

Update CI stability #6625

Open

Uh oh!

Always pick an open port when running tests #6591

Always pick an open port when running tests #6591

Uh oh!

Conversation

fjetter commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Uh oh!

Uh oh!

crusaderky Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

graingert Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Jun 20, 2022

Uh oh!

graingert Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

fjetter left a comment

Choose a reason for hiding this comment

Uh oh!

fjetter Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

fjetter Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

fjetter commented Jun 22, 2022

Uh oh!

Uh oh!

graingert Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjetter Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graingert Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fjetter Jun 22, 2022

Choose a reason for hiding this comment

Uh oh!

fjetter commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fjetter commented Jun 16, 2022 •

edited

Loading

github-actions bot commented Jun 16, 2022 •

edited

Loading

graingert Jun 22, 2022 •

edited

Loading

fjetter Jun 22, 2022 •

edited

Loading