[Store] Single Go HA-wrapper for etcd+k8s#2643
Conversation
Signed-off-by: Vladislav Nosivskoy <vladnosiv@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request consolidates the separate Go-based HA wrappers for etcd and Kubernetes lease into a single unified Go HA wrapper library under mooncake-common/ha-wrapper, updating the CMake build configurations, scripts, and bindings across the repository. Feedback on these changes suggests removing go mod tidy from the CMake build phase to support offline builds, committing the missing go.sum file, and downgrading the Go toolchain version in go.mod from the non-existent 1.25.0 to a stable release like 1.24.0 to avoid compilation failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| add_custom_command( | ||
| OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.so | ||
| ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.h | ||
| COMMAND bash -c "go mod tidy" | ||
| COMMAND | ||
| bash -c | ||
| "go build -buildmode=c-shared -o ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.so etcd_wrapper.go k8s_lease_wrapper.go" | ||
| COMMAND | ||
| ${CMAKE_COMMAND} -E copy | ||
| ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.h | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/libmooncake_ha_wrapper.h | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| COMMENT "Building unified Go HA wrapper" | ||
| DEPENDS etcd_wrapper.go k8s_lease_wrapper.go go.mod) |
There was a problem hiding this comment.
Running go mod tidy during the CMake build phase is an anti-pattern. It requires internet access to fetch dependencies, which will fail in offline or restricted build environments (e.g., secure CI/CD, hermetic package builds). It also modifies the source directory during the build, violating the out-of-source build principle. go mod tidy should be run manually by developers during development, and the resulting go.sum should be committed to the repository.\n\nAdditionally, using bash -c to execute go build is unnecessary and reduces portability. Since CMake executes multiple COMMAND arguments sequentially, you can invoke go directly.
add_custom_command(\n OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.so\n ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.h\n COMMAND\n go build -buildmode=c-shared -o ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.so etcd_wrapper.go k8s_lease_wrapper.go\n COMMAND\n ${CMAKE_COMMAND} -E copy\n ${CMAKE_CURRENT_BINARY_DIR}/libmooncake_ha_wrapper.h\n ${CMAKE_CURRENT_SOURCE_DIR}/libmooncake_ha_wrapper.h\n WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}\n COMMENT "Building unified Go HA wrapper"\n DEPENDS etcd_wrapper.go k8s_lease_wrapper.go go.mod)
| @@ -0,0 +1,68 @@ | |||
| module github.com/kvcache-ai/Mooncake/mooncake-common/ha-wrapper | |||
There was a problem hiding this comment.
| go 1.25.0 | ||
|
|
||
| toolchain go1.25.10 |
There was a problem hiding this comment.
Go 1.25.0 is not a released version of Go (the current latest stable release is Go 1.24). Specifying go 1.25.0 and toolchain go1.25.10 in go.mod will cause compilation failures for developers using standard Go toolchains. It is recommended to downgrade this to a stable, widely available version such as go 1.24.0.
go 1.24.0\n\ntoolchain go1.24.0
|
A good contribution! However, you may first check CI for build problems. |
|
Thanks for working on this. The direction makes sense to me; having one Go HA wrapper for both etcd and k8s should make the wheel story much easier. One thing I noticed is that the Rust binding path still seems to look for the old wrapper name. In I also wonder if we should avoid running |
Description
Currently, the HA etcd and k8s backends are mutually exclusive when building, as they are two different Go modules that must be separated for a single go runtime.
This creates a strong UX problem: the pip package must have different versions for different HA backends. This makes it extremely difficult to use the k8s HA backend when integrating with SGLang/vLLM.
It is proposed to combine 2 Go modules into one HA-wrapper module, in this case, when building, work with different HA-backends will be available. Redis-backend uses C++, so there is no such problem.
A large part of the diff appeared due to pre-commit hooks on CMakeLists.txt
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
# Example: bash scripts/run_ci_test.shTest results:
Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks passAI Assistance Disclosure