feat(server): conditional compilation of compute drivers#2163
feat(server): conditional compilation of compute drivers#2163dimityrmirchev wants to merge 4 commits into
Conversation
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
Introduce driver-kubernetes, driver-docker, driver-podman, and driver-vm cargo features on openshell-server, all enabled by default. Building with --no-default-features and selecting a subset produces a gateway that only carries the requested drivers. The three container-driver crates flip to optional dependencies. The VM driver runs out-of-process already, so driver-vm gates the in-server launcher and match arm rather than a Cargo dep. Feature gates cover: - driver crate imports and re-exports in compute/mod.rs - ComputeRuntime::new_docker / new_kubernetes / new_podman - Docker's ShutdownCleanup / StartupResume trait impls - the compute::vm module and VmComputeConfig re-export - driver-specific *_config_from_context and apply_*_runtime_defaults - the K8s ServiceAccount authenticator bootstrap in ServerState - the auth::k8s_sa module and state field - authenticator-chain construction in multiplex.rs - SupervisorReadiness impl for SupervisorSessionRegistry - driver-specific unit tests in cli.rs, lib.rs, driver_config.rs A zero-driver build is a supported extension-only gateway shape: it carries no in-tree driver code and speaks compute_driver.proto over a Unix socket to an out-of-tree driver named in compute_drivers with a matching [openshell.drivers.<name>].socket_path. The runtime gate in resolve_configured_compute_driver (see the follow-up commit) still rejects a built-in that was compiled out with the "rebuild with --features driver-X" message, so silent misconfiguration is impossible. Default builds are byte-equivalent to before. Each of the four single-driver builds compiles cleanly under -D warnings, as does the docker + podman combo and the zero-driver extension-only build. Signed-off-by: Dimitar Mirchev <28221091+dimityrmirchev@users.noreply.github.com>
Move the availability check out of the build_compute_runtime match arm into resolve_configured_compute_driver so it fires alongside the other config-time driver validation. Configuring a driver that this gateway was not built with now fails at startup with a message naming both the driver and the Cargo flag that re-enables it. Auto-detection (empty --drivers) filters through filtered_detect_driver: if openshell-core's environment probe picks a driver that was compiled out, we return None and let the caller produce the "no suitable driver" error rather than silently starting a different backend. In an extension-only build (zero built-ins compiled in) the message adapts to point operators at the [openshell.drivers.<name>].socket_path path instead of listing built-in names that don't exist in this binary; otherwise it lists only the built-ins actually linked and mentions that extension drivers are also valid. Adds unit tests that exercise the compiled-out rejection path and the extension-only empty-drivers message. The latter is #[cfg]-gated so it only compiles when no driver features are on. Signed-off-by: Dimitar Mirchev <28221091+dimityrmirchev@users.noreply.github.com>
Adds verify-drivers-compiled-out.sh, a mise task that exercises the per-driver Cargo features, and a matching step in the branch-checks workflow. The script mirrors verify-telemetry-compiled-out.sh: a table of stable driver-exclusive marker strings pulled from each driver crate, with present/absent/only modes. The 'only' mode asserts presence for a selected driver list AND absence for every other driver in one sweep, so a single script invocation covers both directions per build. The mise task exercises: default (all four drivers present), each single-driver build, one mixed feature set (docker+podman), and a zero-driver extension-only build that must link cleanly and must contain none of the four driver markers. After the zero-driver build the task also runs `cargo test -p openshell-server --lib --no-default-features` so the #[cfg]-gated extension-only unit tests actually execute in CI rather than being silently skipped by the default-features test job. The telemetry-off task uses a mixed --features driver-...,driver-... gateway build so the telemetry-marker absence check still exercises a representative in-tree-driver build path. Signed-off-by: Dimitar Mirchev <28221091+dimityrmirchev@users.noreply.github.com>
- README.md gains a "Choosing Which Compute Drivers Are Compiled In" section covering the four driver features, the extension-only build shape, and the startup-time compiled-out rejection. - architecture/compute-runtimes.md gains a "Compile-Time Driver Selection" section covering the four driver features, the extension-only zero-driver build, the startup-time missing-driver error, filtered auto-detection, and the always-available extension driver path. - docs/reference/gateway-config.mdx gains a matching reference section under "Choosing Which Drivers Are Compiled In" with a per-feature table, single-driver / mixed / extension-only build examples, and the startup-time rejection contract. Signed-off-by: Dimitar Mirchev <28221091+dimityrmirchev@users.noreply.github.com>
dde468f to
9a1e04c
Compare
| /// e.g. `--no-default-features` exercises all four rejection messages; | ||
| /// the default build has every driver and the loop body is skipped. | ||
| #[test] | ||
| fn configured_compute_driver_rejects_compiled_out_builtin() { |
There was a problem hiding this comment.
Question: What about building a map and checking missing vs present regardless of the feature set?
| #[cfg(feature = "driver-kubernetes")] | ||
| if let Some(k8s) = state.k8s_sa_authenticator.clone() { | ||
| authenticators.push(k8s); | ||
| } |
There was a problem hiding this comment.
When I see something like this, I wonder why we need to toggle these features at such low-levels. If state.k8s_sa_authenticator is never set when the driver-kubernetes feature is not enabled, then if should just be skipped here. I would assume that this field itself doesn't add any drastic external dependnecies. (is a possible issue the fact that the k8s_sa_authenticator stores the real type instead of an auth::authenticator::Authenticator?
| #[cfg(feature = "driver-docker")] | ||
| impl openshell_driver_docker::SupervisorReadiness for SupervisorSessionRegistry { | ||
| fn is_supervisor_connected(&self, sandbox_id: &str) -> bool { | ||
| Self::is_connected(self, sandbox_id) | ||
| } | ||
| } |
| # gateway needs at least one compute driver to link, so build with a | ||
| # single-driver feature set that excludes telemetry. | ||
| "cargo build -p openshell-server --bin openshell-gateway --no-default-features --features driver-kubernetes,driver-docker,driver-podman,driver-vm", | ||
| "tasks/scripts/verify-telemetry-compiled-out.sh absent target/debug/openshell-gateway", |
There was a problem hiding this comment.
Question: How similar is verify-telemetry-compiled-out.sh to verify-drivers-compiled-out.sh? Does it make sense to use the same mechanism there to test for being compiled out?
| "tasks/scripts/verify-telemetry-compiled-out.sh absent target/debug/openshell-sandbox", | ||
| ] | ||
|
|
||
| ["rust:verify:drivers-off"] |
There was a problem hiding this comment.
Does it make sense to generate a different named output for each of the features that w'ere testing here so that these verification tests are more self-contained?
| // Unreachable at runtime. This fallback is required for match exhaustiveness | ||
| // in non-default builds (including the extension-only build with zero driver features), | ||
| // where some or all of the arms above are cfg'd out. | ||
| #[cfg(not(all( | ||
| feature = "driver-kubernetes", | ||
| feature = "driver-docker", | ||
| feature = "driver-podman", | ||
| feature = "driver-vm", | ||
| )))] | ||
| ConfiguredComputeDriver::Builtin(kind) => { | ||
| unreachable!( | ||
| "compute driver '{}' passed startup validation but is not compiled into this gateway", | ||
| kind.as_str() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Would matching on _ => make sense -- at the expense of a possibly more generic error message? My conern is that each of these feature checks needs to be updated as dirvers are added or removed.
| )] | ||
| pub gateway_port: u16, | ||
| #[cfg_attr(not(feature = "driver-vm"), allow(dead_code))] | ||
| pub gateway_tls_enabled: bool, |
There was a problem hiding this comment.
Prefer #[cfg] directly on the fields over #[cfg_attr(..., allow(dead_code))]. The cfg_attr form uses a double-negated predicate (not(any(...))) that is harder to read and fragile — if a new driver reads one of these fields, the suppression predicate needs updating in sync or the warning silently returns.
With #[cfg] the struct accurately reflects what exists per build:
#[derive(Clone, Copy)]
pub struct DriverStartupContext<'a> {
pub file: Option<&'a config_file::ConfigFile>,
#[cfg(any(feature = "driver-docker", feature = "driver-podman", feature = "driver-vm"))]
pub guest_tls: Option<&'a GuestTlsPaths>,
#[cfg(any(feature = "driver-podman", feature = "driver-vm"))]
pub gateway_port: u16,
#[cfg(feature = "driver-vm")]
pub gateway_tls_enabled: bool,
pub endpoint_overrides: &'a BTreeMap<String, PathBuf>,
}The three construction sites (run_server, test_context_with_endpoint_overrides, test_driver_startup) each need matching #[cfg] on the three field assignments — the same pattern used throughout the rest of this PR. Non-blocking; the current approach compiles and is tested.
|
Bug:
Concretely: a This regression is introduced by this PR — before per-driver features, The fix is to have |
|
A couple of additional suggestions around
|
Summary
Adds per-driver Cargo features (
driver-kubernetes,driver-docker,driver-podman,driver-vm) onopenshell-server, all default-on. Building--no-default-featuresand selecting a subset produces a gateway that carries only those drivers, trimming binary size and supply-chain surface. Mirrors the existingtelemetryfeature pattern; the default build is behaviorally unchanged.Warning
A zero-driver build (
cargo build -p openshell-server --no-default-features) is a supported extension-only gateway shape: no in-tree driver code is linked, andcompute_driversmust name an out-of-tree driver reached via[openshell.drivers.<name>].socket_path. Configuring a built-in that this binary was not built with still fails at startup with a message naming the Cargo flag that re-enables it.This is a breaking change as setting
--no-default-featuresin previous versions produced a geteway that had only telemetry disabled, but still all 4 default drivers compiled in.Related Issue
Fixes #1943
Related to #1999 and #2061
Changes
driver-kubernetes,driver-docker,driver-podman, anddriver-vmfeatures onopenshell-server, all in the new default set. The three container-driver crates flip tooptional = true. The VM driver has no in-server crate dependency, sodriver-vmgates the in-server launcher plumbing rather than an optional dep.#[used] static COMPILE_MARKER: &str = "OPENSHELL_DRIVER_MARKER:<name>". This is an explicit, documented contract — the compile-out guard grepping for driver-specific text no longer depends on implicit metadata.tasks/scripts/verify-drivers-compiled-out.shgreps built binaries for the driver markers. A newrust:verify:drivers-offmise task walks the matrix: default (all four drivers present), each single-driver build, one mixed set (docker+podman), and a zero-driver extension-only build that must link cleanly and contain none of the driver markers. The zero-driver build is followed bycargo test -p openshell-server --lib --no-default-featuresso the#[cfg]-gated extension-only tests actually execute in CI.README.mdgains a "Choosing Which Compute Drivers Are Compiled In" section and updates the telemetry-off build example. Note: I am not sure if this is the best place for this piece of documentation and am open to suggestions.architecture/compute-runtimes.mdgains a "Compile-Time Driver Selection" section,docs/reference/gateway-config.mdxgains a matching reference section under Driver References with a per-feature table and worked examples including the extension-only build.Note
This PR adds roughly 28
#[cfg(feature = "driver-*")]gates acrossopenshell-server— I am not a fan of how much clutter that is, but on today's crate layout it is the cost of compile-time driver selection. Extracting driver-specific compute wiring into a dedicated crate (tracked in #1999) would delete about two-thirds of these gates: driver imports, theComputeRuntime::new_*constructors, theBuiltinmatch arms and theunreachable!exhaustiveness arm, the*_config_from_contextandapply_*_runtime_defaultshelpers,compute::vm, and theSupervisorReadinessimpl would all move behind a driver-agnostic entry point in the new crate. The residual gates would cluster aroundauth::k8s_sa— a gateway-level authenticator that happens to talk to a K8s API server — whose home is a separate design question.Testing
mise run pre-commitpassesChecklist