You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Stacked on #1329 (3/5). Review only the top commit; the rest is that PR.
PRs 1–3 built the LXC streaming handle and made it safe to hold more than one at a time. Nothing reaches it yet: ContainmentBackend::Lxc still falls to the engine's catch-all, so spawn_sandbox, run, and every SDK built on mxc_spawn_request answer UnsupportedContainment. This PR routes it.
Two production changes:
dispatch.rs gains the spawn_lxc arm, modelled on spawn_bubblewrap, with the matching off-Linux rejection.
platform.rs reports lxc on Linux. It hard-coded an exclusion, so a host with LXC but no usable bwrap reported isSupported: false — discovery would have kept sending callers away from a backend that now works. The Linux branch now mirrors the TypeScript SDK's getPlatformSupport: order [lxc, bubblewrap], either one alone makes the host supported.
Everything else is the coverage that turning it on requires: a live streaming suite (mxc-sdk/tests/streaming_lxc.rs), a container-free FFI routing pin, .NET and Node cases, and the CI steps that run them. lxc-exec goes through mxc_engine::run and never reaches spawn_sandbox, so no shell script can cover this path — the tests have to be Rust.
Availability is not privilege: lxc in availableMethods means the tooling is present, not that the caller may use it. LXC needs root for a system container. That is documented on the field rather than gated in the probe, because resolve_default_lxcpath supports unprivileged LXC and a euid check would hide hosts where it works.
Validation
cargo fmt --all --check, clippy --all-targets -D warnings, and the unit tests pass on Windows and cross-compiled to x86_64-unknown-linux-gnu. Every #[cfg(target_os = "linux")] body is invisible to a Windows-only lane, so the cross-compile is the part that type-checks the new code.
The live LXC cases need Linux, LXC, and root, so they have not run locally. lxc-e2e.yml runs them under MXC_LXC_TESTS_REQUIRE_EXECUTION=1, which turns a skipped prerequisite into a failure rather than a green job that tested nothing. The kill case asserts the container is no longer running after kill() — a container cannot be stopped while it holds a live process, so that is what proves the kill reached the workload and the descendant it backgrounded, rather than stopping at the host lxc-attach.
This example is rejected before launch: its legacy allowOutbound: false shape is normalized with defaultPolicy and the default enforcementMode: 'capabilities', while LXC explicitly rejects capabilities enforcement. Use the directional 0.8 shape shown by the new live tests so the documented LXC sample actually runs.
Fix invalid LXC network example
src/core/mxc-sdk/README.md:165
The example immediately above cannot actually be passed to run or spawn_sandbox: network: None is emitted in the legacy form with a default network posture, which LXC interprets as capabilities enforcement and rejects. Please make the example use explicit directional deny egress/ingress (as streaming_lxc.rs does), or avoid claiming that this exact request runs.
kill() can throw when native termination fails, which skips dispose() and defeats this finally block's root-container cleanup. Guarantee disposal with a nested finally (or call dispose() directly, since it already kills active processes).
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The Node LXC test passes a forbidden policy version and fails before spawning, while several documentation claims contradict supported unprivileged and preserved-container behavior.
Document LXC availability as a probe, not launch usability
src/core/mxc_engine/src/platform.rs:48
This overstates the LXC probe: is_lxc_available() only checks that lxc-ls --version exits successfully, so it does not establish that the rest of the runtime tooling is installed or that a container can be launched. Document the field as probe availability rather than usability; otherwise callers may treat availableMethods as a launch preflight even though docs/backend-support-probe-api-plan.md:100 explicitly says it can report LXC on a host where a real run fails.
This issue also appears on line 127 of the same file.
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
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.
Stacked on #1329 (3/5). Review only the top commit; the rest is that PR.
PRs 1–3 built the LXC streaming handle and made it safe to hold more than one at a time. Nothing reaches it yet:
ContainmentBackend::Lxcstill falls to the engine's catch-all, sospawn_sandbox,run, and every SDK built onmxc_spawn_requestanswerUnsupportedContainment. This PR routes it.Two production changes:
dispatch.rsgains thespawn_lxcarm, modelled onspawn_bubblewrap, with the matching off-Linux rejection.platform.rsreportslxcon Linux. It hard-coded an exclusion, so a host with LXC but no usablebwrapreportedisSupported: false— discovery would have kept sending callers away from a backend that now works. The Linux branch now mirrors the TypeScript SDK'sgetPlatformSupport: order[lxc, bubblewrap], either one alone makes the host supported.Everything else is the coverage that turning it on requires: a live streaming suite (
mxc-sdk/tests/streaming_lxc.rs), a container-free FFI routing pin, .NET and Node cases, and the CI steps that run them.lxc-execgoes throughmxc_engine::runand never reachesspawn_sandbox, so no shell script can cover this path — the tests have to be Rust.Availability is not privilege:
lxcinavailableMethodsmeans the tooling is present, not that the caller may use it. LXC needs root for a system container. That is documented on the field rather than gated in the probe, becauseresolve_default_lxcpathsupports unprivileged LXC and a euid check would hide hosts where it works.Validation
cargo fmt --all --check,clippy --all-targets -D warnings, and the unit tests pass on Windows and cross-compiled tox86_64-unknown-linux-gnu. Every#[cfg(target_os = "linux")]body is invisible to a Windows-only lane, so the cross-compile is the part that type-checks the new code.The live LXC cases need Linux, LXC, and root, so they have not run locally.
lxc-e2e.ymlruns them underMXC_LXC_TESTS_REQUIRE_EXECUTION=1, which turns a skipped prerequisite into a failure rather than a green job that tested nothing. The kill case asserts the container is no longer running afterkill()— a container cannot be stopped while it holds a live process, so that is what proves the kill reached the workload and the descendant it backgrounded, rather than stopping at the hostlxc-attach.Refs #1291.
Microsoft Reviewers: Open in CodeFlow