Unix socket support - #9337
Conversation
|
@jacobtomlinson could you let me know if you would be interested in merging this PR (after feedback/refactoring of course)? Thanks! |
jacobtomlinson
left a comment
There was a problem hiding this comment.
This generally seems like a positive change. I agree that I like unix:// more. I would be apprehensive to make this the default without broader community testing, but I agree that it's a better default in the long run.
Could I ask you to make sure CI is happy (unrelated failures are fine) and add docs?
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 40 files ± 0 40 suites ±0 14h 52m 43s ⏱️ + 5m 23s For more details on these failures, see this check. Results for commit 588d58f. ± Comparison against base commit 40fcd99. ♻️ This comment has been updated with latest results. |
Make get_uds_path() respect XDG_RUNTIME_DIR and dask.config['temporary-directory']
…in service definition
| retries_left = 3 | ||
|
|
||
| if os.path.isabs(http_address["address"]): # unix socket | ||
| try: # remove any old sockets |
There was a problem hiding this comment.
Does this make sense or should we do a check on whether the server is bound to unix socket again in stop, and remove there?
|
I added some docs here and here. It's pretty short, but the existing documentation on protocols is also pretty basic. For the Let me know if there's anywhere else where I should add any more extensive documentation! |
|
The remaining linting errors are pre-existing. |
This is easier than removing on stop. It technically introduces a race condition, but so would removing on stop, I think.
On Linux we need to unset XDG_RUNTIME_DIR when we're not testing it.
| Set kernel-level TCP timeout on the stream. | ||
| """ | ||
| if comm.closed(): | ||
| if comm.closed() or (not WINDOWS and comm.socket.family is socket.AF_UNIX): |
There was a problem hiding this comment.
I don't like that we have to check for unix socket stuff in tcp.py -- that should all belong to the subclass.
To refactor this we could change set_tcp_timeout to a function in the TCP class, create a new UDS subclass of TCP in uds.py, and override set_tcp_timeout there.
An example of this approach is here (see also my other comment): dometto@5e60065
Let me know if you'd like me to implement that.
| raise CommClosedError() | ||
|
|
||
| return unparse_host_port(*comm.socket.getsockname()[:2]) | ||
| if not WINDOWS and comm.socket.family is socket.AF_UNIX: |
There was a problem hiding this comment.
I don't like that we have to check for unix socket stuff in tcp.py -- that should all belong to the subclass.
Refactoring this is slightly more involved than for set_tcp_tmeout, as this function is called from inside _handle_stream.
To work around this, we would have to implement _handle_stream entirely in UDSListener, instead of making it subclass TCPListener. That is perhaps cleaner anyway.
An example of this approach is again here: dometto@5e60065
Let me know if you'd like me to implement that.
There was a problem hiding this comment.
Basically dometto@5e60065 removes all subclassing from uds.py except for the UDS Comm, which subclasses TCP -- in order not to have to duplicate the code for reading and writing streams.
To make the UDS code entirely independent from TCP another option would be to create a new mixin class for IOStream reading and writing, and include that in both UDS and TCP. Not sure if that's worth it.
8b9f840 to
0ca84de
Compare
|
@jacobtomlinson I think the remaining test failures are probably unrelated. |
Resolves #3630
This PR adds support for Unix Domain Sockets...
commsThe rationale for this is twofold:
Test this branch with:
New UDS backend
This is the main part of this PR. We implement a new backend
UDSwhich subclasses the standardTCPclasses. When setting protocol on a cluster/worker/scheduler touds://, this backend is used. When a worker or scheduler is given an absolute path, it will use that absolute path to create the unix socket. If instead any other value is passed in (e.g. the defaultlocalhost), the backend will create the socket at a new temporary path.Benefits
Performance
@mrocklin tested this:
Security
UDS support is desireable for usecases where:
inproc)ucxis not available (e.g. not on Linux)tcp).dask-labextensionfor instance), or desired (more overhead).Dashboard listening on UDS
When the
dashboard_urlis set to an absolute path, listen on UDS instead of TCP. This can help prevent new CVE's of the form GHSA-c336-7962-wfj2Todo
pixi run linttest_comms.py?unix://instead ofuds://?