feat(server): support out-of-tree compute drivers via --compute-driver-socket#1703
feat(server): support out-of-tree compute drivers via --compute-driver-socket#1703st-gr wants to merge 2 commits into
Conversation
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
|
/ok to test 8689967 |
8689967 to
464018b
Compare
|
Pushed a
|
NVIDIA/OpenShell's Cargo.toml has
[workspace.lints.clippy]
pedantic = "warn"
nursery = "warn"
all = "warn"
so `cargo clippy --workspace -- -D warnings` escalates pedantic lints
(including `doc_markdown`) to errors. Their CI uses this as part of
`mise run pre-commit`. The fork's existing `release-gateway.yml` only
runs `docker build` (which calls `cargo build --release` — no clippy),
so the same lints weren't enforced on the fork.
PR NVIDIA#1703 caught us with two `doc_markdown` violations
in `crates/openshell-core/src/config.rs:56` (`compute_driver.proto` and
`OPENSHELL_COMPUTE_DRIVER_SOCKET` missing backticks in the External
variant's doc comment). The fork's CI didn't catch it; NVIDIA's did.
This workflow runs on push to main + `feat/**` branches and on
workflow_dispatch. It pins to Rust 1.95.0 (matches upstream mise.toml)
and installs the same native deps `Dockerfile.gateway` needs
(libclang-dev + libz3-dev for transitive bindgen + z3-sys).
Workflow is light: ubuntu-latest runner, ~10-15 min cold, ~3-5 min
with the Swatinem/rust-cache hot. Path-filtered so doc-only changes
don't trigger it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: st-gr <38470677+st-gr@users.noreply.github.com>
464018b to
1c6663d
Compare
|
Force-pushed once more — caught a second pedantic clippy lint locally that the previous run didn't get to ( New HEAD: 1c6663d. Verified clean with the full-workspace check NVIDIA's CI runs: Could I get a re-vet with Apologies for the round-trip — I've added a |
elezar
left a comment
There was a problem hiding this comment.
Thinking ahead to changes such as #1589 and support for multiple drivers in future, does it make sense to formulate this as a "named driver endpoint" where a user would supply a driver name and a socket path? Especially given that the in-tree vm driver is already an example of such a driver and we would only be extending the existing functionality.
In practice, the gateway would select a driver by name (instead of a kind enum) and one could use GetCapabilities.driver_name to validate the user-provided name if required.
|
@elezar A. Minimal add. Keep B. Restructured Replacing I'd lean (A) because it doesn't pre-commit the data model to a shape #1589 might want to rewrite, and it costs ~30 LOC. Either is fine, though. Let me know which you'd prefer to see here. |
|
As a quick follow-up: What would it look like if we first migrate to implementing the Also note, that although #1589 is mentioned here as a motivator for restructuring how drivers are managed from a gateway perspective, moving to a name-keyed registry for managing drivers is not specifically related to the driver-specific config. |
|
Right, separating from #1589 makes sense, the named-driver registry is its own concern. Confirmed against the #1051 OpenShell Drivers roadmap: Its stated intent of "the core OpenShell team maintains a small deployment footprint and third parties may develop their own drivers" is exactly the direction your Here's what Phase A could look like: A pure refactor that pulls today's Maps cleanly to the existing wiring at
Then Phase B (this PR) becomes additive:
Ordering: I'd implement Phase A first as a precursor PR (pure refactor, no behavior change since Before I start coding, anything about the
Once we converge on the shape, Phase A is straightforward. |
|
Thanks, this is the right direction, but I'd separate the pieces slightly differently. For Phase A, I don't think For the VM refactor in Phase A, the shape I'd like to see is:
Conceptually: For Phase B, an external driver should enter at the same acquired-endpoint boundary: That keeps driver construction separate from driver consumption. The VM lifecycle guard can own process/socket cleanup; an external driver can use a no-op lifecycle guard. I'd also prefer to avoid a special Reserved names like In that model, bare For the initial parser, I'd keep the rules narrow: parse the current comma-delimited One follow-up to call out is the gateway TOML mapping. Today [openshell.gateway]
compute_drivers = ["kyma@/run/openshell/kyma.sock"]or a more structured form under the driver table: [openshell.gateway]
compute_drivers = ["kyma"]
[openshell.drivers.kyma]
endpoint = "/run/openshell/kyma.sock"I don't think that has to block Phase A, but it should be discussed and clarified before Phase B lands so CLI/env/TOML do not grow incompatible shapes. Name matching via |
|
Thanks @elezar — agree the named-driver-endpoint framing is the right shape, and lines up with #1589's name-keyed
One scope question before I push: should I generalise the dispatch beyond |
+1 to avoiding the External enum variant. I like the We will eventually want to support multiple drivers, and possibly different versions of the same driver. That said, can we add the |
+1 to your read. Let's leave the four in-tree for now. |
|
@elezar @drew — reading the convergence: This PR:
Deferred to follow-up(s): Branch will be rewritten as: (1) refactor extracting the boundary, (2) wire |
1c6663d to
be90905
Compare
|
@elezar @drew — pushed the rewrite. PR body has been refreshed end-to-end to match the new shape. Summary of what changed since your last comments:
Branch is force-pushed; commits are squash-friendly (4 small, named per conventional-commits). Happy to revisit any cut point if you'd prefer a different shape. |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
be90905 to
b1ed362
Compare
Upstream merge changed `ComputeRuntime::from_driver` to take `driver_kind: ComputeDriverKind` by value, but the fork's external-driver constructor has no kind to supply, and `External(PathBuf)` makes the enum non-Copy — multiple `move out of self.driver_kind` errors result. Adopt the shape PR NVIDIA#1703's branch already shipped: - `from_driver` accepts `Option<ComputeDriverKind>` - four in-tree constructors pass `Some(kind)`, external passes `None` - clone `self.driver_kind` and the local `driver` at the three sites that previously moved (struct field is now non-Copy) Signed-off-by: st-gr <38470677+st-gr@users.noreply.github.com>
b1ed362 to
f0d5b27
Compare
|
@drew @elezar — pinging on this one. Pushed the rewrite Jun 5 implementing the construction/consumption split @elezar described and dropping the Just rebased onto Verified clean: New head: |
|
/ok to test f0d5b27 |
f0d5b27 to
0925729
Compare
|
Rebased onto current Conflicts resolved:
No conflicts in Verified inside
4 commits on top of upstream/main, new HEAD |
| tracing_log_bus: TracingLogBus, | ||
| supervisor_sessions: Arc<supervisor_session::SupervisorSessionRegistry>, | ||
| ) -> Result<ComputeRuntime> { | ||
| if let Some(socket_path) = config.external_compute_driver_socket.as_deref() { |
There was a problem hiding this comment.
Given that we discussed that the current in-tree VM driver is an example of a "remote socket driver", I don't think that we should introduce a new conditional branch for this special case.
I am prepping a PR from a fork that would propose additional changes. See st-gr#1
| | Podman | Rootless or single-machine deployments. | Container plus nested sandbox namespace. | Uses the Podman REST API, OCI image volumes, and CDI GPU devices when available. | | ||
| | Kubernetes | Cluster deployment through Helm. | Pod plus nested sandbox namespace. | Uses Kubernetes API objects, service accounts, secrets, PVC-backed workspace storage, and GPU resources. | | ||
| | VM | Experimental microVM isolation. | Per-sandbox libkrun VM. | Gateway spawns `openshell-driver-vm` as a subprocess over a private, state-local Unix socket. The VM driver boots a cached bootstrap `rootfs.ext4`, prepares requested OCI images inside a bootstrap VM with `umoci`, attaches the prepared image disk read-only, and gives each sandbox a writable `overlay.ext4` for merged-root changes and runtime material. The driver persists each accepted launch request beside the overlay and restarts those VMs on driver startup without recreating the overlay. | | ||
| | External | Out-of-tree drivers operated alongside the gateway. | Whatever boundary the driver implements. | Activated by `--compute-driver-socket=<path>` (env `OPENSHELL_COMPUTE_DRIVER_SOCKET`). The gateway connects to a UDS the operator already provisioned, runs `GetCapabilities`, logs the advertised `driver_name`, and dispatches all sandbox lifecycle calls through the same `compute_driver.proto` surface as the in-tree drivers. The driver process and socket lifecycle are operator-owned; the gateway does not spawn, supervise, or remove the driver. The trust boundary is the socket's filesystem permissions — the operator must ensure only the gateway uid can read/write it. | |
There was a problem hiding this comment.
In working through a local review, I think using "Remote" instead of "External" is clearer going forward. This also makes it less confusing if we implement the VM driver as a specific in-tree instance of a remote driver.
|
@elezar — adopted your refactor PR (st-gr/OpenShell#1) on top of the rebased PR #1703 branch. Branch is now This addresses both inline comments:
Verified inside
The branch now sits at 5 commits ahead of |
Add named remote compute driver endpoint support to the gateway. Remote drivers are selected by a non-reserved compute driver name and either a CLI/env socket endpoint or [openshell.drivers.<name>].socket_path. The VM driver now enters ComputeRuntime through the same acquired remote endpoint path, while Docker, Podman, and Kubernetes retain their in-process drivers. Require --drivers/OPENSHELL_DRIVERS when pairing an ad-hoc socket endpoint so the socket does not imply a magic driver name, and keep reserved in-tree names unavailable for unmanaged socket endpoints. Co-authored-by: Evan Lezar <elezar@nvidia.com> Signed-off-by: st-gr <38470677+st-gr@users.noreply.github.com> Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
2979bf9 to
02d9b31
Compare
|
Hi @st-gr. I spent some time on this again today. I rebased the changes onto the current There is also #1949 which should further cleanup driver aquisition / configuration, but once again is not a blocker. If you could verify the current changes against your remote driver, we could look at getting this integrated. We would also need to update the PR description. Did you want to do that, or should I? |
|
/ok-to-test 02d9b31 |
|
@elezar — verified against my Kyma compute driver. Both the cold-start dispatch and live sandbox traffic work cleanly with Build (
Deploy (Helm upgrade on Gardener-managed Kyma cluster, 4 amd64 workers; the existing Required one chart-side change in Gateway log on first boot: That's the exact shape your refactor advertises — configured/advertised name comparison via Follow-ups in this fork's chart (
I'm happy for you to do the PR description update — your wording on the construction/consumption split and the named-endpoint framing is the canonical version, and you've been driving the refactor. From my side, this is ready to merge once you've updated the description. |


Summary
Adds an opt-in
--compute-driver-socket=<path>flag (envOPENSHELL_COMPUTE_DRIVER_SOCKET) to the gateway. When set, the gateway dispatches sandbox lifecycle to an out-of-tree compute driver speaking the existingcompute_driver.protocontract over a Unix domain socket, instead of one of the four built-in drivers (Kubernetes / Podman / Docker / VM).The implementation deliberately avoids adding a fifth
ComputeDriverKindvariant — the gateway treats out-of-tree drivers as an acquired endpoint rather than a parallel kind in the in-tree enum. This was rewritten in response to review feedback from @elezar and @drew (see "Review history" below).Related Issue
Supersedes the closed draft #1604 (same author, same patch shape — re-opened as a non-draft after rebasing onto current
main, with all commits DCO-signed and st-gr-attributed). Same direction ascheese-head's vouch in #1345 ("extend or provide new out-of-tree compute drivers to OpenShell").No upstream tracking issue filed — happy to file one if reviewers prefer.
Changes
crates/openshell-core/src/config.rs: addsConfig.external_compute_driver_socket: Option<PathBuf>pluswith_external_compute_driver_socket(...). No newComputeDriverKindvariant. The four in-tree variants remainCopyand bare.crates/openshell-server/src/cli.rs: adds--compute-driver-socket=<path>flag (envOPENSHELL_COMPUTE_DRIVER_SOCKET) onRunArgs. When set,effective_single_driverreturnsNone, short-circuiting both--driversand the auto-detect probe so callers keyed off the in-tree enum skip the match.crates/openshell-server/src/compute/mod.rs:ComputeRuntime::from_drivernow takesOption<ComputeDriverKind>; the four in-tree constructors wrap their kind inSome(...). Out-of-tree drivers passNone.connect_external_compute_driver(&Path)produces a tonicChannelover a pre-existing UDS, mirroring the connector pattern the VM driver already uses (hyper-util::TokioIo+tokio::net::UnixStream).ComputeRuntime::new_remote_externalconsumes the channel via the existingRemoteComputeDriverproxy and skips shutdown cleanup / managed-process supervision (the operator owns the lifecycle).from_driverlogs thedriver_nameadvertised byGetCapabilitieswheneverdriver_kindisNone, so operators can confirm the gateway connected to the driver they expect (logged for diagnostics — not validated against operator-supplied input in this PR).crates/openshell-server/src/lib.rs:build_compute_runtimeshort-circuits toconnect_external_compute_driver+new_remote_externalwhenConfig.external_compute_driver_socketis set, before consulting--drivers/ auto-detect. The four in-tree backends keep their existing dispatch arms unchanged.architecture/compute-runtimes.md: adds an "External" row to the Runtime Summary table (activation flag,GetCapabilities.driver_namelogging, operator-owned process+socket lifecycle, trust boundary = filesystem permissions on the UDS) and a matching row in Supervisor Delivery.No
compute_driver.protochanges. No new workspace dependencies (hyper-utilandtowerare already in scope;tokioalready exposesUnixStreamundercfg(unix)). No security-model changes.Testing
mise run pre-commitpasses — not run end-to-end (misenot on author's dev environment), but the equivalent rust pieces verified independently in arust:1.95.0-slim-bookwormdev container with the workspace's apt deps:cargo fmt --all -- --checkclean;cargo clippy --no-deps --workspace --all-targets -- -D warningsclean;cargo test --workspace --lib --binspasses (incl. all 769openshell-serverlib tests).crates/openshell-server/src/cli.rs::tests:compute_driver_socket_flag_populates_run_args,compute_driver_socket_overrides_drivers_flag,compute_driver_socket_reads_from_env_var. The four existing in-tree compute-driver tests (configured_compute_driver_*) continue to pass unchanged.Externaldriver in CI would require shipping a stub driver binary; deferring until at least one in-tree consumer wants to test against it. The patch is exercised in production at SAP via thest-gr/openshell-driver-kymareference driver running against a Gardener-managed Kyma cluster since 2026-05-28 (sandbox CR creation,openshell sandbox exec,openshell sandbox upload/downloadall work).Checklist
feat(core):,feat(server):,docs(compute):)architecture/compute-runtimes.md::Runtime Summaryand::Supervisor Delivery. Per-runtime implementation-notes section is intentionally NOT extended for external drivers because they ship their own documentation; the reference implementation lives atst-gr/openshell-driver-kyma.Review history
The original revision of this PR added
ComputeDriverKind::External(PathBuf)as a fifth enum variant. @elezar and @drew both flagged this as the wrong shape — out-of-tree drivers are an acquired endpoint rather than a parallel kind. The current revision implements that direction:Externalenum variant.ComputeDriverKindis unchanged; out-of-tree dispatch lives onConfig.external_compute_driver_socketand goes through a separate construction path that produces aComputeRuntimewithdriver_kind = None.(name, socket)tuple syntax is deferred. The flag remains a single path.GetCapabilities.driver_nameis logged for diagnostics, not validated against operator input. This matches @drew's suggestion to defer name-keying /<name>@<socket>to a follow-up that aligns with the broader name-keyed driver registry direction (rfc-0006: add driver config passthrough proposal #1589).RemoteComputeDriverfor the actual proto consumption — the only divergence is at construction.Happy to revisit any of those choices if reviewers prefer a different cut point.
Notes for reviewers
Out of scope (intentional):
--compute-driver-socketat its socket path.compute_driver.protochanges. The existing protocol is reused verbatim — the same one already used by the in-tree VM driver to talk to its tonic peer.--drivers vmposture.Operator context:
The forked gateway image at
ghcr.io/st-gr/openshell-gateway(carrying these patches plus an in-treeDockerfile.gateway) has been driving production-shaped Kubernetes clusters with thest-gr/openshell-driver-kymacompute driver since 2026-05-28. The flag has needed zero changes since it landed — the API shape is stable. Merging this PR lets the driver consume the unmodified upstream gateway image, retiring the fork.