Repository navigation
Conversation
The Rust SDK now does what the CLI already does for long-lived sandbox calls: it asks the gateway which replica owns the sandbox's supervisor session, sends that owner in the x-openshell-replica header on ExecSandbox, and retries once without the header when the edge proxy reports that the owner's route has no ready endpoints. The owner is cached per client for 30 seconds, keyed by workspace and sandbox name, so repeated execs do not add a GetSandbox round trip. A failed owner lookup sends the call unrouted and leaves any error to the exec itself. Part of NVIDIA#4223 Signed-off-by: divesh <dgude@nvidia.com>
FrostGod
changed the title
|
/ok to test 92907e2 |
EmilienM
left a comment
EmilienM
left a comment
There was a problem hiding this comment.
Needs a human read: 5 minor (inline)
Nothing blocking. The minors are about the CLI helpers being copied, the extra GetSandbox per exec, and how the owner cache handles failures. It's SDK code, so a maintainer should still read it.
How this was reviewed
- Commit
92907e2, round 1 - Standard review, because it changes
crates/openshell-sdk/src/client.rs,crates/openshell-sdk/src/routing.rs, andcrates/openshell-sdk/src/lib.rs - Passes: architecture, performance, security, scalability, rust, kubernetes, then correctness
- Independent skeptics checked each candidate finding (2); 6 of 11 held up
- Repo rules read from
main: AGENTS.md, CLAUDE.md, CONTRIBUTING.md, docs/CONTRIBUTING.mdx - Not covered: nothing skipped
| (workspace.to_string(), name.to_string()) | ||
| } | ||
|
|
||
| pub fn with_owner_hint<T>(message: T) -> tonic::Request<T> { |
There was a problem hiding this comment.
Minor: These four helpers are a line-for-line copy of crates/openshell-cli/src/tls.rs, so the exec retry rule now lives in two places (this copy already lost the CLI's connection termination test case). The CLI already depends on this crate; could they move into openshell_core::replica_routing next to the header constants, with the CLI importing them from there?
| workspace: &str, | ||
| request: proto::ExecSandboxRequest, | ||
| ) -> Result<tonic::Streaming<proto::ExecSandboxEvent>> { | ||
| let owner = self.owner_of(workspace, &request.sandbox).await; |
There was a problem hiding this comment.
Minor: A cache miss now costs every exec an extra GetSandbox, even on single-replica gateways (they never report an owner) and with replica routing off, which is the chart default. The CLI reads the owner off a GetSandbox it was already making. Could the curated get_sandbox/wait_ready fill this cache instead, or callers turn the lookup off? Also, "behavior is unchanged" in the PR body isn't quite true with the extra round trip.
| async move { grpc.get_sandbox(request).await } | ||
| }) | ||
| .await | ||
| .ok()?; |
There was a problem hiding this comment.
Minor: .ok()? drops every lookup error. With a rejected token the lookup forces one refresh and the exec forces another, and with an unreachable gateway exec waits out two 10s connect timeouts. I'd return Unauthenticated and Unavailable (exec hits them anyway) and only fall back on the rest, with a debug!.
| owners.forget(workspace, &request.sandbox); | ||
| grpc.exec_sandbox(request).await | ||
| } | ||
| response => response, |
There was a problem hiding this comment.
Minor: When a routed call fails in a way retry_unrouted doesn't match (Envoy's connection timeout when the owner's node goes dark, for example), the owner stays cached and execs keep going to it for 30s. The CLI would just ask again. Forgetting the entry on any routed Unavailable fixes that:
| response => response, | |
| Err(status) => { | |
| if owner.is_some() && status.code() == tonic::Code::Unavailable { | |
| owners.forget(workspace, &request.sandbox); | |
| } | |
| Err(status) | |
| } | |
| response => response, |
| let state = Arc::new(state); | ||
| let client = connect(&start_mock(state.clone()).await).await; | ||
| for _ in 0..count { | ||
| let _ = client |
There was a problem hiding this comment.
Minor: let _ = throws away each exec result, so these tests would still pass if exec returned the routed error after a successful unrouted retry, or swallowed supervisor not connected. Return the results and assert on them.
This branch has not been deployed
2 participants
Summary
The Rust SDK now sends
execstraight to the gateway replica that owns the sandbox's supervisor session, as the CLI already does. Behind an edge proxy with replica routing enabled, this skips the relay hop through a peer replica; without one, the header is ignored and behavior is unchanged.Related Issue
Part of #4223 (the "Every SDK asks for the owner and sends the replica hint, like the CLI" item, Rust portion). Go, TypeScript, and Python follow in separate PRs.
Changes
routingmodule: header helpers matching the CLI, plus a per-client owner cache (30s TTL, bounded, also caches "no owner").execonOpenShellClientandWorkspaceScopedClientasks for the owner, sendsx-openshell-replica, and retries once without it when Envoy reports the owner's route has no ready endpoints. A failed owner lookup falls back to an unrouted call.docs/kubernetes/ingress.mdxnote that the Rust SDK sets the header.Release Notes
Rust SDK
execcalls now take the direct route to the sandbox's owner replica when replica routing is enabled.Testing
cargo fmt,cargo clippy -p openshell-sdk --all-targets -D warnings,cargo test -p openshell-sdk,cargo check -p openshell-cli)Checklist