Improve URMA build support, mock isolation, and transport reliability - #25
Open
cw20050111-prog wants to merge 29 commits into
Open
Improve URMA build support, mock isolation, and transport reliability#25cw20050111-prog wants to merge 29 commits into
cw20050111-prog wants to merge 29 commits into
Conversation
…works The umdk repo ships its own src/urma/BUILD.bazel, which makes Bazel treat src/urma/ as a separate package. glob() in our umdk.BUILD (rooted at the umdk repo root) can't cross that boundary, so hdrs = glob(["src/urma/lib/urma/**/include/*.h"]) silently resolved to an empty list. The cc_library then exported no header inputs, so compiling anything that #includes urma_api.h failed with "No such file or directory" even though the -isystem path was correct and the file existed on disk. Confirmed via GitHub Actions on both ubuntu-22.04 (x86_64) and ubuntu-24.04-arm (arm64): `bazel build --define=BRPC_WITH_URMA=true //:brpc` failed identically on both before this patch_cmds fix.
Reviewer dwh110 pointed out that src/brpc/urma/mock_urma.cpp had no independent feature switch: whenever liburma wasn't found, CMake/Make silently linked the mock, which can produce a binary that looks URMA-capable but can't reach real hardware. Add WITH_URMA_MOCK (CMake) / --with-urma-mock (config_brpc.sh), default OFF. When WITH_URMA is enabled and liburma isn't found, the build now fails with a clear message unless the mock is explicitly requested, instead of substituting it implicitly. Document the new flag in docs/en/urma.md and docs/cn/urma.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add conditional linking for URMA library based on availability.
Added a comment regarding linking with liburma based on brpc build options.
Added logic to find the URMA library and handle its absence.
Removed conditional check for URMA library and related messages.
Update logic for linking liburma based on header presence.
Add local design notes and Graphify artifacts to .gitignore
Ensure bond_include is always symlinked, adding a check for its presence.
…C++14 clean The cmake-unittest-urma-mock CI job failed to compile: the urma_user_ctl() mock added in 9b540ac was pasted into the middle of urma_create_context(), so the file had a function definition nested inside another function ("error: a function-definition is not allowed here before '{' token"). Move urma_user_ctl() out to file scope, after urma_create_context(). While verifying the fix, the same sources were built the way a real deployment builds them (Makefile path, -std=c++14, --werror) and three further problems showed up: * mock_urma.cpp used std::shared_mutex, which is C++17. config_brpc.sh pins -std=c++14, so `--with-urma --with-urma-mock` could not compile at all outside CMake. Use std::shared_timed_mutex (C++14, same reader/ writer semantics) behind a MockSharedMutex alias. * mock_urma.cpp ignored the results of write()/read() on the JFCE eventfd with a plain (void) cast, which glibc's warn_unused_result still diagnoses; capture the result instead. * urma_endpoint.cpp compared a signed bthread_tag_t against _poller_groups.size() (-Wsign-compare), and brpc_urma_unittest.cpp memset() a ParsedHello, which is non-trivial (-Wclass-memaccess). Add a make-compile-urma-mock CI job so the C++14/--werror combination is covered from now on; the existing CMake job builds with the compiler's default standard and cannot catch it. Verified locally: CMake mock build + ctest, CMake build against a real liburma.so (mock compiled as a shared library standing in for the SDK), and the Makefile build of every URMA object with --werror --with-urma --with-urma-mock. All 14 brpc_urma_unittest cases pass in both CMake configurations.
WITH_URMA_MOCK=ON was silently ignored on any machine that has liburma installed: CMake checked "is liburma found?" first and only consulted the switch in the not-found branch. config_brpc.sh had the same inversion. That makes a mock build mean different things on different machines -- the mock on a CI image without liburma, a hardware build on a developer box that happens to have the SDK -- which is exactly the "the mock lacks an independent feature switch" problem the switch was added to solve. It also disagreed with Bazel, where brpc_with_urma_mock is a specialization of brpc_with_urma and therefore already wins whenever both defines are set. Check the explicit opt-in first, so all three build systems agree. When the switch overrides a real liburma, CMake emits a WARNING and config_brpc.sh an info line naming the library being bypassed. Not finding liburma without the switch still fails the build, unchanged. Found on an aarch64 box with umdk-urma installed: -DWITH_URMA_MOCK=ON configured as mock=0 there, leaving the mock side of the build unbuildable and untestable.
On CTP -- which is what brpc advertises in MakeLocalParsedHello -- a single URMA SEND may not carry more than 4096 bytes. Over-size WRs are not rejected by urma_post_jetty_send_wr: the call succeeds and reports a successful completion, but the payload never arrives intact, so the peer's parser waits forever for bytes that never come. The RPC only fails on timeout, with clean logs on both ends. The send path previously bounded a WR only by the peer's advertised recv block size (urma_buffer_size - sizeof(IOBuf::Block), 8160 by default) -- twice the real limit. Clamp it by the transport limit as well so larger messages are split across WRs. No protocol or handshake change is needed: the receiver already appends each completion to _read_buf in order and the protocol parser reassembles them, exactly as it does today for messages larger than one recv block. The device's own dev_cap.max_msg_size cannot be used as this bound. Measured on aarch64 with umdk-urma 26.06.0-B020 the device reports 65536, yet every message above 4096 fails on the wire. Also: - add --urma_max_msg_size so the bound can be raised where the transport really does carry larger messages, still clamped by the device value; - report the full completion record (direction, opcode, lengths, jetty ids, window state) when a WR fails instead of a bare status number, and map urma_cr_status_t to a readable name; - zero-initialize the on-stack SGE array -- cut_into_sglist() sets addr/len/tseg but never user_tseg. Verified across two aarch64 nodes over real URMA hardware: with the cap in place every attachment size from 0 to 1MB completes with zero errors; with the cap raised to the device-reported 65536, everything from 4096 up fails. Single-node loopback behaves differently from a real link and must not be used to validate this path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related issue: apache/brpc 3428
This PR addresses URMA review findings around Bazel support, mock isolation, build compatibility, transport limits, observability, and TCP fallback.
What is changed?
URMA_ROOT.BRPC_DOWNLOAD_URMA_HEADERSto control the Bazel download fallback.WITH_URMA_MOCK/BRPC_WITH_URMA_MOCKswitch so mock builds cannot silently replace real URMA builds.--werror..gitignore.CMake and hardware validation
Environment: aarch64 openEuler, umdk-urma 26.06.0-B020, real URMA hardware between node1 and node3.
liburmacompleted successfully.brpc_urma_unittest: 11 tests passed, 4 mock-only tests skipped, and the process exited cleanly.max_msg_size=65536cannot be used as the transport limit.Bazel validation
Environment: Bazel 7.4.1, aarch64 openEuler, UMDK installed under
/usr.@umdk//:urma_headerssuccessfully withURMA_ROOT=/usrandBRPC_DOWNLOAD_URMA_HEADERS=false.bazel-bin/libbrpc.aandbazel-bin/libbrpc.so.readelfconfirmed that the real build linksliburma.so.0.BRPC_WITH_URMA=trueandBRPC_WITH_URMA_MOCK=true.liburma, andnmconfirmed that it definesurma_create_contextitself.URMA_ROOTandBRPC_DOWNLOAD_URMA_HEADERS=false, repository resolution failed with the expected clear error instead of downloading UMDK.WORKSPACE-mode UMDK repository resolution also succeeded. Full WORKSPACE-mode analysis on this aarch64 host is blocked by the repository's existing pinned
rules_perl, which only registers x86_64 toolchains. This limitation is unrelated to the URMA changes; the Bzlmod real and mock builds both completed successfully.