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.

Sign up for GitHub

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

Jump to bottom

Fix worker dashboard proxy #8528

Merged
merged 13 commits into from
Mar 13, 2024
Merged

Fix worker dashboard proxy #8528

merged 13 commits into from
Mar 13, 2024

Conversation

milesgranger
Copy link
Contributor

Closes #8335

xref jupyter-server/jupyter_server#1012

  • Tests added / passed
  • Passes pre-commit run --all-files

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 26, 2024 •
edited
Loading

Unit Test Results

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

    27 files  ± 0      27 suites  ±0   10h 27m 51s ⏱️ + 30m 37s
 4 055 tests + 5   3 942 ✅ + 5    109 💤  -  1  4 ❌ +1 
50 872 runs  +36  48 555 ✅ +62  2 313 💤  - 27  4 ❌ +1 

For more details on these failures, see this check.

Results for commit 04c5415. ± Comparison against base commit 54ed6a1.

♻️ This comment has been updated with latest results.

@hendrikmakait
Copy link
Member

Is there a way for us to test these changes?

@milesgranger
Copy link
Contributor Author

Attempted in 4ad2bba but appears just having jupyter-server-proxy installed is causing almost every test to timeout. Will poke at it a bit more.

@milesgranger milesgranger force-pushed the milesgranger/8335-fix-proxy-to-worker-dashboard branch 2 times, most recently from 73acdd2 to 6e23fb3 Compare March 4, 2024 07:29
@milesgranger milesgranger force-pushed the milesgranger/8335-fix-proxy-to-worker-dashboard branch from 6e23fb3 to 389eb1e Compare March 4, 2024 12:25
hendrikmakait
hendrikmakait reviewed Mar 4, 2024
View reviewed changes
distributed/http/proxy.py
@@ -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)
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 •
edited
Loading

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.

@milesgranger
Copy link
Contributor Author

@hendrikmakait

Windows seems like a hard one to please for the jupyter tests:

distributed/tests/test_jupyter.py::test_jupyter_server SKIPPED (Valu...) [ 68%]
distributed/tests/test_jupyter.py::test_jupyter_cli PASSED [ 69%]
Error in sys.excepthook:
Original exception was:
distributed/tests/test_jupyter.py::test_jupyter_idle_timeout FAILED [ 69%]
Error: Process completed with exit code 1.

And similar issue with test_jupyter_server on c711856, so I'm inclined to just skip the module for windows when jupyter_server is installed. What do you think?

@milesgranger
Copy link
Contributor Author

Okay @hendrikmakait, sorry for dragging this out. Think it's ready for another gander. Failures are unrelated, (mostly #8561)

hendrikmakait
hendrikmakait approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @milesgranger!

distributed/cli/tests/test_dask_worker.py Outdated Show resolved Hide resolved
distributed/cli/tests/test_dask_worker.py Outdated Show resolved Hide resolved
distributed/http/proxy.py
@@ -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)
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.

@milesgranger @hendrikmakait
Apply suggestions from code review
cc502f1 
Co-authored-by: Hendrik Makait <hendrik@makait.com>
@milesgranger milesgranger force-pushed the milesgranger/8335-fix-proxy-to-worker-dashboard branch from cc502f1 to dda3786 Compare March 13, 2024 11:13
@milesgranger milesgranger force-pushed the milesgranger/8335-fix-proxy-to-worker-dashboard branch from dda3786 to 04c5415 Compare March 13, 2024 11:17
@milesgranger milesgranger force-pushed the milesgranger/8335-fix-proxy-to-worker-dashboard branch from a9b764b to 04c5415 Compare March 13, 2024 13:32
@milesgranger
Copy link
Contributor Author

There seems to be a flaky test now:

FAILED distributed/tests/test_jupyter.py::test_shutsdown_cleanly - OSError: Timed out trying to connect to tcp://127.0.0.1:40245 after 30 s

If it's okay with you, @hendrikmakait, can we merge this and I sort that out in a follow-up tomorrow?

@hendrikmakait
Copy link
Member

If it's okay with you, @hendrikmakait, can we merge this and I sort that out in a follow-up tomorrow?

Works for me, could you add an issue for that and assign it to yourself?

@hendrikmakait hendrikmakait merged commit 29cb664 into dask:main Mar 13, 2024
54 of 65 checks passed
@milesgranger milesgranger deleted the milesgranger/8335-fix-proxy-to-worker-dashboard branch March 14, 2024 05:06
@milesgranger milesgranger mentioned this pull request Mar 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hendrikmakait hendrikmakait hendrikmakait approved these changes

@fjetter fjetter Awaiting requested review from fjetter fjetter is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proxy to worker dashboard not working
2 participants
@milesgranger @hendrikmakait