From 0874a9b79bd906c2e1b14e4e048b5401fbf99f41 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Mon, 26 Feb 2024 14:43:01 +0100 Subject: [PATCH 1/9] Fix worker dashboard proxy --- distributed/http/proxy.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/distributed/http/proxy.py b/distributed/http/proxy.py index 02da2f34ae..29ca16e5fb 100644 --- a/distributed/http/proxy.py +++ b/distributed/http/proxy.py @@ -19,6 +19,15 @@ def initialize(self, dask_server=None, extra=None): self.scheduler = dask_server self.extra = extra or {} + # `get_current_user` and `prepare` method impls reference + # issue in tornado & jupyter server compat here + # https://github.com/jupyter-server/jupyter_server/issues/1012 + def get_current_user(self): + return "dask" + + async def prepare(self): + web.authenticated(lambda rq: None)(self) + async def http_get(self, port, host, proxied_path): # route here first # incoming URI /proxy/{port}/{host}/{proxied_path} @@ -29,6 +38,9 @@ async def http_get(self, port, host, proxied_path): uri = f"/proxy/{port}/{proxied_path}" self.request.uri = uri + if self.host not in self.host_allowlist: + self.host_allowlist.append(self.host) + # slash is removed during regex in handler proxied_path = "/%s" % proxied_path @@ -41,6 +53,8 @@ async def http_get(self, port, host, proxied_path): return await self.proxy(port, proxied_path) async def open(self, port, host, proxied_path): + if host not in self.host_allowlist: + self.host_allowlist.append(host) # finally, proxy to other address/port return await self.proxy_open(host, port, proxied_path) From 4ad2bbab1c562f2e363bd4f9dabad5b002b6bce5 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Tue, 27 Feb 2024 09:52:20 +0100 Subject: [PATCH 2/9] Refactor and setup to always test jupyter-server-proxy --- continuous_integration/environment-3.10.yaml | 1 + continuous_integration/environment-3.11.yaml | 1 + continuous_integration/environment-3.12.yaml | 1 + continuous_integration/environment-3.9.yaml | 1 + distributed/cli/tests/test_dask_worker.py | 43 ++++++++------------ 5 files changed, 21 insertions(+), 26 deletions(-) diff --git a/continuous_integration/environment-3.10.yaml b/continuous_integration/environment-3.10.yaml index c277491e8a..fbdb8ba675 100644 --- a/continuous_integration/environment-3.10.yaml +++ b/continuous_integration/environment-3.10.yaml @@ -18,6 +18,7 @@ dependencies: - ipykernel - ipywidgets - jinja2 + - jupyter-server-proxy - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.11.yaml b/continuous_integration/environment-3.11.yaml index dd2b150e0e..ae5e2df163 100644 --- a/continuous_integration/environment-3.11.yaml +++ b/continuous_integration/environment-3.11.yaml @@ -18,6 +18,7 @@ dependencies: - ipykernel - ipywidgets - jinja2 + - jupyter-server-proxy - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.12.yaml b/continuous_integration/environment-3.12.yaml index cb9124d883..75293fffda 100644 --- a/continuous_integration/environment-3.12.yaml +++ b/continuous_integration/environment-3.12.yaml @@ -18,6 +18,7 @@ dependencies: - ipykernel - ipywidgets - jinja2 + - jupyter-server-proxy - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.9.yaml b/continuous_integration/environment-3.9.yaml index d371026e95..c8b716086f 100644 --- a/continuous_integration/environment-3.9.yaml +++ b/continuous_integration/environment-3.9.yaml @@ -20,6 +20,7 @@ dependencies: - ipykernel - ipywidgets - jinja2 + - jupyter-server-proxy - locket - lz4 # Only tested here - msgpack-python diff --git a/distributed/cli/tests/test_dask_worker.py b/distributed/cli/tests/test_dask_worker.py index 34023e4c4d..ba01f7d6b2 100644 --- a/distributed/cli/tests/test_dask_worker.py +++ b/distributed/cli/tests/test_dask_worker.py @@ -485,38 +485,29 @@ def func(dask_worker): @pytest.mark.slow -@gen_cluster( - client=True, nthreads=[], scheduler_kwargs={"dashboard_address": "localhost:8787"} -) -async def test_dashboard_non_standard_ports(c, s, requires_default_ports): +def test_dashboard_non_standard_ports(): pytest.importorskip("bokeh") requests = pytest.importorskip("requests") - try: - import jupyter_server_proxy # noqa: F401 + host = "127.0.0.1" + s_port = "3233" + s_dashboard_port = "3232" + w_dashboard_port = "4833" + s_cmd = f"dask scheduler --host {host} --port {s_port} --dashboard-address :{s_dashboard_port}" + w_cmd = f"dask worker {host}:{s_port} --dashboard-address :{w_dashboard_port} --host {host}" - proxy_exists = True - except ImportError: - proxy_exists = False + with popen(s_cmd.split()): + with popen(w_cmd.split()): + with Client(f"{host}:{s_port}") as c: + c.wait_for_workers(1) - with popen( - [ - "dask", - "worker", - s.address, - "--dashboard-address", - ":4833", - "--host", - "127.0.0.1", - ] - ): - await c.wait_for_workers(1) + response = requests.get(f"http://{host}:{w_dashboard_port}/status") + response.raise_for_status() - response = requests.get("http://127.0.0.1:4833/status") - response.raise_for_status() - # TEST PROXYING WORKS - if proxy_exists: - response = requests.get("http://127.0.0.1:8787/proxy/4833/127.0.0.1/status") + # TEST PROXYING WORKS + response = requests.get( + f"http://{host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{host}/status" + ) response.raise_for_status() with pytest.raises(requests.ConnectionError): From b0a24cb6a59f92a20308f02103e974c694d9bbbc Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Tue, 27 Feb 2024 11:29:13 +0100 Subject: [PATCH 3/9] Ignore jupyter deprecation warning for platformdirs --- pyproject.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index f42887c490..b221dad13d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -110,6 +110,9 @@ addopts = ''' -p no:legacypath''' filterwarnings = [ "error", + # xref: https://github.com/jupyter/jupyter_core/pull/292 + # xref: https://github.com/jupyter/jupyter_core/issues/309 + '''ignore:Jupyter is migrating its paths to use standard platformdirs''', '''ignore:Please use `dok_matrix` from the `scipy\.sparse` namespace, the `scipy\.sparse\.dok` namespace is deprecated.:DeprecationWarning''', '''ignore:elementwise comparison failed. this will raise an error in the future:DeprecationWarning''', '''ignore:unclosed Date: Tue, 27 Feb 2024 13:27:27 +0100 Subject: [PATCH 4/9] Fix jupyter tests --- continuous_integration/environment-3.10.yaml | 1 + continuous_integration/environment-3.11.yaml | 1 + continuous_integration/environment-3.12.yaml | 1 + continuous_integration/environment-3.9.yaml | 1 + distributed/tests/test_jupyter.py | 10 ++++++---- 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/continuous_integration/environment-3.10.yaml b/continuous_integration/environment-3.10.yaml index fbdb8ba675..1288767469 100644 --- a/continuous_integration/environment-3.10.yaml +++ b/continuous_integration/environment-3.10.yaml @@ -19,6 +19,7 @@ dependencies: - ipywidgets - jinja2 - jupyter-server-proxy + - jupyterlab - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.11.yaml b/continuous_integration/environment-3.11.yaml index ae5e2df163..1ff635ac8c 100644 --- a/continuous_integration/environment-3.11.yaml +++ b/continuous_integration/environment-3.11.yaml @@ -19,6 +19,7 @@ dependencies: - ipywidgets - jinja2 - jupyter-server-proxy + - jupyterlab - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.12.yaml b/continuous_integration/environment-3.12.yaml index 75293fffda..cd787bea96 100644 --- a/continuous_integration/environment-3.12.yaml +++ b/continuous_integration/environment-3.12.yaml @@ -19,6 +19,7 @@ dependencies: - ipywidgets - jinja2 - jupyter-server-proxy + - jupyterlab - locket - msgpack-python - netcdf4 diff --git a/continuous_integration/environment-3.9.yaml b/continuous_integration/environment-3.9.yaml index c8b716086f..4ed60da0d8 100644 --- a/continuous_integration/environment-3.9.yaml +++ b/continuous_integration/environment-3.9.yaml @@ -21,6 +21,7 @@ dependencies: - ipywidgets - jinja2 - jupyter-server-proxy + - jupyterlab - locket - lz4 # Only tested here - msgpack-python diff --git a/distributed/tests/test_jupyter.py b/distributed/tests/test_jupyter.py index 7c2774062a..9bfa63c09c 100644 --- a/distributed/tests/test_jupyter.py +++ b/distributed/tests/test_jupyter.py @@ -10,6 +10,7 @@ from tornado.httpclient import AsyncHTTPClient from distributed import Client, Scheduler +from distributed.compatibility import MACOS, WINDOWS from distributed.core import Status from distributed.utils import open_port from distributed.utils_test import gen_test, popen @@ -73,7 +74,8 @@ async def test_jupyter_idle_timeout(): assert s.status not in (Status.closed, Status.closing) - await asyncio.sleep(s.idle_timeout) + # small bit of extra time to catch up + await asyncio.sleep(s.idle_timeout + 0.5) assert s.status in (Status.closed, Status.closing) @@ -94,12 +96,12 @@ async def test_jupyter_idle_timeout_returned(): assert next_idle is not None assert next_idle > last_idle - assert s.check_idle() is None - # ^ NOTE: this probably should be `== next_idle`; - # see discussion in https://github.com/dask/distributed/pull/7687#discussion_r1145095196 + assert s.check_idle() is next_idle @pytest.mark.slow +@pytest.mark.xfail(WINDOWS, reason="Subprocess launching scheduler TimeoutError") +@pytest.mark.xfail(MACOS, reason="Client fails to connect on OSX") def test_shutsdown_cleanly(loop): port = open_port() with concurrent.futures.ThreadPoolExecutor() as tpe: From f9cd3dc2cc672da09071872a0b047f9e82924e06 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Tue, 5 Mar 2024 07:06:14 +0100 Subject: [PATCH 5/9] Adapt dask worker test to verify proper proxy --- distributed/cli/tests/test_dask_worker.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/distributed/cli/tests/test_dask_worker.py b/distributed/cli/tests/test_dask_worker.py index ba01f7d6b2..e87f441d36 100644 --- a/distributed/cli/tests/test_dask_worker.py +++ b/distributed/cli/tests/test_dask_worker.py @@ -18,7 +18,7 @@ from distributed.compatibility import LINUX, WINDOWS from distributed.deploy.utils import nprocesses_nthreads from distributed.metrics import time -from distributed.utils import open_port +from distributed.utils import get_ip, open_port from distributed.utils_test import ( gen_cluster, inc, @@ -489,24 +489,27 @@ def test_dashboard_non_standard_ports(): pytest.importorskip("bokeh") requests = pytest.importorskip("requests") - host = "127.0.0.1" + s_host = "127.0.0.1" + # use internal ip instead of localhost ip to verify GlobalProxyHandler will update + # to allow internal host ip of a worker. + w_host = get_ip() s_port = "3233" s_dashboard_port = "3232" w_dashboard_port = "4833" - s_cmd = f"dask scheduler --host {host} --port {s_port} --dashboard-address :{s_dashboard_port}" - w_cmd = f"dask worker {host}:{s_port} --dashboard-address :{w_dashboard_port} --host {host}" + s_cmd = f"dask scheduler --host {s_host} --port {s_port} --dashboard-address :{s_dashboard_port}" + w_cmd = f"dask worker {s_host}:{s_port} --dashboard-address :{w_dashboard_port} --host {w_host}" with popen(s_cmd.split()): with popen(w_cmd.split()): - with Client(f"{host}:{s_port}") as c: + with Client(f"{s_host}:{s_port}") as c: c.wait_for_workers(1) - response = requests.get(f"http://{host}:{w_dashboard_port}/status") + response = requests.get(f"http://{s_host}:{w_dashboard_port}/status") response.raise_for_status() # TEST PROXYING WORKS response = requests.get( - f"http://{host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{host}/status" + f"http://{s_host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{w_host}/status" ) response.raise_for_status() From b1fa686c97864f5565dd49fa19c691941d4993a6 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Tue, 5 Mar 2024 10:22:02 +0100 Subject: [PATCH 6/9] Skip test_jupyter_server on Windows --- distributed/tests/test_jupyter.py | 1 + 1 file changed, 1 insertion(+) diff --git a/distributed/tests/test_jupyter.py b/distributed/tests/test_jupyter.py index 9bfa63c09c..1e1149507c 100644 --- a/distributed/tests/test_jupyter.py +++ b/distributed/tests/test_jupyter.py @@ -23,6 +23,7 @@ pytestmark = pytest.mark.filterwarnings("ignore:Jupyter is migrating its paths") +@pytest.mark.skipif(WINDOWS, reason="ValueError: I/O operation on closed file") @gen_test() async def test_jupyter_server(): async with Scheduler(jupyter=True) as s: From 049dcd020f90f9430ea9b23c1160e168daa7b1a3 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Tue, 5 Mar 2024 10:59:40 +0100 Subject: [PATCH 7/9] Just skip test_jupyter tests for Windows --- distributed/tests/test_jupyter.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/distributed/tests/test_jupyter.py b/distributed/tests/test_jupyter.py index 1e1149507c..52ca25c169 100644 --- a/distributed/tests/test_jupyter.py +++ b/distributed/tests/test_jupyter.py @@ -22,8 +22,18 @@ pytestmark = pytest.mark.filterwarnings("ignore:Jupyter is migrating its paths") +if WINDOWS: + try: + import jupyter_server # noqa: F401 + except ImportError: + pass + else: + pytest.skip( + allow_module_level=True, + reason="Windows struggles running these tests w/ jupyter server", + ) + -@pytest.mark.skipif(WINDOWS, reason="ValueError: I/O operation on closed file") @gen_test() async def test_jupyter_server(): async with Scheduler(jupyter=True) as s: From cc502f19541ac8a069fb2831a851884857d0d358 Mon Sep 17 00:00:00 2001 From: Miles Date: Wed, 13 Mar 2024 11:40:32 +0100 Subject: [PATCH 8/9] Apply suggestions from code review Co-authored-by: Hendrik Makait --- distributed/cli/tests/test_dask_worker.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/distributed/cli/tests/test_dask_worker.py b/distributed/cli/tests/test_dask_worker.py index e87f441d36..a75810d882 100644 --- a/distributed/cli/tests/test_dask_worker.py +++ b/distributed/cli/tests/test_dask_worker.py @@ -499,18 +499,17 @@ def test_dashboard_non_standard_ports(): s_cmd = f"dask scheduler --host {s_host} --port {s_port} --dashboard-address :{s_dashboard_port}" w_cmd = f"dask worker {s_host}:{s_port} --dashboard-address :{w_dashboard_port} --host {w_host}" - with popen(s_cmd.split()): - with popen(w_cmd.split()): - with Client(f"{s_host}:{s_port}") as c: - c.wait_for_workers(1) + with popen(s_cmd.split()), popen(w_cmd.split()): + with Client(f"{s_host}:{s_port}") as c: + c.wait_for_workers(1) - response = requests.get(f"http://{s_host}:{w_dashboard_port}/status") - response.raise_for_status() + response = requests.get(f"http://{s_host}:{w_dashboard_port}/status") + response.raise_for_status() - # TEST PROXYING WORKS - response = requests.get( - f"http://{s_host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{w_host}/status" - ) + # TEST PROXYING WORKS + response = requests.get( + f"http://{s_host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{w_host}/status" + ) response.raise_for_status() with pytest.raises(requests.ConnectionError): From 04c5415c6ddc34c3c47c44fabd909b80660fccf8 Mon Sep 17 00:00:00 2001 From: Miles Granger Date: Wed, 13 Mar 2024 12:16:57 +0100 Subject: [PATCH 9/9] Fixup: fix bad indentation --- distributed/cli/tests/test_dask_worker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distributed/cli/tests/test_dask_worker.py b/distributed/cli/tests/test_dask_worker.py index a75810d882..771dd6d2af 100644 --- a/distributed/cli/tests/test_dask_worker.py +++ b/distributed/cli/tests/test_dask_worker.py @@ -510,7 +510,7 @@ def test_dashboard_non_standard_ports(): response = requests.get( f"http://{s_host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{w_host}/status" ) - response.raise_for_status() + response.raise_for_status() with pytest.raises(requests.ConnectionError): requests.get("http://localhost:4833/status/")