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 Virtio VSOCK support #1410

Closed
wants to merge 2 commits into from
Closed

Conversation

jul-sh
Copy link

@jul-sh jul-sh commented Jun 10, 2023

Motivation

gRPC recently merged support for Virtio VSOCK as transport. It would be helpful for us if tonic were to match gRPC's support for it.

gRPC PR that adds VSOCK support: grpc/grpc#32847

Solution

Add Virtio VSOCK support, integration test, and example

cc @tiziano88

Comment on lines 12 to 15
// Use vsock-loopback so we don't need to spin up a VM. Must match server.
static TEST_CID: u32 = vsock::VMADDR_CID_LOCAL;
// Arbitrarily choosen. Must match server.
static TEST_PORT: u32 = 8000;
Copy link
Author

Choose a reason for hiding this comment

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

Personally I would prefer to store this in a public variable and import them in both client & server.

However, I decided to repeat them in each place they are used, to match the other examples & tests.

Comment on lines +17 to +20
// Virtio VSOCK does not use URIs, hence this URI will never be used.
// It is defined purely since in order to create a channel, since a URI has to
// be supplied to create an `Endpoint`.
static IGNORED_ENDPOINT_URI: &str = "file://[::]:0";
Copy link
Author

@jul-sh jul-sh Jun 10, 2023

Choose a reason for hiding this comment

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

Not proud of this, but this matches the code of the UDS example.

In the long term it would probably be a good idea to update the endpoint struct to no longer require a URI. However that should be done in a separate PR and it'd be a breaking change.

gRPC recently merged support for Virtio VSOCK as transport. It would be helpful for us if tonic were to match gRPC's support for it.

gRPC PR that adds VSOCK support: grpc/grpc#33309
@jul-sh
Copy link
Author

jul-sh commented Jun 10, 2023

cc @LucioFranco for review

@LucioFranco
Copy link
Member

I believe a lot of this could be achieved outside of tonic. So I don't see why we should add another dependency when you can new type and wrap it in your own code?

I am weary of adding this project as a dependency as well since it states:

tokio-vsock is for the most part pre-alpha quality, so there are probably sharp edges. Please test it thoroughly before using in production. Happy to receive pull requests and issue reports.

In the readme, while I probably trust the code I wouldn't want to include it in a wide spread project like this. IF for some reason you can't work around it then we should try to fix that problem.

Thanks!

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.

None yet

2 participants