Repository navigation
[TE][Store] refactor: centralize environment variable handling across components - #1538
stmatengss wants to merge 20 commits into
Conversation
Summary of ChangesHello @stmatengss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant refactoring effort to centralize how environment variables are accessed throughout the Mooncake codebase. By moving all environment variable retrieval into a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors environment variable access across the codebase by introducing a new Environ singleton class. The changes involve adding numerous getter methods and corresponding member variables to mooncake-common/include/environ.h and initializing them in mooncake-common/src/environ.cpp. Subsequently, all direct calls to std::getenv() for MC_ prefixed environment variables are replaced with calls to Environ::Get().Get<EnvVarName>() in various files, including mooncake-store/src/client_metric.cpp, client_service.cpp, ha_helper.cpp, master.cpp, transfer_task.cpp, and mooncake-transfer-engine/src/config.cpp, topology.cpp, transfer_engine.cpp, transfer_engine_impl.cpp, transfer_metadata.cpp, transfer_metadata_plugin.cpp, transport/cxl_transport/cxl_transport.cpp, transport/hip_transport/hip_transport.cpp, transport/nvlink_transport/nvlink_transport.cpp, and transport/rpc_communicator/rpc_communicator.cpp. Review comments highlight a missing closing brace in client_metric.cpp causing a compilation error, a broken fallback logic for MC_GID_INDEX to NCCL_IB_GID_INDEX in config.cpp due to the new Environ class's default return value, a risky C-style cast hiding a const_cast in cxl_transport.cpp when assigning cxl_dev_path, and a suggestion to change MC_STORE_MEMCPY and MC_STORE_USE_HUGEPAGE to return bool instead of std::string for consistency.
| } else { | ||
| LOG(WARNING) |
There was a problem hiding this comment.
There appears to be a missing closing brace } after the if (interval > 0) block on line 568. This is a syntax error that will cause a compilation failure.
} else if (interval < 0) {
LOG(WARNING)| int gid_index = env.GetGidIndex(); | ||
| if (gid_index == 0) { | ||
| const char *nccl_gid = std::getenv("NCCL_IB_GID_INDEX"); | ||
| if (nccl_gid) gid_index = atoi(nccl_gid); | ||
| } | ||
|
|
||
| const char *mtu_length_env = std::getenv("MC_MTU"); | ||
| if (mtu_length_env) { | ||
| size_t val = atoi(mtu_length_env); | ||
| if (val == 512) | ||
| config.mtu_length = IBV_MTU_512; | ||
| else if (val == 1024) | ||
| config.mtu_length = IBV_MTU_1024; | ||
| else if (val == 2048) | ||
| config.mtu_length = IBV_MTU_2048; | ||
| else if (val == 4096) | ||
| config.mtu_length = IBV_MTU_4096; | ||
| else { | ||
| LOG(ERROR) << "Ignore value from environment variable MC_MTU, it " | ||
| "should be 512|1024|2048|4096"; | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| if (gid_index >= 0 && gid_index < 256) | ||
| config.gid_index = gid_index; |
There was a problem hiding this comment.
The fallback logic for MC_GID_INDEX to NCCL_IB_GID_INDEX appears to be broken with this change. env.GetGidIndex() returns a default value of 3 if MC_GID_INDEX is not set, so the if (gid_index == 0) block is not executed, and the fallback to NCCL_IB_GID_INDEX is missed. Also, if a user explicitly sets MC_GID_INDEX=0, the current logic would incorrectly fall back to NCCL_IB_GID_INDEX.
The original logic of checking MC_GID_INDEX first, then NCCL_IB_GID_INDEX, should be preserved. A proper fix would be to handle this fallback logic inside the Environ class itself for better encapsulation.
| if (!env_cxl_dev_path.empty()) { | ||
| LOG(INFO) << "MC_CXL_DEV_PATH: " << env_cxl_dev_path; | ||
| cxl_dev_path = (char *)env_cxl_dev_path; | ||
| cxl_dev_path = (char *)env_cxl_dev_path.c_str(); |
There was a problem hiding this comment.
The C-style cast (char *) hides a const_cast. The member cxl_dev_path is a char*, but it's being assigned from env_cxl_dev_path.c_str() which returns a const char*. Modifying the string literal returned by c_str() is undefined behavior. It would be safer to change the type of cxl_dev_path to std::string or const char* to avoid this risky cast.
| std::string GetStoreMemcpy() const { return store_memcpy_; } | ||
| std::string GetStoreUseHugepage() const { return store_use_hugepage_; } |
There was a problem hiding this comment.
The environment variables MC_STORE_MEMCPY and MC_STORE_USE_HUGEPAGE seem to be boolean flags. However, they are defined here to return std::string. For consistency with other boolean flags like GetUseHipIpc and to simplify usage, consider changing them to return bool and use GetBool in the implementation. This would also require changing the corresponding member variables store_memcpy_ and store_use_hugepage_ to bool.
stmatengss
changed the title
f2389e3 to
b220000
Compare
|
Glad that we now have a unified place to set and retrieve environment variables. I believe this will make things much more convenient for users. |
|
LGTM for TE part. |
alogfans
left a comment
alogfans
left a comment
There was a problem hiding this comment.
Before merging it, pls fix the build issue of Mooncake TE.
|
@copilot Please find a solution for this failing job. Use the logs, job definition, and any referenced files where the failure occurred. Keep your response focused on the solution and include code suggestions when appropriate |
|
@stmatengss I've opened a new pull request, #1665, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Refactors Mooncake Transfer Engine and Mooncake Store to centralize environment-variable access through mooncake-common’s Environ singleton, reducing direct getenv() usage and consolidating parsing/defaults.
Changes:
- Extend
mooncake-commonEnvironwith additional getters and wiremooncake_commoninto TE build/link paths. - Migrate multiple TE/store components (transports, topology, metadata, master/client paths) from
getenv()toEnviron::Get()getters. - Make CXL-related tests skip cleanly when CXL is unavailable.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| mooncake-transfer-engine/tests/cxl_transport_test.cpp | Skip CXL tests when device/transport install isn’t available; minor test robustness tweaks. |
| mooncake-transfer-engine/src/transport/rpc_communicator/rpc_communicator.cpp | Use Environ for MC_RPC_PROTOCOL. |
| mooncake-transfer-engine/src/transport/nvlink_transport/nvlink_transport.cpp | Switch NVLink IPC/fabric decision to Environ. |
| mooncake-transfer-engine/src/transport/intranode_nvlink_transport/intranode_nvlink_transport.cpp | Add non-CUDA stubs returning NotImplemented when USE_CUDA is off. |
| mooncake-transfer-engine/src/transport/hip_transport/hip_transport.cpp | Switch HIP stream/event counts + IPC/fabric decision to Environ. |
| mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp | Use Environ for CXL device path/size; modernize cxl_dev_path storage. |
| mooncake-transfer-engine/src/transfer_metadata_plugin.cpp | Use Environ for Redis password/DB index. |
| mooncake-transfer-engine/src/transfer_metadata.cpp | Use Environ for metadata cluster ID prefix. |
| mooncake-transfer-engine/src/transfer_engine_impl.cpp | Use Environ for multiple TE env toggles (RPC binding, CXL, topology JSON, transport forcing, metrics interval). |
| mooncake-transfer-engine/src/transfer_engine.cpp | Use Environ to select TENT/TEv1 modes. |
| mooncake-transfer-engine/src/topology.cpp | Use Environ for round-robin path selection. |
| mooncake-transfer-engine/src/config.cpp | Refactor global config env parsing to use Environ getters. |
| mooncake-transfer-engine/src/CMakeLists.txt | Link TE against mooncake_common. |
| mooncake-transfer-engine/include/transport/cxl_transport/cxl_transport.h | Replace char* path with std::string, add member initialization defaults. |
| mooncake-transfer-engine/CMakeLists.txt | Add mooncake-common subdir for standalone TE builds. |
| mooncake-store/tests/cxl_client_integration_test.cpp | Skip some CXL tests when client creation fails / CXL unavailable. |
| mooncake-store/src/transfer_task.cpp | Use Environ for MC_STORE_MEMCPY. |
| mooncake-store/src/real_client.cpp | Include environ.h (prep for centralized env usage). |
| mooncake-store/src/master.cpp | Use Environ for MC_RPC_PROTOCOL. |
| mooncake-store/src/ha_helper.cpp | Use Environ for MC_STORE_CLUSTER_ID. |
| mooncake-store/src/client_service.cpp | Use Environ for auto-discovery config and filters. |
| mooncake-store/src/client_metric.cpp | Use Environ for client metrics enable/interval. |
| mooncake-p2p-store/build.sh | Link mooncake_common and add its library path. |
| mooncake-common/src/environ.cpp | Add cached parsing for many additional env vars. |
| mooncake-common/include/environ.h | Add getters + stored fields for new env vars. |
| mooncake-common/common.cmake | Make sccache optional when ENABLE_SCCACHE=ON, with warning if missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| std::string GetTentConf() const { return tent_conf_; } | ||
| int GetTransferTimeout() const { return transfer_timeout_; } | ||
| bool GetUseHipIpc() const { return use_hip_ipc_; } | ||
| bool GetUseNvlinkIpc() const { return use_nvlink_ipc_; } |
There was a problem hiding this comment.
Fixed in c187d54. The nvlink_transport.cpp now uses GetNvlinkFabricMemEnabled() which returns true only when MC_USE_NVLINK_IPC is explicitly set to "0", preserving the tri-state contract. Also fixed the inverted supportFabricMem() logic in hip_transport.cpp and the GID index default/fallback handling.
| auto &env = Environ::Get(); | ||
| std::string password_str = env.GetRedisPassword(); | ||
|
|
||
| uint8_t db_index = 0; | ||
| const char *db_index_str = std::getenv("MC_REDIS_DB_INDEX"); | ||
| if (db_index_str) { | ||
| try { | ||
| int index = std::stoi(db_index_str); | ||
| if (index >= 0 && index <= 255) { | ||
| db_index = static_cast<uint8_t>(index); | ||
| } else { | ||
| LOG(WARNING) << "Invalid Redis DB index: " << index | ||
| << ", using default 0"; | ||
| } | ||
| } catch (const std::exception &e) { | ||
| LOG(WARNING) | ||
| << "Failed to parse MC_REDIS_DB_INDEX: " << e.what() | ||
| << ", using default 0"; | ||
| } | ||
| int db_index_val = env.GetRedisDbIndex(); | ||
| if (db_index_val >= 0 && db_index_val <= 255) { | ||
| db_index = static_cast<uint8_t>(db_index_val); | ||
| } else if (db_index_val != 0) { | ||
| LOG(WARNING) << "Invalid Redis DB index: " << db_index_val | ||
| << ", using default 0"; |
| // By default, use IPC mode | ||
| // Fabric memory is enabled only when MC_USE_HIP_IPC=false | ||
| bool hip_ipc = Environ::Get().GetUseHipIpc(); | ||
| if (!hip_ipc) { |
| static bool supportFabricMem() { | ||
| if (getenv("MC_USE_NVLINK_IPC")) return false; | ||
| if (Environ::Get().GetUseNvlinkIpc()) return false; | ||
|
|
| int gid_index = env.GetGidIndex(); | ||
| if (gid_index == 0) { | ||
| const char *nccl_gid = std::getenv("NCCL_IB_GID_INDEX"); | ||
| if (nccl_gid) gid_index = atoi(nccl_gid); | ||
| } | ||
|
|
||
| const char *mtu_length_env = std::getenv("MC_MTU"); | ||
| if (mtu_length_env) { | ||
| size_t val = atoi(mtu_length_env); | ||
| if (val == 512) | ||
| config.mtu_length = IBV_MTU_512; | ||
| else if (val == 1024) | ||
| config.mtu_length = IBV_MTU_1024; | ||
| else if (val == 2048) | ||
| config.mtu_length = IBV_MTU_2048; | ||
| else if (val == 4096) | ||
| config.mtu_length = IBV_MTU_4096; | ||
| else { | ||
| LOG(ERROR) << "Ignore value from environment variable MC_MTU, it " | ||
| "should be 512|1024|2048|4096"; | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| if (gid_index >= 0 && gid_index < 256) config.gid_index = gid_index; |
| static std::string metadata_url_; | ||
| static UUID test_client_id_; | ||
| static inline bool is_cxl = false; | ||
| static inline bool cxl_available_ = true; | ||
| static int tmp_fd; |
|
@stmatengss I've opened a new pull request, #1686, to work on those changes. Once the pull request is ready, I'll request review from you. |
190a01c to
0bac351
Compare
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Review Summary
Centralizing env handling into Environ and wiring it through TE, Store, EP, and Tent is a solid refactor that improves consistency and makes a lot of configuration paths easier to reason about. The added dynamic getter tests and CXL test hardening are good steps toward stability. The remaining concerns are mostly around diagnosability and a few brittle semantics for Redis and NVLink env-driven behavior.
🛡️ Key Risks & Issues
- Redis DB index and auth configuration: The new Environ-backed Redis DB index handling in redis_connection.cpp, metastore.cpp, and transfer_metadata_plugin.cpp tightens validation but makes misconfigurations harder to see. Invalid MC_REDIS_DB_INDEX values now cause connection failures with little context, and there’s no explicit logging of the offending value. This can turn a small config error into an operational incident that looks like Redis unavailability.
- NVLink fabric-mem opt-in semantics: GetNvlinkFabricMemEnabled and its use in nvlink_transport.cpp and PG’s connection_poller hinges on MC_USE_NVLINK_IPC being exactly "0" to enable fabric-mem. That’s a brittle magic-string convention that doesn’t align with the more general GetBool semantics you introduced, and makes it easy for operators to think "false" or "off" will flip the mode when they don’t.
- Boolean env parsing surprises: The more permissive GetBool helper now treats a blank value and several tokens as false and logs warnings to stderr only. Because many critical toggles rely on this, and stderr isn’t always captured in production, a subtle mis-typed env value can quietly fall back to the default without durable diagnostics.
🧪 Verification Advice
- For Redis-backed HA and metadata deployments, verify behavior under: valid MC_REDIS_DB_INDEX, out-of-range values (e.g. 256), and malformed strings, and confirm that logs clearly indicate configuration errors rather than generic connection failures.
- For NVLink and HIP transports, exercise MC_USE_NVLINK_IPC and MC_USE_HIP_IPC combinations (unset, "0", "1", and common boolean tokens) and confirm fabric-mem vs IPC modes behave as intended in both TE and PG paths.
- In a staging environment, run through a set of deliberately malformed boolean env values (for WITH_NVIDIA_PEERMEM, MC_DISABLE_GPU_DIRECT_RDMA, MC_USE_TENT, MC_USE_UBSHMEM_IPC) and check that the resulting mode, logs, and performance match expectations.
💡 Thoughts & Suggestions
- Consider adding a small layer of explicit logging around Environ::GetRedisDbIndex and the Redis connection helpers so that invalid DB indices or auth combinations are clearly surfaced at startup.
- It may be worthwhile to either document the MC_USE_NVLINK_IPC="0" convention very prominently or adjust GetNvlinkFabricMemEnabled to honor the same boolean vocabulary as GetBool, making operator-facing behavior more intuitive.
- Overall the direction of the refactor is good; adding a handful of focused tests for the most critical env-driven behaviors (Redis, RPC protocol and timeouts, NVLink/HIP fabric-mem) would significantly increase confidence before rolling this widely.
🤖 Generated by Qoder • View workflow run
| @@ -74,11 +75,10 @@ tl::expected<int, ErrorCode> ParsePositiveInt(std::string_view text, | |||
| } | |||
|
|
|||
| tl::expected<int, ErrorCode> ResolveRedisDbIndex() { | |||
There was a problem hiding this comment.
The new Redis DB index handling changes the failure mode in a way that could make configuration issues harder to diagnose.
In ResolveRedisDbIndex (mooncake-store/src/ha/common/redis/redis_connection.cpp:77), any value outside [0,255] now returns ErrorCode::INVALID_PARAMS based on Environ::Get().GetRedisDbIndex(), and ConnectRedis immediately propag that as a generic connection_error without logging which MC_REDIS_DB_INDEX value was problematic. Previously, the DB index was parsed directly from the env string and either coerced or at least validated with ParsePositiveInt, which gave clearer feedback.
For operators, a typo like MC_REDIS_DB_INDEX=256 or a negative value will now surface as "Redis connection failed" with no hint that the DB index itself is invalid. On a busy HA deployment this looks more like an availability incident than a simple configuration bug.
Consider adding an explicit log on INVALID_PARAMS that includes the raw DB index value, or tightening Environ::GetRedisDbIndex to validate the range and log once at startup. That way misconfigurations fail fast and leave a clear breadcrumb, while still keeping the stricter range checking you introduced.
🤖 Generated by Qoder • Fix in Qoder
| @@ -315,7 +316,7 @@ static int getNumDevices() { | |||
| } | |||
|
|
|||
| static bool supportFabricMem() { | |||
There was a problem hiding this comment.
The tight coupling between MC_USE_NVLINK_IPC and the new GetNvlinkFabricMemEnabled() helper makes the fabric-mem opt-in semantics a bit brittle.
GetNvlinkFabricMemEnabled (mooncake-transfer-engine/include/environ.h:226–230) returns true only when the raw MC_USE_NVLINK_IPC string is exactly "0"; any other value (including common booleans like "false" or "off") is treated as "do not enable fabric-mem". That value is now fed into the NVLink supportFabricMem logic here and into the PyTorch backend’s connection_poller, so operators have to know about the special "0" magic literal to activate fabric memory.
Given that other flags now use GetBool with a richer vocabulary ("true", "on", "enable", etc.), this tri-state behavior is easy to misconfigure: someone might set MC_USE_NVLINK_IPC="false" expecting IPC to be disabled and fabric-mem to be enabled, but the code will quietly keep fabric-mem off. At scale that shows up as performance differences rather than a clear error.
It may be worth either documenting the "0" requirement very prominently, or relaxing GetNvlinkFabricMemEnabled to treat a recognized "false" token (per GetBool semantics) as fabric-mem opt-in as well, so the behavior matches operator intuition while still distinguishing unset from "off".
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
👋 Review Summary
This refactor does a solid job of centralizing environment-variable handling into the Environ singleton across transfer engine, store, EP, and tent, fixing several legacy issues (GID index defaulting, NVLink fabric mem semantics, CXL test flakiness) while wiring mooncake_common into all the relevant build paths. The Environ API looks coherent, and most of the call sites now consistently rely on strongly-typed getters instead of ad-hoc getenv parsing.
🛡️ Key Risks & Issues
- HIP/NVLink fabric-mem gating: the new supportFabricMem() logic in HipTransport now couples HIP fabric enablement to both
GetUseHipIpc()andGetNvlinkFabricMemEnabled(). BecauseGetUseHipIpc()defaults to true andGetNvlinkFabricMemEnabled()only becomes true whenMC_USE_NVLINK_IPC="0", the default "envs unset" case disables HIP fabric entirely, which may not match prior behavior or the intent described in the comment. This could silently prevent fabric-mem from being used in deployments that previously relied onMC_USE_HIP_IPC=0without any NVLink-specific configuration. - Redis DB index consistency: store-side Redis connection and tent metastore both now depend on Environ’s
GetRedisDbIndex(). The store HA path correctly rejects out-of-range DB indices withINVALID_PARAMS, while the tent metastore still silently falls back toREDIS_DEFAULT_DB_INDEXwhen the getter returns an invalid value. This is mostly a semantics issue, but it means misconfigured DB indices are handled differently depending on which component is reading them. - Dynamic vs cached env semantics: the PR introduces a mix of cached (constructor-time) and dynamic (per-call) getters. The tests for dynamic getters cover a good subset of new fields (CXL path, PKEY index, RPC timeouts, client metrics, hot cache), but the static vs dynamic distinction is subtle and not yet documented in tests, so future changes could inadvertently move a setting from one category to the other and surprise users who expect runtime changes to be visible.
🧪 Verification Advice
- HIP/NVLink fabric: explicitly verify behavior under the main env combinations by running focused HIP transport tests or small integration checks with different settings for
MC_USE_HIP_IPCandMC_USE_NVLINK_IPC(unset, "0", "1") and confirm that fabric mem is enabled only when desired. A small unit test aroundsupportFabricMem()would make this safer. - Redis DB index and auth: add tests that set
MC_REDIS_DB_INDEXto valid and invalid values and assert consistent handling between store HA and tent metastore, plus basic authentication coverage forMC_REDIS_USERNAME/MC_REDIS_PASSWORDto ensure the new%bcalls behave correctly. - RPC protocol and timeout: run the existing
RpcTimeoutTestplus a simple master+client setup withMC_RPC_PROTOCOL="rdma"to confirm that RDMA sockets are used uniformly by master, client, and RpcCommunicator, and that malformed timeout envs cleanly fall back to defaults.
💡 Thoughts & Suggestions
- Consider documenting the intended matrix for HIP/NVLink IPC vs fabric mem in one place (even just in code comments near Environ getters), so future refactors have a clear behavioral target.
- Where reasonable, prefer having invalid env values either fail fast or log at a single central point in Environ, rather than diverging behavior across components; this would make misconfiguration easier to diagnose.
- Overall this is a valuable cleanup—completing the test coverage for the new Environ semantics (especially around transports, Redis, and hot cache) will make it much easier to safely evolve configuration knobs in subsequent changes.
🤖 Generated by Qoder • View workflow run
| static bool supportFabricMem() { | ||
| // By default, use IPC mode. Fabric memory is enabled only when | ||
| // MC_USE_HIP_IPC=0 or MC_USE_NVLINK_IPC=0 is explicitly set. | ||
| const char* hip_ipc = getenv("MC_USE_HIP_IPC"); | ||
| const char* nvlink_ipc = getenv("MC_USE_NVLINK_IPC"); | ||
|
|
||
| bool fabric_enabled = (hip_ipc && strcmp(hip_ipc, "0") == 0) || | ||
| (nvlink_ipc && strcmp(nvlink_ipc, "0") == 0); | ||
|
|
||
| if (!fabric_enabled) { | ||
| bool hip_ipc = Environ::Get().GetUseHipIpc(); |
There was a problem hiding this comment.
The refactor to Environ-based gating for HIP/NVLink fabric memory changes how mixed HIP/NVLink deployments behave and may not match the intent described in the comment.
Today supportFabricMem() requires both GetUseHipIpc() and GetNvlinkFabricMemEnabled() to line up as hip_ipc && !nvlink_fabric to disable fabric. In particular:
GetNvlinkFabricMemEnabled()only returns true whenMC_USE_NVLINK_IPCis explicitly set to "0"; any other value (including unset) is treated as "fabric disabled".GetUseHipIpc()defaults to true whenMC_USE_HIP_IPCis unset.
Under those defaults, HIP fabric memory is always disabled even when the NVLink path is available and configured to use IPC by default. Previously, fabric could be enabled for HIP when either MC_USE_HIP_IPC=0 or MC_USE_NVLINK_IPC=0 was set, while leaving the common "unset" case in IPC mode. If the desired behavior is "only opt into HIP fabric when we explicitly turn off HIP IPC or explicitly opt into NVLink fabric", we may want to gate on the raw env state more directly, e.g.:
- Treat
MC_USE_NVLINK_IPCunset as "IPC mode" rather than "fabric disabled". - Require
MC_USE_HIP_IPC=0(explicit) to enable HIP fabric regardless of NVLink settings, instead of coupling HIP fabric strictly to NVLink fabric.
It would be good to double-check the intended matrix for HIP vs NVLink fabric/IPCs and consider a small unit test that exercises combinations of MC_USE_HIP_IPC and MC_USE_NVLINK_IPC so we don’t accidentally disable fabric on deployments that rely on it.
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
👋 Review Summary
This is a substantial, well‑structured refactor that centralizes environment variable handling into Environ across transfer engine, store, EP/PG, Tent, HA, and build tooling. It also fixes a few subtle semantics (fabric‑mem opt‑in, GID index fallback, Redis DB index parsing) and hardens CXL‑related tests, which collectively improve correctness, debuggability, and CI stability.
🛡️ Key Risks & Issues
- Fabric‑memory enablement (NVLink, HIP, UBSHMEM) now depends on new
Environgetters likeGetNvlinkFabricMemEnabled()andGetHipFabricMemEnabled(). The matrix tested inenviron_test.cpplooks sound, but it’s worth being mindful that misconfigured envs could silently flip transports between IPC and fabric modes; staging coverage with representative env combinations will help avoid surprises. - Redis configuration is now strictly validated via
Environ::GetRedisDbIndex()and propagated through HA and Tent metastore. This is an improvement, but operators should note that invalidMC_REDIS_DB_INDEXwill yield-1and log warnings; deployments relying on non‑default DBs should be checked after upgrading. - Store hot cache and hugepage settings, TCP/RPC protocol selection, and CXL availability are now more tightly controlled via env‑driven getters and new test skips. The behaviors I inspected are consistent with previous intent, but these are high‑impact knobs for performance and availability, so I’d recommend doing a focused smoke run in each major deployment profile (RDMA/TCP, HIP/NVLink, CXL/no‑CXL) when rolling this out.
🧪 Verification Advice
- For transports, run end‑to‑end data‑path tests in environments that exercise both IPC and fabric‑mem modes (varying
MC_USE_HIP_IPC,MC_USE_NVLINK_IPC,MC_USE_UBSHMEM_IPC) and confirm latency/throughput are in line with expectations and no transport unexpectedly falls back. - For Redis, configure
MC_REDIS_DB_INDEXto a non‑zero value in a test cluster and verify that both HA components and Tent metastore talk to the intended logical DB; also try an invalid value to confirm the new error handling behaves as intended. - For store hot cache and RPC, run a load test with realistic
MC_STORE_LOCAL_HOT_CACHE_*andMC_RPC_*envs and observe metrics/logs to ensure cache admission, eviction, and RPC timeouts align with existing SLOs.
💡 Thoughts & Suggestions
- The
Environsurface is getting quite rich; the added unit tests for dynamic getters and fabric‑mem matrix are very helpful. Over time, it might be worth grouping related getters (e.g., transport vs store vs HA) in small helper structs or namespaces to make it easier for new contributors to navigate. - The non‑CUDA stub for
IntraNodeNvlinkTransportis a good defensive addition. If you expect non‑GPU builds to be common, a small test that asserts the NotImplemented behavior and log message would make this contract explicit and prevent regressions. - Overall the refactor is thoughtful and consistent; once the minor include duplication in RDMA context is cleaned up, this looks ready to ship after the targeted staging runs mentioned above.
🤖 Generated by Qoder • View workflow run
| #include "config.h" | ||
| #include "environ.h" | ||
| #include "cuda_alike.h" | ||
| #include "environ.h" |
There was a problem hiding this comment.
In rdma_context.cpp the header "environ.h" is included twice near the top of the file (#include "environ.h" appears back‑to‑back after the existing includes). Functionally this is harmless because of include guards, but it’s unnecessary duplication in a hot path and makes it slightly harder to scan dependencies.
Since this PR is already focused on centralizing env handling, it would be cleaner to drop the redundant second include so RDMA context has a single, clear dependency on Environ.
🤖 Generated by Qoder • Fix in Qoder
Description
Centralizes all
getenv()calls across mooncake-transfer-engine and mooncake-store into theEnvironsingleton inmooncake-common/environ.h. Eliminates scattered env var parsing, consolidates defaults, and fixes several semantic bugs discovered during migration.Key changes:
GetRedisUsername(),GetNvlinkFabricMemEnabled()(tri-state: returns true only whenMC_USE_NVLINK_IPCis explicitly"0"), and ~30 other getters coveringMC_MAX_SGE,MC_MAX_WR,MC_MAX_INLINE,MC_MTU,MC_WORKERS_PER_CTX,MC_SLICE_SIZE,MC_RETRY_CNT, etc.3→-1(auto-selection),NCCL_IB_GID_INDEXfallback now handled inside Environ instead of config.cppsupportFabricMem()had inverted logic — was disabling fabric-mem when IPC was disabled. Fixed:if (hip_ipc) return false;GetUseNvlinkIpc()(bool, can't distinguish unset from"0") toGetNvlinkFabricMemEnabled()to preserve opt-in semanticsnullptr, addcxl_available_skip guard toEvictOperationtestmooncake_commonlinked into standalone TE builds, Python extension builds, P2P Store builds, Rust builds, and CI Go test CGO_LDFLAGS (including-lmlx5,-lxxhash)libmooncake_common.atomooncake-store/rust/build.rslink list to resolveundefined symbol: mooncake::Environ::Get()at Rust build timeopendir()withstd::filesystem::is_directory()(from main)Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-pg)mooncake-rl)Type of Change
How Has This Been Tested?
environ.cpp,config.cpp,transfer_metadata_plugin.cpp)GetRedisUsername()after main merge identified and fixedundefined symbol: mooncake::Environ::Get()) identified and fixed by addingmooncake_commonto Rust link list./scripts/code_format.shChecklist
./scripts/code_format.shbefore submitting.