Repository navigation
[Active-passive] Phase1: Introduces a native, lightweight C++ leader election client - #63773
Conversation
![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 introduces a distributed leader election library for GCS using Kubernetes Leases, including a lease client, background elector, watchdog thread, and associated unit tests. While the implementation is comprehensive, several critical issues must be addressed: a race condition in Stop() that can cause a stopped node to re-acquire the lease, a compilation error when calling IsNotFound() on StatusOr, potential crashes in the K8s API utility functions if uninitialized, full-replacement PUT requests in Renew() that wipe out existing Lease metadata, a race condition in the watchdog's elapsed time calculation, and a discrepancy in process termination logging when the renewal deadline is breached.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
9475cec to
5e44ee8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a Kubernetes-based distributed leader election library for the GCS, including a lease client, elector, and associated tests, while also refactoring the Kubernetes API utility functions to support GET and PUT requests with proper status returns. The review feedback highlights several critical improvements: consistently using absl::Now() to prevent clock skew issues during lease renewals, making the Stop() method thread-safe and idempotent, explicitly setting timeouts before each HTTP write and read operation to avoid indefinite blocking, and adding type checks during JSON parsing to prevent potential crashes.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a platform-agnostic distributed leader election library for Ray's GCS using Kubernetes Leases, including the K8sLeaseClient for API interactions, a background LeaderElector loop with an asynchronous watchdog, and corresponding unit tests. Feedback highlights critical concurrency and lifecycle issues in LeaderElector: multiple calls to Run() can trigger std::terminate() by overwriting joinable threads, the watchdog thread's condition variable predicate prevents early wakeups on successful renewals, and detaching threads during self-stopping can lead to use-after-free vulnerabilities.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a platform-agnostic distributed leader election library for GCS using Kubernetes Leases, including the lease client, elector, watchdog, and associated tests, alongside updates to the Kubernetes RPC utilities to support GET and PUT operations with proper status propagation. The review feedback highlights several robustness and reliability improvements: adding defensive checks to prevent JSON parsing exceptions in ParseLeaseMetadata, enforcing that the retry period is smaller than the renew deadline via a RAY_CHECK, using non-throwing error_code overloads for Boost.Asio/Beast operations in EstablishSslConnection, and logging failures during graceful lease release.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a generic distributed leader election library for Ray GCS using Kubernetes Leases, including a platform-agnostic client interface, a concrete K8s lease client, a background leader elector with a safety watchdog loop, and corresponding unit tests. Additionally, the Kubernetes utility functions are updated to support GET and PUT requests with SSL connection establishment and timeout handling. The review feedback highlights critical improvements: removing the static keyword from service account token reads to handle Kubernetes token rotation gracefully, declaring EstablishSslConnection with internal linkage to avoid polluting the global namespace, and notifying the election thread immediately when the watchdog triggers a step-down to prevent delayed renewal attempts.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a platform-agnostic distributed leader election library for GCS using Kubernetes Leases. It includes a K8sLeaseClient for interacting with the Kubernetes API server, a LeaderElector to manage background lease acquisition and renewal loops, and an asynchronous watchdog to ensure timely step-downs. Additionally, the Kubernetes RPC utility is extended to support GET and PUT requests with SSL connection reuse and timeout handling. The review feedback highlights three key areas for improvement: adding validation to ensure configuration durations are strictly positive to prevent busy-wait loops, avoiding thread detaching in Stop() to prevent potential use-after-free bugs, and optimizing the watchdog's standby loop by replacing the periodic 100ms sleep with a conditional wait.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a generic distributed leader election library for Ray's GCS using Kubernetes Leases, adding K8sLeaseClient, LeaderElector, and extending k8s_util to support GET and PUT requests. The review feedback identifies several critical issues: a potential split-brain vulnerability if renewTime parsing fails and defaults to Unix Epoch; concurrent invocation risks for the on_stopped_leading callback; use-after-free hazards from detaching threads in Stop(); potential crashes when calling erase on invalid JSON records; clock-jump vulnerabilities from using wall time instead of a steady clock; and false-positive step-downs if the renewal timestamp is uninitialized.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a generic distributed leader election library for Ray, featuring a Kubernetes Lease-based client (K8sLeaseClient) and a background leader elector (LeaderElector) with watchdog capabilities. It also updates the Kubernetes API utility functions to return Status objects and support timeouts. The review feedback highlights several critical improvements: setting renew_time to absl::UnixEpoch() on parse failure to prevent deadlocks, adding a timeout before the SSL handshake to avoid indefinite blocking, preventing self-join deadlocks in Stop() when called from callbacks, and optimizing Release() to use cached lease records instead of performing a synchronous GET request.
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.
YoyinZyc
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit 2c42e43. Configure here.
|
@rueian I addressed the above comments and please let me know if you have further concerns. Thanks:) |
…ubernetes client Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
…tchdog Signed-off-by: Yuchen Zhou <yczhou@google.com>
…rnal field when update Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
Signed-off-by: Yuchen Zhou <yczhou@google.com>
…d simplify standby wait - Protect modifications and condition checks of atomic state variables (is_leading_, is_stopped_) using appropriate mutexes (watchdog_mutex_, election_mutex_) to prevent lost wake-ups on condition variables. Signed-off-by: Yuchen Zhou <yczhou@google.com>
…l observed time tracking - Replace local_observed_time_ (absl::Time, system clock) with local_observed_time_steady_ (std::chrono::steady_clock) to prevent local observed time elapsed comparisons from being affected by local system clock jumps or NTP adjustments. - Update comments in leader_elector.h. Signed-off-by: Yuchen Zhou <yczhou@google.com>
… threads and CAS callback gate - Remove thread self-join check and detach logic from Stop(), enforcing the contract that Stop() must be called from external threads. - Implement was_leading = is_leading_.exchange(false) gate under locks in both Renew() and WatchdogLoop() to guarantee on_stopped_leading is executed at most once. Signed-off-by: Yuchen Zhou <yczhou@google.com>
…ne timeouts - Modify EstablishSslConnection to accept a steady_clock::time_point deadline. - Refactor K8sApiPost, K8sApiGet, and K8sApiPut to calculate a single global 5-second deadline and apply it across all connection and read/write operations using expires_at(deadline). This prevents sequential timeouts from accumulating and causing unexpected long hangs. Signed-off-by: Yuchen Zhou <yczhou@google.com>
… of mutex Signed-off-by: Yuchen Zhou <yczhou@google.com>
- Default renew_time to InfiniteFuture() so a missing/unparseable renewTime no longer instantly preempts a possibly-live leader. - Refuse lease takeover from another holder without a resourceVersion (optimistic-concurrency split-brain guard). - Init K8s client config via call_once in the factory so it no longer depends on the auth-token path running first. - Add tests; update two expired-lease tests to include a resourceVersion. Signed-off-by: Yuchen Zhou <yczhou@google.com>
|
Thank you @rueian. I just rebased the change but it seems like I cannot merge the code, can you help me merge it if everything looks ok? |
andrewsykim
left a comment
andrewsykim
left a comment
There was a problem hiding this comment.
Overall LGTM, @YoyinZyc were you able to test the library manually using the changes in your other PR?
|
@andrewsykim yes the change can only be tested locally with the local kuberay change. |
…election client (ray-project#63773) ## Description This PR implements the phase 1 of Active-passive Ray head replicas. It introduces a native, lightweight C++ Kubernetes Lease client and a robust background leader elector state machine with an integrated watchdog. ### Kubernetes API Integration Extended the internal Kubernetes client (k8s_util) to support SSL-verified GET and PUT REST requests ### Kubernetes Lease Client - LeaderLeaseClientInterface: Defined a platform-agnostic interface for lease acquisition, renewal, and release. - K8sLeaseClient: Implemented a concrete client that interacts with the Kubernetes /apis/coordination.k8s.io/v1/namespaces/{namespace}/leases API. - Best practice adopted from client-go lib: - Clock-skew resilience - Cache lease object and direct-put the renewed lease object - Implement both graceful exit and fail-fast step-down ### Generic Leader Elector State Machine with Watchdog - Election Thread: Implements a continuous loop with two modes: - Standby Mode: Polls the lease client periodically to acquire the lease. Sleeps on a condition variable to avoid high CPU spin cycles. - Active Mode: Periodically renews the lease heartbeat. Steps down immediately if the lease has been pre-empted by another node. - Safety Watchdog Thread: Continuously monitors the elapsed time since the last successful lease renewal using monotonic clock time. If renewals fail persistently and exceed the safety renew_deadline_seconds, the watchdog forces an immediate step-down (triggering the on_stopped_leading suicide callback) before another candidate can steal the lease. - Responsive Thread Wakeups: Enabled immediate thread wakeup signals so that when the watchdog triggers a step-down, the election thread immediately stops any ongoing renewal attempts instead of waiting for retry timeouts. ### Unit tests - k8s_lease_client_test.cc: Tests lease acquisition, heartbeats, conflicts, and error handling. - leader_elector_test.cc: Tests promotion state transitions, transient network failures, persistent renewal timeouts, watchdog expiration steps, and multi-replica election with graceful/non-graceful exits. ### Risk Low, as it is just a unused library. ## Related issues ray-project#63643 ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Yuchen Zhou <yczhou@google.com> Signed-off-by: 400Ping <jiekaichang@apache.org>
Description
This PR implements the phase 1 of Active-passive Ray head replicas.
It introduces a native, lightweight C++ Kubernetes Lease client and a robust background leader elector state machine with an integrated watchdog.
Kubernetes API Integration
Extended the internal Kubernetes client (k8s_util) to support SSL-verified GET and PUT REST requests
Kubernetes Lease Client
Generic Leader Elector State Machine with Watchdog
Unit tests
Risk
Low, as it is just a unused library.
Related issues
#63643
Additional information