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

[core] Add support for vsock transport #32847

Merged
merged 24 commits into from May 30, 2023
Merged

[core] Add support for vsock transport #32847

merged 24 commits into from May 30, 2023

Conversation

YadongQi
Copy link
Contributor

@YadongQi YadongQi commented Apr 11, 2023

This is another attempt to add support for vsock in grpc since previous
PRs(#24551, #21745) all closed without merging.

The VSOCK address family facilitates communication between
virtual machines and the host they are running on.
This patch will introduce new scheme: [vsock:cid:port] to
support VSOCK address family.

Fixes #32738.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 11, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@YadongQi
Copy link
Contributor Author

@markdroth could you help review this PR?

@markdroth
Copy link
Member

Sorry for the delay; I was out of the office last week and am still trying to get caught back up. I'll aim to take a look at this PR within the next few days.

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Overall, this looks good! Comments are mostly minor.

Looks like there's a merge conflict that needs to be resolved. Please merge master into this branch rather than force-pushing. Thanks!

@drfloob, can you please provide a second review for this?

src/core/lib/address_utils/parse_address.cc Outdated Show resolved Hide resolved
src/core/lib/address_utils/sockaddr_utils.cc Outdated Show resolved Hide resolved
src/core/lib/address_utils/sockaddr_utils.cc Outdated Show resolved Hide resolved
#include "src/core/lib/iomgr/port.h"
#include "src/core/lib/iomgr/resolve_address.h"

void grpc_create_socketpair_if_vsock(int sv[2]);
Copy link
Member

Choose a reason for hiding this comment

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

@ctiller Do you think it makes sense to have a new core e2e test fixture for a vsock socket pair?

I think the existing core e2e test fixture for UDS socket pairs is the only reason that we have the equivalent of this function for UDS sockets. If we're not going to add such a fixture, then we probably don't need this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall, this looks good! Comments are mostly minor.

Looks like there's a merge conflict that needs to be resolved. Please merge master into this branch rather than force-pushing. Thanks!

@drfloob, can you please provide a second review for this?

Thank you @markdroth. I have resolved most comments. For the e2e test fixture, I think I need comments from @ctiller ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed your request to merge instead of force-push, I used to use rebase and force-push...... I will follow the rule for further commits.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't add an e2e test for it, I'm not sure how we'd keep this code path tested.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we'll want an e2e test fixture for this - to make sure all the bits work together (I'd love separate unit tests for the resolver, socket management, proxy hooks, etc.. but we don't have that yet for anything else)

Copy link
Member

Choose a reason for hiding this comment

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

There is an added hiccup here: gRPC supports CentOS 7, which distributes linux kernel v3.10. I don't think this PR will build on CentOS 7 right now. GRPC_HAVE_VSOCK will have to be defined more carefully: only if we can detect that the current platform supports it. See https://opensource.google/documentation/policies/cplusplus-support

The path to testing this may be a bit messy. It would require an entirely new job to be set up similar to the test configs in tools/internal_ci/linux/*, with some corresponding internal Google changes to make the test run (that last bit is not hard). It's possible for you to do, but it would be some extra work to grok the system. Also, the current socketpair implementation in this PR won't work, per https://github.com/torvalds/linux/blob/47a2ee5d4a0bda05decdda7be0a77e792cdb09a3/net/vmw_vsock/af_vsock.c#L2225, so something like VSOCK-local would have to be used to mimic a socket pair, so the end2end tests can run. See here for an example of the end2end test fixture for other socketpairs

CoreTestConfiguration{
"Chttp2SocketPair",
FEATURE_MASK_IS_HTTP2 | FEATURE_MASK_DO_NOT_FUZZ, nullptr,
[](const ChannelArgs&, const ChannelArgs&) {
return std::make_unique<SockpairFixture>(ChannelArgs());
}},
CoreTestConfiguration{
"Chttp2SocketPair1ByteAtATime",
FEATURE_MASK_IS_HTTP2 | FEATURE_MASK_1BYTE_AT_A_TIME |
FEATURE_MASK_DO_NOT_FUZZ,
nullptr,
[](const ChannelArgs&, const ChannelArgs&) {
return std::make_unique<SockpairFixture>(
ChannelArgs()
.Set(GRPC_ARG_TCP_READ_CHUNK_SIZE, 1)
.Set(GRPC_ARG_TCP_MIN_READ_CHUNK_SIZE, 1)
.Set(GRPC_ARG_TCP_MAX_READ_CHUNK_SIZE, 1));
}},
CoreTestConfiguration{
"Chttp2SocketPairMinstack",
FEATURE_MASK_IS_HTTP2 | FEATURE_MASK_IS_MINSTACK |
FEATURE_MASK_DO_NOT_FUZZ,
nullptr,
[](const ChannelArgs&, const ChannelArgs&) {
return std::make_unique<SockpairWithMinstackFixture>(
ChannelArgs());
}},

If the fixes can be made to get this to build cleanly on CentOS 7, I'd be in support of documenting this everywhere as community-supported and leave it mostly untested. That does mean that if it breaks (and we won't know it when it happens), the community would need to repair it, or the functionality would likely be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drfloob,

  1. for CentOS 7(CentOS Linux release 7.9.2009 (Core)), I have installed the OS and tried to build gRPC on it, the result turns out:
  • Failed to build with bazel(w or w/o this PR), see below errors:
[sdp@a4bf016a433b grpc]$ bazel build :all
INFO: Running bazel wrapper (see //tools/bazel for details), bazel version 6.1.2 will be used instead of system-wide bazel installation.
Starting local Bazel server and connecting to it...
INFO: Analyzed 174 targets (169 packages loaded, 5375 targets configured).
INFO: Found 174 targets...
INFO: From Compiling utf8_validity.cc [for tool]:
external/utf8_range/utf8_validity.cc:139:12: warning: unused function 'CodepointSkipBackwards' [-Wunused-function]
inline int CodepointSkipBackwards(int32_t codepoint_word) {
           ^
1 warning generated.
ERROR: /home/sdp/grpc/BUILD:677:16: Compiling src/core/lib/gpr/alloc.cc failed: (Exit 1): clang-11 failed: error executing command (from target //:gpr) /opt/rh/llvm-toolset-11.0/root/usr/bin/clang-11 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -fcolor-diagnostics -fno-omit-frame-pointer '-std=c++0x' -MD -MF ... (remaining 75 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
src/core/lib/gpr/alloc.cc:21:10: error: module //:gpr does not depend on a module exporting 'stdlib.h'
#include <stdlib.h>
         ^
1 error generated.
INFO: Elapsed time: 27.790s, Critical Path: 5.43s
INFO: 928 processes: 765 internal, 163 processwrapper-sandbox.
FAILED: Build did NOT complete successfully
  • Succeed to build with cmake(w or w/o this PR).
  • Since the build issue with bazel is not introduced by this PR, we can ignore the failure in this session, right?
  1. Just as you commented, we need to define GRPC_HAVE_VSOCK carefully, I have uploaded new commits to guard GRPC_HAVE_VSOCK, it will be enabled only when build with linux version >= 3.9. And also removed grpc_create_socketpair_if_vsock() since it is not supported. please help review.

Copy link
Member

Choose a reason for hiding this comment

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

Failed to build with bazel(w or w/o this PR), see below errors:

Oh, fun! That looks like a bazel configuration issue. What version of bazel did you use? gRPC only supports 5.x and newer https://github.com/grpc/grpc/blob/master/bazel/supported_versions.txt. Something like bazelisk would let you test with a supported version, if that's the issue.

I have uploaded new commits to guard GRPC_HAVE_VSOCK, it will be enabled only when build with linux version >= 3.9.

That looks good. I've kicked off the CI again, and I'll take another pass at your implementation today. Thank you for your persistence!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What version of bazel did you use? gRPC only supports 5.x and newer https://github.com/grpc/grpc/blob/master/bazel/supported_versions.txt. Something like bazelisk would let you test with a supported version, if that's the issue.

Original bazel version is v4.2.1, but even after I upgrade it to v6.1.2, the issue still exists.

[sdp@a4bf016a433b grpc]$ bazel --version
bazel 6.1.2
[sdp@a4bf016a433b grpc]$ bazel build :all
Starting local Bazel server and connecting to it...
INFO: Analyzed 174 targets (169 packages loaded, 5375 targets configured).
INFO: Found 174 targets...
ERROR: /home/sdp/grpc/BUILD:2408:16: Compiling src/core/lib/config/config_vars_non_generated.cc failed: (Exit 1): clang-11 failed: error executing command (from target //:config_vars) /opt/rh/llvm-toolset-11.0/root/usr/bin/clang-11 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -fcolor-diagnostics -fno-omit-frame-pointer '-std=c++0x' -MD -MF ... (remaining 57 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
src/core/lib/config/config_vars_non_generated.cc:19:10: error: module //:config_vars does not depend on a module exporting 'atomic'
#include <atomic>
         ^
src/core/lib/config/config_vars_non_generated.cc:20:10: error: module //:config_vars does not depend on a module exporting 'string'
#include <string>
         ^
2 errors generated.
INFO: Elapsed time: 25.616s, Critical Path: 3.96s
INFO: 1016 processes: 884 internal, 132 processwrapper-sandbox.
FAILED: Build did NOT complete successfully

I think we can create a seperate issue for the grpc build with bazel on CentOS 7, no need to expand more here.

For the CI results, there still 3 failures:
Bazel BinderTransport APK building/testing : submitted commit to resolve it
Distribution Tests Python Linux : No idea...
IWYU (internal CI) : It seems suggest some including position adjust, not sure to follow, do you have any comments?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can create a seperate issue for the grpc build with bazel on CentOS 7, no need to expand more here.

I totally agree, let's remove that requirement from this work, we'll have to work on it separately. Thank you for checking.

Regarding the other issues, I've sent a PR to fix the IWYU problems, the tool needed a little guidance: YadongQi#22. The python problem is unrelated, and won't block this PR.

src/core/lib/iomgr/vsock.cc Outdated Show resolved Hide resolved
@markdroth markdroth requested a review from drfloob April 19, 2023 21:29
The VSOCK address family facilitates communication between
virtual machines and the host they are running on.
This patch will introduce new scheme: [vsock:cid:port] to
support VSOCK address family.
@markdroth
Copy link
Member

Please do not force-push to a branch once a review has started. That makes it very difficult for the reviewer to see what has changed since their last review pass. Thanks!

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Sorry, I missed your request to merge instead of force-push, I used to use rebase and force-push...... I will follow the rule for further commits.

Whoops -- my turn to have missed a comment: I hadn't seen what you said there before I posted my last comment. Sorry for belaboring the point. :)

Anyway, this looks good to me!

What's left here is (a) getting input from @ctiller about the e2e test question and (b) getting a second review from @drfloob.

For now, I'll go ahead and trigger the tests.

@markdroth markdroth added kokoro:run release notes: yes Indicates if PR needs to be in release notes labels Apr 20, 2023
@markdroth
Copy link
Member

Oh, one more thing: Please also add an entry in https://github.com/grpc/grpc/blob/master/doc/naming.md describing the new vsock: URI scheme.

Copy link
Member

@drfloob drfloob left a comment

Choose a reason for hiding this comment

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

A few additional notes:

  1. We should add URI parser tests for vsock:<CID>:<port>, to be sure things like vsock:-1:-1 are parsed correctly.

  2. If this same vsock-handling logic is not ported to the PosixEventEngine, it will stop working sometime this year. Similar changes will likely need to be made to:

  • src/core/lib/event_engine/tcp_socket_utils.cc
  • src/core/lib/event_engine/posix_engine/posix_engine.cc
  • src/core/lib/event_engine/posix_engine/posix_engine_listener.cc
  • src/core/lib/event_engine/posix_engine/posix_endpoint.cc

src/core/lib/iomgr/vsock.cc Outdated Show resolved Hide resolved
src/core/lib/iomgr/vsock.cc Outdated Show resolved Hide resolved
src/core/lib/address_utils/parse_address.cc Show resolved Hide resolved
src/core/lib/iomgr/vsock.h Outdated Show resolved Hide resolved
#include "src/core/lib/iomgr/port.h"
#include "src/core/lib/iomgr/resolve_address.h"

void grpc_create_socketpair_if_vsock(int sv[2]);
Copy link
Member

Choose a reason for hiding this comment

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

If we don't add an e2e test for it, I'm not sure how we'd keep this code path tested.

@drfloob
Copy link
Member

drfloob commented Apr 20, 2023

Also, given that we don't have any CI environments that will actually exercise host/guest communications, the best we can probably do is claim that this feature is community-supported, and we could add some such disclaimer to the documentation about VSOCK support. @markdroth what do you think?

@markdroth
Copy link
Member

Also, given that we don't have any CI environments that will actually exercise host/guest communications, the best we can probably do is claim that this feature is community-supported, and we could add some such disclaimer to the documentation about VSOCK support. @markdroth what do you think?

This is tied into the question I asked @ctiller above about the e2e tests. How hard is it to set up tests for this?

If we can't have tests, then I agree that documenting the fact that this is community-supported makes sense. But if we can have tests without too much work, that seems better.

@YadongQi
Copy link
Contributor Author

Also, given that we don't have any CI environments that will actually exercise host/guest communications, the best we can probably do is claim that this feature is community-supported, and we could add some such disclaimer to the documentation about VSOCK support. @markdroth what do you think?

This is tied into the question I asked @ctiller above about the e2e tests. How hard is it to set up tests for this?

If we can't have tests, then I agree that documenting the fact that this is community-supported makes sense. But if we can have tests without too much work, that seems better.

Linux 5.6 introduces a new transport called vsock-loopback, it is useful for testing w/o running VMs(https://stefano-garzarella.github.io/posts/2020-02-20-vsock-nested-vms-loopback/). Maybe it can be used in e2e tests?

@YadongQi
Copy link
Contributor Author

Oh, one more thing: Please also add an entry in https://github.com/grpc/grpc/blob/master/doc/naming.md describing the new vsock: URI scheme.

done.

@YadongQi
Copy link
Contributor Author

This looks pretty good to me, I'll run the CI again after you merge the fixit PR I created on your branch.

Done.

I learned that our CI environment supports host/guest communications in nested VMs. None of our tests are set up to run across that boundary, though, so it would be some internal effort to get any tests working. I don't think anyone on the team has bandwidth at the moment, but it seems like the required infrastructure is there if/when the team has spare cycles.

Good to hear this. So the plan is to 1. make this a community supported feature now; 2. build E2E tests when internal team has bandwidth and then make it an official supported feature. Am I understanding correctly?

@jul-sh
Copy link

jul-sh commented May 24, 2023

Also, given that we don't have any CI environments that will actually exercise host/guest communications, the best we can probably do is claim that this feature is community-supported, and we could add some such disclaimer to the documentation about VSOCK support. @markdroth what do you think?

This is tied into the question I asked @ctiller above about the e2e tests. How hard is it to set up tests for this?
If we can't have tests, then I agree that documenting the fact that this is community-supported makes sense. But if we can have tests without too much work, that seems better.

Linux 5.6 introduces a new transport called vsock-loopback, it is useful for testing w/o running VMs(https://stefano-garzarella.github.io/posts/2020-02-20-vsock-nested-vms-loopback/). Maybe it can be used in e2e tests?

Until we have nested VM support in CI, should we write tests using vsock-loopback? Shouldn't that be enough to mark this feature as not community supported?

@drfloob
Copy link
Member

drfloob commented May 24, 2023

@jul-sh

Until we have nested VM support in CI, should we write tests using vsock-loopback?

I welcome it! You'd have to wire them up to look like a sockpair for it to work with the end2end tests. The tests would also have to be guarded behind another linux kernel version guard, but yes, it's doable.

Shouldn't that be enough to mark this feature as not community supported?

It's a step in that direction. If we had it under CI, I think we could offer best-effort support.

@YadongQi

So the plan is to 1. make this a community supported feature now; 2. build E2E tests when internal team has bandwidth and then make it an official supported feature. Am I understanding correctly?

Pretty much, yes. I think we can commit to best-effort support of the feature if we're continually testing the code paths.

@YadongQi
Copy link
Contributor Author

@drfloob
There still 2 failures of CI. But I think they are not caused by this PR, please help double confirm.

@drfloob
Copy link
Member

drfloob commented May 25, 2023

This LGTM. I'm comfortable merging this as community-supported at the moment, and we welcome any test improvements using the vsock loopback if someone wants to attempt it.

@markdroth can you take a second look, please?

@YadongQi YadongQi requested a review from markdroth May 26, 2023 00:26
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

LGTM. I'll let @drfloob get this merged.

@drfloob drfloob merged commit 9d76586 into grpc:master May 30, 2023
63 of 66 checks passed
@drfloob
Copy link
Member

drfloob commented May 30, 2023

Merged! Thank you for the contribution!

It will take maybe a day or two before we know if there are any complications when this code gets built in new environments that we don't have test coverage for. If we run into issues, I'll post back here with next steps. Otherwise, this feature is shipped.

@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 30, 2023
@gnossen
Copy link
Contributor

gnossen commented May 30, 2023

@drfloob This broke the Python build:

src/core/lib/iomgr/vsock.cc: In function 'int grpc_is_vsock(const grpc_resolved_address*)':
src/core/lib/iomgr/vsock.cc:50:29: error: 'AF_VSOCK' was not declared in this scope

@drfloob
Copy link
Member

drfloob commented May 30, 2023

Ah, I missed that, sorry. There was an unrelated python build error last time I checked. Let's roll this back and re-land it with fixes for the Python build.

@YadongQi I'll do the revert, would you please work on the python build fixes afterwards? You can see them by clicking on the View Details button on the merge notification in this thread.
image

drfloob added a commit that referenced this pull request May 30, 2023
drfloob added a commit that referenced this pull request May 30, 2023
@YadongQi
Copy link
Contributor Author

@YadongQi I'll do the revert, would you please work on the python build fixes afterwards?

uhmm, it seems the build failed with aarch64-unknown-linux-gnu-gcc:

aarch64-unknown-linux-gnu-gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -D_WIN32_WINNT=1536 -DGRPC_XDS_USER_AGENT_NAME_SUFFIX=\"Python\" -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX=\"1.56.0.dev0\" -DGPR_BACKWARDS_COMPATIBILITY_MODE=1 -DHAVE_CONFIG_H=1 -DGRPC_ENABLE_FORK_SUPPORT=1 "-DPyMODINIT_FUNC=extern \"C\" __attribute__((visibility (\"default\"))) PyObject*" -DGRPC_POSIX_FORK_ALLOW_PTHREAD_ATFORK=1 -Isrc/python/grpcio -Iinclude -I. -Ithird_party/abseil-cpp -Ithird_party/address_sorting/include -Ithird_party/cares/cares/include -Ithird_party/cares -Ithird_party/cares/cares -Ithird_party/cares/config_linux -Ithird_party/re2 -Ithird_party/boringssl-with-bazel/src/include -Ithird_party/upb -Isrc/core/ext/upb-generated -Isrc/core/ext/upbdefs-generated -Ithird_party/utf8_range -Ithird_party/xxhash -Ithird_party/zlib -I/opt/_internal/cpython-3.8.15/include/python3.8 -c src/core/lib/iomgr/vsock.cc -o python_build/temp.linux-x86_64-cpython-38/src/core/lib/iomgr/vsock.o -std=c++14 -fvisibility=hidden -fno-wrapv -fno-exceptions -pthread
src/core/lib/iomgr/vsock.cc: In function 'int grpc_is_vsock(const grpc_resolved_address*)':
src/core/lib/iomgr/vsock.cc:50:29: error: 'AF_VSOCK' was not declared in this scope
   return addr->sa_family == AF_VSOCK;
                             ^~~~~~~~
src/core/lib/iomgr/vsock.cc:50:29: note: suggested alternative: 'F_LOCK'
   return addr->sa_family == AF_VSOCK;
                             ^~~~~~~~
                             F_LOCK

I just tried with same compiling command with aarch64-linux-gnu-gcc(9.4.0) on my local env, it passed compiling and generated vsock.o(ELF 64-bit LSB relocatable, ARM aarch64, version 1 (SYSV), with debug_info, not stripped).

May I know the CI build environment? Is it a AARCH64 VM? Any specific configurations? It would be better if you could guiding me with detail steps on how to reproduce this build issue.

@Allen-Webb
Copy link

@YadongQi I'll do the revert, would you please work on the python build fixes afterwards?

uhmm, it seems the build failed with aarch64-unknown-linux-gnu-gcc:

aarch64-unknown-linux-gnu-gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -D_WIN32_WINNT=1536 -DGRPC_XDS_USER_AGENT_NAME_SUFFIX=\"Python\" -DGRPC_XDS_USER_AGENT_VERSION_SUFFIX=\"1.56.0.dev0\" -DGPR_BACKWARDS_COMPATIBILITY_MODE=1 -DHAVE_CONFIG_H=1 -DGRPC_ENABLE_FORK_SUPPORT=1 "-DPyMODINIT_FUNC=extern \"C\" __attribute__((visibility (\"default\"))) PyObject*" -DGRPC_POSIX_FORK_ALLOW_PTHREAD_ATFORK=1 -Isrc/python/grpcio -Iinclude -I. -Ithird_party/abseil-cpp -Ithird_party/address_sorting/include -Ithird_party/cares/cares/include -Ithird_party/cares -Ithird_party/cares/cares -Ithird_party/cares/config_linux -Ithird_party/re2 -Ithird_party/boringssl-with-bazel/src/include -Ithird_party/upb -Isrc/core/ext/upb-generated -Isrc/core/ext/upbdefs-generated -Ithird_party/utf8_range -Ithird_party/xxhash -Ithird_party/zlib -I/opt/_internal/cpython-3.8.15/include/python3.8 -c src/core/lib/iomgr/vsock.cc -o python_build/temp.linux-x86_64-cpython-38/src/core/lib/iomgr/vsock.o -std=c++14 -fvisibility=hidden -fno-wrapv -fno-exceptions -pthread
src/core/lib/iomgr/vsock.cc: In function 'int grpc_is_vsock(const grpc_resolved_address*)':
src/core/lib/iomgr/vsock.cc:50:29: error: 'AF_VSOCK' was not declared in this scope
   return addr->sa_family == AF_VSOCK;
                             ^~~~~~~~
src/core/lib/iomgr/vsock.cc:50:29: note: suggested alternative: 'F_LOCK'
   return addr->sa_family == AF_VSOCK;
                             ^~~~~~~~
                             F_LOCK

I just tried with same compiling command with aarch64-linux-gnu-gcc(9.4.0) on my local env, it passed compiling and generated vsock.o(ELF 64-bit LSB relocatable, ARM aarch64, version 1 (SYSV), with debug_info, not stripped).

May I know the CI build environment? Is it a AARCH64 VM? Any specific configurations? It would be better if you could guiding me with detail steps on how to reproduce this build issue.

This looks like the kernel headers didn't have vsock included (I am not sure if that would be from an unsupported kernel version or the KCONFIG not being set).

@YadongQi
Copy link
Contributor Author

YadongQi commented Jun 1, 2023

@drfloob @Allen-Webb
Found the root cause: the CI docker(us-docker.pkg.dev/grpc-testing/testing-images-public/grpc_artifact_python_manylinux2014_aarch64) is using an old glibc(v2.17) which not support vsock.
AF_VSOCK is introduced since glibc-v2.18: https://sourceware.org/git/?p=glibc.git;a=commit;h=164fd39d05925717e75715929c7ced14a2c1505e

So I will modify the guard code of #define GRPC_HAVE_VSOCK 1 and create a new PR for you to review.

eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 1, 2023
This is another attempt to add support for vsock in grpc since previous
PRs(grpc#24551, grpc#21745) all closed without merging.

The VSOCK address family facilitates communication between
virtual machines and the host they are running on.
This patch will introduce new scheme: [vsock:cid:port] to
support VSOCK address family.

Fixes grpc#32738.

---------

Signed-off-by: Yadong Qi <yadong.qi@intel.com>
Co-authored-by: AJ Heller <hork@google.com>
Co-authored-by: YadongQi <YadongQi@users.noreply.github.com>
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 1, 2023
@Allen-Webb
Copy link

Is this going to be relanded? It looks like it was reverted and there hasn't been much movement sense.

@drfloob
Copy link
Member

drfloob commented Jun 13, 2023

@Allen-Webb VSOCK support was added in #33309

mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
This is another attempt to add support for vsock in grpc since previous
PRs(grpc#24551, grpc#21745) all closed without merging.

The VSOCK address family facilitates communication between
virtual machines and the host they are running on.
This patch will introduce new scheme: [vsock:cid:port] to
support VSOCK address family.

Fixes grpc#32738.

---------

Signed-off-by: Yadong Qi <yadong.qi@intel.com>
Co-authored-by: AJ Heller <hork@google.com>
Co-authored-by: YadongQi <YadongQi@users.noreply.github.com>
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grpc over vsock (2023)
9 participants