Repository navigation
[Store] Reuse an in-process master for tests and evaluation - #3665
stmatengss merged 11 commits into
Conversation
Enable single-node Mooncake Store deployments without a separate mooncake_master process. When enable_standalone is set (Python setup, config/env, mooncake_client, or mooncake_store_service), the client starts an in-process master and defaults Transfer Engine metadata to P2PHANDSHAKE. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
Include complete coro_rpc/admin/metadata headers and define the destructor inline so unique_ptr members can be destroyed from real_client.cpp. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
Add enable_standalone to mocked argparse objects, pass the flag through setup, and keep getattr fallback so incomplete mocks do not crash service startup. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
SGLang HiCache calls MooncakeDistributedStore.setup() without the enable_standalone kwarg. Infer the flag from the environment so a single-node SGLang process can embed master without a SGLang patch. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
github-actions
Bot
added
documentation
Run put/get, batch, session-range, tensor, config-dict, and env-var
standalone e2e in Linux CI before any mooncake_master process starts.
Use a myst {ref} for the standalone deployment cross-link so Sphinx
docs-check no longer fails on a missing heading id.
Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
Standalone mode has no NoF pool, so the default nof_replica_num=1 made every put fail with nof_pool_disabled. Default NoF replicas to 0 when --enable-standalone is set and pass that flag from the CI runner. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
|
I think deployment complexity and maintenance cost are affected by two dimensions: the number of components that need to be deployed, and the number of deployment modes that users need to choose from. One example is the client side. After we introduced three deployment options — embedded, real client, and standalone service — we received quite a few questions for a long time about which mode to use, what the differences were, and how each mode should be deployed. From that perspective, adding another mode may actually increase the overall deployment complexity. At the same time, the scenarios where this new mode would be useful seem relatively limited. Given that, do we think the additional deployment mode is necessary? Also, the term “standalone mode” has already been used for a long time in the existing context, so reusing the same name here may introduce some confusion with the previous meaning. |
This is not a fourth client hosting mode. The existing three options (embedded client, mooncake_store_service, mooncake_client) are still how the client is hosted. enable_standalone only changes whether master is an external mooncake_master process or is started in-process (EmbeddedMaster). The flag is opt-in on top of those same three client paths; it does not add a new way to run the client. The intended use is single-process evaluation and embedding: a notebook, a unit/e2e test, or a library that wants Put/Get without launching mooncake_master (and without a metadata service, via P2PHANDSHAKE). Multi-node and HA still require a dedicated master. Linux CI runs this path in test-wheel-ubuntu and asserts that no mooncake_master process is present. On naming: in the current docs, “standalone” already means the store service / real-client process. Reusing it here for “no external master” is confusing. I can rename the flag and docs to something like enable_embedded_master / “in-process master” if that is preferred. |
|
Thanks for the clarification. My main concern is still around deployment complexity from the user's perspective. For this use case, I’m not yet convinced that the deployment difficulty is fundamentally different from using the client embedded mode with a separate master. Compared with I’m also a bit concerned about continuing to expand the number of deployment combinations we expose. In particular, After our offline discussion, we agreed to go through the concrete use cases and deployment differences in more detail in next week’s meeting. |
Keep enable_standalone on Python setup()/config and MOONCAKE_ENABLE_STANDALONE inside RealClient. Revert dummy-client, store-service, mooncake_client CLI, HiCache docs, and existing e2e script changes so this is not a new client hosting mode. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
Drop the user-facing standalone deployment docs and rename the flag to enable_embedded_master so this is not a fourth documented client/master combination. CI and unit tests still exercise EmbeddedMaster. Co-authored-by: Teng Ma <stmatengss@users.noreply.github.com>
|
@zxpdemonio Thanks for the review. Could you share a bit more context on the motivation and background behind this PR? |
|
Thanks for the clarification on the meeting. Will start the review process. Meanwhile, could you resolve the conflict? |
|
Thanks for narrowing the scope to in-process testing and evaluation. I reviewed the latest revision and noticed two points that I’d like to discuss:
The new E2E tests passed in CI, which is useful evidence for the main path. The two cases above do not appear to be covered yet; these observations are from code review rather than a local runtime reproduction. |
Signed-off-by: Teng Ma <stmatengss@gmail.com>
stmatengss
changed the title
|
Addressed in 49c2a59, including the merge with current For the listener-scope comment, EmbeddedMaster's RPC and admin listeners now bind to For TransferEngine compatibility, empty metadata is accepted when an initialized engine is supplied and embedded-master mode is disabled. The regression initializes the caller-owned TCP engine separately, checks that its endpoint/metadata state is preserved, and exercises Put/Get through it. Setup without either metadata or an existing engine remains rejected. The PR title and description now reflect the existing narrowed scope: All applicable PR-scoped pre-commit hooks, changed-line clang-format 20 checks, Python/Shell syntax checks and CI YAML checks passed locally. Native C++ and wheel runtime regressions are pending in the new Linux CI run; they were not run on this macOS checkout. There are no inline review threads to mark resolved; these changes address the conversation comments above. Human review of the AI-assisted changes remains required before merge. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate issues affect metadata reuse and embedded-master port allocation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Adds opt-in in-process EmbeddedMaster support for Store tests and local evaluation without launching mooncake_master.
Changes:
- Adds embedded master lifecycle, listener handling, and setup options.
- Adds C++ and wheel E2E/regression coverage.
- Integrates embedded-master tests into CI.
- Outstanding issues include metadata-mode reuse, concurrent port allocation, and legacy default-port handling.
| File | Summary |
|---|---|
scripts/run_standalone_store_e2e.sh |
Runs embedded-master wheel E2E tests. |
mooncake-wheel/tests/test_standalone_store_e2e.py |
Covers embedded Store workflows. |
mooncake-store/tests/test_server_helpers.h |
Preserves the legacy test helper alias. |
mooncake-store/tests/standalone_client_test.cpp |
Tests embedded setup and engine reuse. |
mooncake-store/tests/embedded_master_test.cpp |
Tests listener scope and startup cleanup. |
mooncake-store/tests/e2e/readme.md |
Documents embedded-master E2E usage. |
mooncake-store/tests/CMakeLists.txt |
Registers the new tests. |
mooncake-store/src/real_client.cpp |
Configures and owns the embedded master. |
mooncake-store/src/http_metadata_server.cpp |
Applies the configured bind host. |
mooncake-store/src/embedded_master.cpp |
Implements embedded master lifecycle. |
mooncake-store/src/CMakeLists.txt |
Builds the embedded-master implementation. |
mooncake-store/include/types.h |
Adds the embedded-master configuration key. |
mooncake-store/include/real_client.h |
Extends setup internals and ownership. |
mooncake-store/include/embedded_master.h |
Declares the reusable master interface. |
mooncake-integration/store/store_py.cpp |
Exposes embedded-master setup to Python. |
.github/workflows/ci.yml |
Runs embedded-master E2E tests in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Teng Ma <stmatengss@gmail.com>
Adapt to upstream's in-process master support (kvcache-ai#3665): setup_internal gains enable_embedded_master; prefetch config parsing and bindings kept alongside the new setup plumbing.
…on API kvcache-ai#4401 removed Environ::GetBool and Environ::Get(), while two call sites that landed around the same time still use them, so main does not compile: - real_client.cpp (kvcache-ai#3665) calls Environ::GetBool for MOONCAKE_ENABLE_EMBEDDED_MASTER. Declare the variable in environment_variables.h and read it with Environ::ReadOr. - rdma_transport.cpp (kvcache-ai#4329, USE_CUDA only) calls Environ::Get().GetRdmaDataDirect(). Use globalConfig().rdma_data_direct, as kvcache-ai#4401 did in rdma_context.cpp. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kvcache-ai#3665 reads MOONCAKE_ENABLE_EMBEDDED_MASTER in real_client.cpp through Environ::GetBool, and kvcache-ai#4401, merged after it, removed that static helper. Each change built on its own branch, but main no longer compiles mooncake_store. Define MOONCAKE_ENABLE_EMBEDDED_MASTER in the environment variable catalog and read it with Environ::Process().GetTypedOr(..., false). This keeps the previous behavior: an unset value is false, the canonical boolean spellings are accepted, and an invalid value warns and falls back to false. (cherry picked from commit 25958ec)
Adapt to upstream's in-process master support (kvcache-ai#3665): setup_internal gains enable_embedded_master; prefetch config parsing and bindings kept alongside the new setup plumbing.
* [Bugfix][Store] Restore the build after Environ::GetBool removal #3665 reads MOONCAKE_ENABLE_EMBEDDED_MASTER in real_client.cpp through Environ::GetBool, and #4401, merged after it, removed that static helper. Each change built on its own branch, but main no longer compiles mooncake_store. Define MOONCAKE_ENABLE_EMBEDDED_MASTER in the environment variable catalog and read it with Environ::Process().GetTypedOr(..., false). This keeps the previous behavior: an unset value is false, the canonical boolean spellings are accepted, and an invalid value warns and falls back to false. * [Bugfix][TransferEngine] Read Data Direct from globalConfig in CUDA MR registration #4329 reads MC_RDMA_DATA_DIRECT in RdmaTransport::registerLocalMemoryInternal through Environ::Get().GetRdmaDataDirect(). #4401, merged after it, removed that accessor and moved the setting to GlobalConfig::rdma_data_direct. The call is inside #ifdef USE_CUDA, so only CUDA builds fail. Read globalConfig().rdma_data_direct, as rdma_context.cpp already does. Both parse MC_RDMA_DATA_DIRECT with TryParseBool and fall back to false on an invalid value.
Resolve conflicts between the NoF/SPDK decoupling and mainline work: - Env-var structuration PRs (kvcache-ai#4166/kvcache-ai#4170/kvcache-ai#4203): keep the neutral NoF naming/refactor while adopting the client-owned config structs (NoFDebugConfig, NoFWorkerPoolConfig, FilereadWorkerPoolConfig); QoS byte-limit vars stay in transfer_task.cpp, matching main (open kvcache-ai#4171). - kvcache-ai#3665 embedded master + P2PHANDSHAKE metadata resolution compose with the added nof_initiator injection in Client::Create. - kvcache-ai#4070 TENT native API layered onto the refactored TransferSubmitter. - kvcache-ai#3717 (ShmHelper/receiver shm SPDK registration) is ported from the deleted SpdkWrapper singleton onto the NVMeoFInitiator interface and the process-global NofPageRegistry: registrations normalize to 2MB pages with per-page refcounts, -EBUSY pages are absorbed as external, and a failed Register owns no registration. ShmHelper's singleton is now deliberately leaked so exit-time cleanup can never touch a torn-down registry/env; the SpdkEnvGuard process pin makes the old construction-ordering hack unnecessary.
Bind test listeners to kernel-assigned ports, publish their actual ports, and clean up partial startup failures. Preserve explicit metadata disablement and same-port reconnect behavior. Backport the relevant changes from upstream kvcache-ai#2942 and kvcache-ai#3665. Expose HTTP startup status and the bound port through the existing centralized wrapper without importing the admin/HA refactor. Validation: static source comparison and git diff --check. No builds or runtime tests were run.
… compatibility coverage (#5107) * test(store): consolidate RealClient deployment regressions Move shared registration failure cases into deployment coverage, restore the pybind test target name, and preserve empty RDMA device semantics in the copy/move test. * test(store): restore client integration baseline and isolate replica tests * test(store): remove Redis and runtime config cases * test(store): restore client metrics baseline and move label helper coverage * test(store): restore A00 CXL integration tests * test(store): remove redundant tiered backend init helper * test(store): backport upstream in-process master startup fixes Bind test listeners to kernel-assigned ports, publish their actual ports, and clean up partial startup failures. Preserve explicit metadata disablement and same-port reconnect behavior. Backport the relevant changes from upstream #2942 and #3665. Expose HTTP startup status and the bound port through the existing centralized wrapper without importing the admin/HA refactor. Validation: static source comparison and git diff --check. No builds or runtime tests were run. * test(store): register SHM ping tests for both architectures * test(store): restore A00 IPv6 client fixtures * test(store): restore A00 storage backend tests * test(store): restore baseline stress workload calls * test(store): restore A00 e2e wrapper get and put * test(store): isolate e2e master startup options * fix(p2p): stop metric reporting before storage teardown Expose StopMetricReporting() to synchronously join the reporter and clear its interval. Reuse it from reporting restart, destruction and service shutdown, after recovery cancellation and RPC draining but before stopping or destroying the data plane. Document borrowed tier callback lifetimes and retain the controlled StopJoinsMetricReaderBeforeDestroy regression. Update existing reporting tests to use the explicit stop API. Validation: the original shutdown fix and controlled regression passed all 56 non-ASAN HA integration cases on the development host. The explicit stop API revision was checked statically only; it has not been compiled or run. No new runtime validation is claimed for this revision. * test(p2p): use registered pool buffers in integration tests Prepare input and output buffers with MakeBuffer overloads returning span<char> from each client's registered pool. Retain BufferHandles for the test fixture lifetime, use spans for batch payloads, and reuse the pool for allocator-based BatchGet. Keep native transfer calls and the explicit external-registration cases intact. Compare buffer sizes with fatal assertions before checking bytes with memcmp. Reuse existing size guards for allocator results, avoid temporary string copies, and preserve binary content checks. Validation: formatting, diff and static scope checks; 38 test cases and 58 native transfer calls are unchanged. All 17 adapted buffer comparisons have fatal size guards. This pooled-buffer version has not been compiled or run. * test(wheel): add P2P CLI startup coverage Keep the centralized startup flow and add a separate P2P function with its RPC flags and an inline DRAM tier configuration. Select the function from the master version output and return failure when the client fails to start. Validation: Python syntax, inline JSON, centralized-flow comparison and git diff --check. The simplified startup flows have not been run. * test(store): cover AutoPortBinder rebind lifecycle * test(store): clarify SHM ping coverage and bound lock wait * test(store): strengthen inflight tracker lifecycle coverage Check rejected guard destruction and coordinate single-request drain observations. Cover multiple guards and waiters, default hooks, hook completion ordering, and concurrent admission versus close; add a 60-second CTest timeout. Validation: static diff checks only. Builds and runtime tests were not run. * test: remove unused dummy client imports * fix(store): restore P2P Python setup compatibility * test(store): exercise deployment lifecycle through IPC and RPC * test(store): add dual-architecture Python coverage * test: remove IPC collision lifecycle case * test(store): register write RPCs in paused master fixture * fix format --------- Co-authored-by: wanyue.wy <wanyue.wy@oceanbase.com>
Description
Promote the existing test-only
InProcMasterto a reusableEmbeddedMasterowned byRealClient, so in-process tests and local evaluation can exercise Store Put/Get without launchingmooncake_master. This is opt-in test/evaluation plumbing, not an additional client hosting or deployment mode. In an application using the embedded client, the application, client and master share one process. Existing multi-node/HA deployments continue to use a dedicated master.In embedded mode, the external
master_server_addrargument is ignored and each client owns an independent master.The opt-in is
enable_embedded_master=Truein Python setup,"enable_embedded_master": truein a setup dictionary, orMOONCAKE_ENABLE_EMBEDDED_MASTER=true. Missing metadata defaults toP2PHANDSHAKEonly for the embedded-master path. There is no new standalone-service or CLI deployment mode.Review fixes:
127.0.0.1. Honor the metadata server's existing host argument, allocate automatic ports by binding port zero, and clean up listeners after failed startup. Preserve the last bound addresses after Stop for same-port restart.main, preserving the current allocation paths, Python GIL release, CI structure and non-RDMA build guards.Module
mooncake-transfer-engine)mooncake-store)mooncake-integration)mooncake-wheel)Type of Change
How Has This Been Tested?
The previous head
49c2a59apassed wheel builds and its new standalone/listener tests, but failed two existing CTest reconnect cases because Stop cleared the master ports. Follow-up5b3925fdpreserves those addresses and adds a focused restart regression. Hosted Linux CI has now passed at5b3925fd260cd48bb337f2601a02a325cdadfb70: CI run 35978331126. CTest passed all 233 tests, including the reconnect and embedded-master regressions. The TENT cuda-off job passed both Store completion/engine-reuse tests and all 65 TENT tests. All wheel builds, both Ubuntu wheel test jobs, all four TENT configurations, lint/docs checks and CI Gate passed.Local checks passed on macOS:
pre-commit run --files $(git diff --name-only --diff-filter=ACMR origin/main...HEAD) bash scripts/code_format.sh --base origin/main --changed-lines --check git diff --check bash -n scripts/run_standalone_store_e2e.shPython AST parsing and CI YAML parsing/order checks also passed. CodeRabbit reviewed all 16 changed files at
49c2a59aand the 14-file follow-up at5b3925fd, raising 0 issues in each review. Native C++ compilation and runtime tests were not run locally: this macOS checkout has no Mooncake Linux build environment. The following regressions ran successfully in hosted Linux CTest and wheel CI at the updated head:ctest --test-dir build --output-on-failure \ -R '^(embedded_master_test|standalone_client_test|standalone_client_tent_test|http_metadata_server_test|pybind_client_test|client_metrics_test|non_ha_reconnect_test)$' bash scripts/run_standalone_store_e2e.shTest results:
Checklist
./scripts/code_format.shAI Assistance Disclosure
Codex assisted with merging
main, resolving review comments, adding regression coverage and local validation. CodeRabbit provided an additional review. The human submitter must review and understand all changed lines before merge.