feat(spur-client): add client-side controller failover with endpoint rotation#384
Open
shiv-tyagi wants to merge 1 commit into
Open
feat(spur-client): add client-side controller failover with endpoint rotation#384shiv-tyagi wants to merge 1 commit into
shiv-tyagi wants to merge 1 commit into
Conversation
…rotation Add a service-agnostic spur-client crate that dials a tonic Channel from a comma-separated endpoint list, rotating to the next endpoint when one is unreachable. spurctld, spurd, and the CLI now build clients from this channel instead of connecting to a single hardcoded address, so a single reachable controller in an HA quorum is enough (server-side leader forwarding handles the rest). - spur-client: parse_endpoints + connect_channel, with unit tests for parsing and rotation (dead-first, all-down). - ControllerConfig::endpoints(): expand hosts + listen_addr port into an endpoint list; CLI exports it as a comma-joined SPUR_CONTROLLER_ADDR. - Refactor call sites in spur-cli, spurd, and spur-ffi to SlurmControllerClient::new(connect_channel(&addr).await?). - Docs + examples: document the comma-separated HA controller list. - Native-host e2e: test_controller_failover covers rotation past a dead endpoint, live-first lists, clean all-down failure, and submit-through-list.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
+ Coverage 68.22% 68.29% +0.07%
==========================================
Files 134 135 +1
Lines 36098 36235 +137
==========================================
+ Hits 24625 24744 +119
- Misses 11473 11491 +18 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces client-side controller failover for HA deployments by adding a new spur-client crate that dials a comma-separated list of controller endpoints and updates spurd, spur-cli, and spur-ffi call sites to use it. It also adds config/documentation plumbing and E2E coverage to validate endpoint rotation behavior.
Changes:
- Add
crates/spur-clientwith endpoint parsing + connection rotation and unit tests. - Switch controller connections in
spurd,spur-cli, andspur-ffifromSlurmControllerClient::connect(addr)toSlurmControllerClient::new(connect_channel(...)). - Export multi-endpoint controller addresses via config ->
SPUR_CONTROLLER_ADDR, document usage, and add native-host E2E failover tests.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/native_host/e2e/test_controller_failover.py | Adds E2E coverage for comma-separated controller endpoint rotation via CLI. |
| tests/native_host/e2e/cluster.py | Extends CLI helpers to override SPUR_CONTROLLER_ADDR per invocation. |
| examples/spur.conf | Documents controller.hosts HA configuration for client-side failover. |
| docs/deployment/native-host.rst | Documents comma-separated --controller / SPUR_CONTROLLER_ADDR usage for HA. |
| crates/spurd/src/reporter.rs | Uses spur_client::connect_channel for registration/deregistration/heartbeat connections. |
| crates/spurd/src/agent_server.rs | Uses spur_client::connect_channel when reporting job completion to controllers. |
| crates/spurd/Cargo.toml | Adds spur-client dependency to spurd. |
| crates/spur-ffi/src/lib.rs | Routes FFI controller calls through spur_client::connect_channel. |
| crates/spur-ffi/Cargo.toml | Adds spur-client dependency to spur-ffi. |
| crates/spur-core/src/config.rs | Adds ControllerConfig::endpoints() and unit tests for endpoint expansion. |
| crates/spur-client/src/lib.rs | New failover dialer: parse/normalize endpoints, rotate on failure, unit tests. |
| crates/spur-client/Cargo.toml | New crate manifest and dev-deps for transport tests. |
| crates/spur-cli/src/token.rs | Switches token subcommands to build client from connect_channel. |
| crates/spur-cli/src/sstat.rs | Switches sstat controller connection to connect_channel. |
| crates/spur-cli/src/srun.rs | Switches srun controller connections to connect_channel. |
| crates/spur-cli/src/squeue.rs | Switches squeue controller connection to connect_channel. |
| crates/spur-cli/src/sprio.rs | Switches sprio controller connection to connect_channel. |
| crates/spur-cli/src/smd.rs | Switches smd controller connection to connect_channel. |
| crates/spur-cli/src/sinfo.rs | Switches sinfo controller connection to connect_channel. |
| crates/spur-cli/src/sdiag.rs | Switches sdiag controller connection to connect_channel. |
| crates/spur-cli/src/scontrol.rs | Switches scontrol controller connections to connect_channel. |
| crates/spur-cli/src/scancel.rs | Switches scancel controller connection to connect_channel. |
| crates/spur-cli/src/sbatch.rs | Switches sbatch controller connection to connect_channel. |
| crates/spur-cli/src/sattach.rs | Switches sattach controller connection to connect_channel. |
| crates/spur-cli/src/salloc.rs | Switches salloc controller connection to connect_channel. |
| crates/spur-cli/src/node.rs | Switches node subcommands controller connections to connect_channel. |
| crates/spur-cli/src/main.rs | Loads config-derived multi-endpoint controller list into SPUR_CONTROLLER_ADDR. |
| crates/spur-cli/src/exec.rs | Switches exec controller connection to connect_channel. |
| crates/spur-cli/Cargo.toml | Adds spur-client dependency to spur-cli. |
| Cargo.toml | Adds crates/spur-client to the workspace members. |
| Cargo.lock | Records new crate/dependency resolution from adding spur-client + dev-deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+69
to
+71
| async fn try_connect(endpoint: &str) -> Result<Channel, tonic::transport::Error> { | ||
| Endpoint::from_shared(endpoint.to_string())?.connect().await | ||
| } |
Comment on lines
+328
to
+334
| pub fn endpoints(&self) -> Vec<String> { | ||
| let port = self.listen_addr.rsplit(':').next().unwrap_or("6817"); | ||
| self.hosts | ||
| .iter() | ||
| .map(|host| format!("http://{host}:{port}")) | ||
| .collect() | ||
| } |
Comment on lines
+58
to
+62
| Err(e) => warn!( | ||
| %endpoint, | ||
| error = %e, | ||
| "controller endpoint unreachable, trying next" | ||
| ), |
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
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
Closes #338.
spurdand the CLI could only be pointed at a single controller address, so an HA quorum offered no client-side resilience: if the one configured controller was down, connections failed even when other peers were healthy. This adds client-side failover across a list of controller endpoints.Approach
New
spur-clientcrate provides a service-agnostic dialer:connect_channel(endpoints)takes a comma-separated list, tries each in order, and rotates to the next on connection failure, returning atonic::transport::Channel. If every endpoint fails, the last error is returned.SlurmControllerClient::new(channel)), so the crate depends on neitherspur-protonorspur-core.A single reachable controller is sufficient: server-side Raft leader forwarding already routes writes to the current leader, so the client only needs to reach any live peer.
Config plumbing:
ControllerConfig::endpoints()expandshosts+ thelisten_addrport into an endpoint list.SPUR_CONTROLLER_ADDR; the same env var and--controllerflag accept a comma-separated list.Call sites in
spur-cli,spurd, andspur-ffiswitch fromSlurmControllerClient::connect(addr)to building a client fromconnect_channel(&addr).Design notes
http://scheme because tonic requires a full URI for the h2c transport; entries without a scheme are normalized tohttp://.tonic+tracing) to keep layering clean and avoid pulling proto/config into the transport helper.Testing
spur-clientunit tests: endpoint parsing (scheme normalization, whitespace, empty entries) and rotation (dead-first reaches a live server, all-down returns an error).spur-coreunit tests forControllerConfig::endpoints().test_controller_failover.py) run against the 4-node bare-metal cluster, all passing: rotation past a dead first endpoint, live-first list unaffected by a trailing dead endpoint, all-down list fails cleanly with a connection error (no hang), and a job submitted through a failover list completes.cargo clippy --workspace --exclude spur-ffi --all-targetsclean;cargo fmtapplied.Full multi-controller Raft failover in the automated native-host harness is deferred; the harness needs Raft support first.