Repository navigation
Run gRPC deterministically in simulation and test the worker ControlService - #14337
saintstack wants to merge 4 commits into
Conversation
…ervice gRPC services in fdbserver had no simulation coverage: gRPC brings its own threads, reads the real clock, seeds from the OS and orders some work by pointer value, so simulated processes never started it. FDB also linked whatever gRPC was installed in /opt/grpc. - SimEventEngine runs gRPC's I/O, timers and closures on the Flow run loop over INetworkConnections, so under Sim2 real gRPC runs on simulated sockets and time. - gRPC 1.83.0 is built from source in FDB's build graph (cmake/CompileGrpc.cmake) with three patches: insertion-ordered queued-call sets, chttp2 stream cancellation in stream-id order, and an optional abseil seed-material source set only by simulation. - ControlService becomes a callback service whose handlers run Flow work on the hosting process. With the test configuration grpcEnabled, every simulated fdbd process serves it; the GrpcControlService and GrpcEcho workloads and the /fdbrpc/simgrpc unit tests exercise gRPC under faults. - Fixes: the gRPC server's stop path was a no-op; GrpcServer::run logged a SevError on actor_cancelled and the exclude handlers swallowed it; worker gRPC addresses were never persisted (the worker list is now encoded at 8.0, readable by 7.4 and 8.0); Sim2 reported a spurious leaked connection when a peer closed after its owner. See design/grpc-simulation.md.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gRPC 1.83 deprecates the TLS credential APIs FDB used; GCC's -Wno-deprecated does not cover deprecated declarations, so -Werror failed. Port Credentials.h to InMemoryCertificateProvider, set_root/identity_certificate_provider and TlsServerCredentialsOptions::Create (equivalent: the legacy provider plus watch flags set the same root and identity providers), and include gRPC's, abseil's and protobuf's headers as system headers, as a prebuilt package's are. Skip /fdbrpc/simgrpc in fdbrpc_test's normal (non-simulated) mode.
| if(GRPC_SOURCE_DIR) | ||
| grpc_run(copy ${CMAKE_COMMAND} -E copy_directory "${GRPC_SOURCE_DIR}" "${GRPC_SRC}") | ||
| else() | ||
| grpc_run(clone ${GIT_EXECUTABLE} clone --quiet --depth 1 --branch v${GRPC_VERSION} | ||
| https://github.com/grpc/grpc.git "${GRPC_SRC}") | ||
| grpc_run(submodules ${GIT_EXECUTABLE} -C "${GRPC_SRC}" submodule update --init --depth 1 | ||
| third_party/abseil-cpp third_party/protobuf third_party/re2 third_party/cares/cares) |
There was a problem hiding this comment.
If you can, I suggest replacing all the git cloning and patching with ExternalProject_add(), for an example see:
foundationdb/cmake/awssdk.cmake
Lines 20 to 34 in 1915da1
The downside is:
it can be quite difficult to define an appropriate
PATCH_COMMANDthat performs robustly, especially for download methods such as git where changing the GIT_TAG will not discard changes from a previous patch, but the patch command will be called again after updating to the new tag
So the simple thing is to just manually blow the whole grpc source checkout away, and let it clone and patch from scratch. But for the relative simplicity of using the more stock CMake functionality, I think it's worth it. Most devs will not be modifying the grpc dependency here.
In the future I have an idea to use a custom DOWNLOAD_COMMAND that will download and cache tarballs, and extract in the appropriate order and subdirs, instead of recursive git clone.
There was a problem hiding this comment.
FetchContent_makeAvailable is also good, maybe better
There was a problem hiding this comment.
Did Fetch_Content after chatting w/ @ploxiln
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Personally I am pretty against downloading from external websites during the build process. In this case we are much more vulnerable to mitm or upstream library poisoning. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fair. But we currently download boost, rocksdb, etc. This download does deviate some downloading by tag... Should probably do as others do and use a hash... Thanks @xis19 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| # A directory without a CMakeLists.txt, so FetchContent_MakeAvailable only populates; add_grpc() adds the project | ||
| # with EXCLUDE_FROM_ALL, which FetchContent supports only from CMake 3.28. | ||
| SOURCE_SUBDIR fdb-populate-only) |
There was a problem hiding this comment.
Our rockylinux9 build has cmake 3.31, so I think it would be fine to require cmake 3.28 for main (and even release-8.0 branch)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| PATCH_COMMAND ${GIT_EXECUTABLE} reset --quiet --hard --recurse-submodules | ||
| COMMAND ${GIT_EXECUTABLE} clean --quiet -fd | ||
| COMMAND ${GIT_EXECUTABLE} submodule --quiet foreach ${GIT_EXECUTABLE} clean --quiet -fd | ||
| COMMAND ${GIT_EXECUTABLE} apply --whitespace=nowarn ${GRPC_PATCHES} |
There was a problem hiding this comment.
Performing patch step for 'grpc-populate'
C:/Program Files/Git/bin/git.exe: line 1: C:/Program: No such file or directory
fatal: run_command returned non-zero status for third_party/abseil-cpp
I think we could get the windows cmake setup to work if we quoted the GIT_EXECUTABLE like "${GIT_EXECUTABLE}"
(or maybe just disable GRPC in the windows cmake setup CI job)
There was a problem hiding this comment.
... another plausible option is to just assume "git" binary is available:
| COMMAND ${GIT_EXECUTABLE} apply --whitespace=nowarn ${GRPC_PATCHES} | |
| COMMAND git apply --whitespace=nowarn ${GRPC_PATCHES} |
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
This comment has been minimized.
This comment has been minimized.
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
Agreed, that helps a lot. sha1 is considered weak, but git hashes are still "secure" today. In the future some cached tarball downloads protected by sha256 will be nice. |
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Run real gRPC deterministically inside FDB's simulator, build the gRPC that FDB tests, and serve and test the worker
ControlServicein simulation. Design:design/grpc-simulation.md.Until now, gRPC services in
fdbserverhad no simulation coverage. gRPC brings its own threads, reads the real clock, seeds its random generators from the OS and orders some work by pointer value, so simulated processes never started it. FDB also linked whatever gRPC was installed in/opt/grpc.Changes
SimEventEngine(fdbrpc/SimEventEngine.cpp): a gRPCEventEnginethat runs gRPC's I/O, timers and closures on the Flow run loop overINetworkConnections. Under Sim2, real gRPC then runs on simulated sockets and simulated time.cmake/CompileGrpc.cmake): gRPC 1.83.0 with its bundled abseil, protobuf, re2 and c-ares, built as targets in FDB's build graph. It replaces the/opt/grpcsearch.GRPC_SOURCE_DIRsupports hosts without GitHub access. Three patches (cmake/patches/grpc/) make gRPC deterministic:ControlServicebecomes a callback service whose handlers run Flow work on the hosting process.grpcEnabled, every simulated fdbd process serves it.GrpcControlService,GrpcEcho, and the/fdbrpc/simgrpcunit tests.GrpcServer's stop path was a no-op (inverted null check).GrpcServer::runlogged a SevError onactor_cancelled, and the exclude handlers swallowed it.FlowGrpcTests.cppbroke-Werrorbuilds now that gRPC code always compiles.Testing
GrpcControlServicealone, 100k runs: 100,000 passed;restarting/to_*downgrade tests passed.main, plus two-Werrorfixes. Every commit of the series builds on Linux. A-WerrorLinux build passes.20261021-060748-stack-echo-6246e2c69d991d47 compressed=True data_size=42295071 duration=3895412 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:34:44 sanity=False started=100000 stopped=20261021-064232 submitted=20261021-060748 timeout=5400 username=stack-echoCost
Clean Linux build (
ninja fdbserver,-j6): about 7 minutes (+29%) over building against the prebuilt/opt/grpc. Strippedfdbserveris +12.7 MB compared withWITH_GRPC=OFF. Incremental builds are unchanged.For reviewers
*Syncstop paths.@ploxiln Interested in your view of this vendoring of grpc et al.