Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix worker dashboard proxy #8528

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions continuous_integration/environment-3.10.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ dependencies:
- ipykernel
- ipywidgets
- jinja2
- jupyter-server-proxy
- jupyterlab
- locket
- msgpack-python
- netcdf4
Expand Down
2 changes: 2 additions & 0 deletions continuous_integration/environment-3.11.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ dependencies:
- ipykernel
- ipywidgets
- jinja2
- jupyter-server-proxy
- jupyterlab
- locket
- msgpack-python
- netcdf4
Expand Down
2 changes: 2 additions & 0 deletions continuous_integration/environment-3.12.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ dependencies:
- ipykernel
- ipywidgets
- jinja2
- jupyter-server-proxy
- jupyterlab
- locket
- msgpack-python
- netcdf4
Expand Down
2 changes: 2 additions & 0 deletions continuous_integration/environment-3.9.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ dependencies:
- ipykernel
- ipywidgets
- jinja2
- jupyter-server-proxy
- jupyterlab
- locket
- lz4 # Only tested here
- msgpack-python
Expand Down
51 changes: 22 additions & 29 deletions distributed/cli/tests/test_dask_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -485,39 +485,32 @@ 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

proxy_exists = True
except ImportError:
proxy_exists = False

with popen(
[
"dask",
"worker",
s.address,
"--dashboard-address",
":4833",
"--host",
"127.0.0.1",
]
):
await c.wait_for_workers(1)

response = requests.get("http://127.0.0.1:4833/status")
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 {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()), 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()

# TEST PROXYING WORKS
if proxy_exists:
response = requests.get("http://127.0.0.1:8787/proxy/4833/127.0.0.1/status")
response.raise_for_status()
response = requests.get(
f"http://{s_host}:{s_dashboard_port}/proxy/{w_dashboard_port}/{w_host}/status"
)
response.raise_for_status()
milesgranger marked this conversation as resolved.
Show resolved Hide resolved

with pytest.raises(requests.ConnectionError):
requests.get("http://localhost:4833/status/")
Expand Down
14 changes: 14 additions & 0 deletions distributed/http/proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@
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}
Expand All @@ -29,6 +38,9 @@
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

Expand All @@ -41,6 +53,8 @@
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)

Check warning on line 57 in distributed/http/proxy.py

View check run for this annotation

Codecov / codecov/patch

distributed/http/proxy.py#L56-L57

Added lines #L56 - L57 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be tested as well? (same for the change in lines 41-42)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do this w/o some obvious mocking/assert calls as all our tests are ran on the same host, but okay. I'll look at that as well.

Copy link
Contributor Author

@milesgranger milesgranger Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, small adjustment to a test to verify that bit of code. Note that exactly none of GlobalProxyHandler is directly tested but thru integration type testing so followed suit there. The test (test_dashboard_non_standard_ports) will fail now w/o GlobalProxyHandler updating host_allowlist. f9cd3dc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🚀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is still not tested, but given that testing appears to be tricky, I consider this non-blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, the second one there, is only hit on a websocket connection unfortunately.

# finally, proxy to other address/port
return await self.proxy_open(host, port, proxied_path)

Expand Down
21 changes: 17 additions & 4 deletions distributed/tests/test_jupyter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -21,6 +22,17 @@

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",
)


@gen_test()
async def test_jupyter_server():
Expand Down Expand Up @@ -73,7 +85,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)


Expand All @@ -94,12 +107,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:
Expand Down
3 changes: 3 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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''',
# https://github.com/dask-contrib/dask-expr/issues/945
'''ignore:dask_expr does not support the DataFrameIOFunction''',
'''ignore:Please use `dok_matrix` from the `scipy\.sparse` namespace, the `scipy\.sparse\.dok` namespace is deprecated.:DeprecationWarning''',
Expand Down
Loading