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

Add Unix socket support #944

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Jan 22, 2025

This PR adds initial unix socket support to Garnet. The unix domain socket path can be specified with --unixsocket switch. Because string address, int port pair was used everywhere, this change required a lot of tiny changes in testing and networking logic. There is more clean-up work left in some parts of the code, e.g. using IPEndPoint to represent cluster nodes instead of the (string address, int port) tuple.

I'm leaving implementation of --unixsocketperm switch to be done as a follow-up. --unixsocketperm is now implemented for applicable platforms.

I'm also for now making the server initialization throw NotImplementedException if user attempts to specify unix socket listener in cluster mode. Supporting cluster mode would need bit more work to have two different types of listener endpoints active.

Contributes to #682

@PaulusParssinen PaulusParssinen changed the title wip: Add Unix socket support Add Unix socket support Jan 25, 2025
@PaulusParssinen PaulusParssinen marked this pull request as ready for review January 25, 2025 02:53
@badrishc badrishc requested a review from vazois January 27, 2025 01:42
Copy link
Contributor

@vazois vazois left a comment

Choose a reason for hiding this comment

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

Nice work! Going through it in iterations because it changes a lot of files.
Are we adding any explicit tests for unix socket?

libs/common/LightClient.cs Show resolved Hide resolved
libs/host/Configuration/Options.cs Show resolved Hide resolved
libs/server/StoreWrapper.cs Outdated Show resolved Hide resolved
@PaulusParssinen
Copy link
Contributor Author

Are we adding any explicit tests for unix socket?

I added single, a very primitive, test to check that the networking stacks of server and GarnetClient behave normally with unix socket endpoint here.

If you have any ideas for further tests, I'm happy to add them.

@vazois
Copy link
Contributor

vazois commented Jan 31, 2025

Are we adding any explicit tests for unix socket?

I added single, a very primitive, test to check that the networking stacks of server and GarnetClient behave normally with unix socket endpoint here.

If you have any ideas for further tests, I'm happy to add them.

Perfect, this is exactly what I was looking for. I am wondering if having also the same but with TLS on would make sense or if it is even possible. If you can add something like that, it will be great.

@badrishc
Copy link
Contributor

I'm leaving implementation of --unixsocketperm switch to be done as a follow-up

this would be nice to have as part of unix socket support. separate PR is fine if it is a non-trivial addition.

@badrishc
Copy link
Contributor

badrishc commented Jan 31, 2025

Hi @PaulusParssinen, when the unix socket is enabled, does that need to preclude normal TCP listeners and connections to the server? It seems we ought to be able to accept both (or just one if so desired) at the same time, i.e. enabling unix sockets should not preclude TCP connections. That way cluster mode would continue to work (using TCP, because cluster is usually remote servers) when unix sockets are enabled.

E.g., in other systems, you would enable both endpoints as follows:

port 6379
unixsocket /tmp/redis.sock

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Jan 31, 2025

Perfect, this is exactly what I was looking for. I am wondering if having also the same but with TLS on would make sense or if it is even possible. If you can add something like that, it will be great.

You're right, I think TLS path needs to be tested (or atleast how it fails and handle that if it is trivial to support ). I'll investigate 👍

I'm leaving implementation of --unixsocketperm switch to be done as a follow-up

this would be nice to have as part of unix socket support. separate PR is fine if it is a non-trivial addition.

Should be pretty trivial in hindsight, if we presumably gate this permission option for non-windows platforms 👍 (unless someone knows how to map unix permission octal to windows file permission!)

Hi @PaulusParssinen, when the unix socket is enabled, does that need to preclude normal TCP listeners and connections to the server? It seems we ought to be able to accept both (or just one if so desired) at the same time, i.e. enabling unix sockets should not preclude TCP connections. That way cluster mode would continue to work (using TCP, because cluster is usually remote servers) when unix sockets are enabled.

Originally I wrote in the PR description, to currently to keep the PR "simple", to make it temporarily either or decision => inherently makes cluster-mode not supported. This made the PR mostly mechanical (string ip, int port) tuple to EndPoint abstraction change, which just almost automatically brings unix domain socket support to the application (just needs config parsing).

This of course does not need to be case, but having two listener endpoints active (it'd be probably implemented to support any number of endpoints) from my viewpoint would singificantly raise the complexity of this PR as I think I'd have to spend quite bit more time to be confident to start refactoring the networking stack to accommodate this (potential races etc.). That's my rational for why I'd rather see this work be done as a follow up PR. This PR lays foundation for this follow-up refactor imo.

@badrishc
Copy link
Contributor

This of course does not need to be case, but having two listener endpoints active (it'd be probably implemented to support any number of endpoints) from my viewpoint would singificantly raise the complexity of this PR

This makes sense. Now that we have the clean abstraction of endpoints, the introduction of multiple endpoints should be doable as a separate work item, as that is orthogonal to this PR.

@PaulusParssinen PaulusParssinen marked this pull request as draft January 31, 2025 22:56
@PaulusParssinen PaulusParssinen marked this pull request as ready for review January 31, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants