Repository navigation
fix(gateway): keep VM driver alive during Ctrl+C shutdown - #5126
vineethsai7 wants to merge 2 commits into
Conversation
A terminal sends SIGINT to its foreground process group. The VM driver and its launchers shared that group, so they exited before the gateway's shutdown sweep could stop running sandboxes. Give the driver its own group and relay its stdout and stderr through the foreground gateway. The pipes keep driver logs visible without suspending the background group in TOSTOP terminals. Add unit coverage for process-group separation and piped output. Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffc8f7dd11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I have read the DCO document and I hereby sign the DCO. |
Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Summary
Ctrl+C sends SIGINT to a foreground gateway's entire process group. The VM compute driver and its VM launchers inherited that group, so they could exit before the gateway's shutdown sweep stopped its sandboxes. Start the driver in its own process group and relay its output through the gateway so the driver remains available during the sweep, including in terminals with
stty tostop.Related Issue
Part of #4770 (the driver now survives Ctrl+C;
--no-keepdeletion on shutdown is left for a follow-up)Supersedes #5068 (auto-closed before the vouch)
Changes
Review findings from #5068:
Signed-off-bytrailer.The shutdown sweep now has a live driver to stop sandboxes. This change does not make
--no-keepsandboxes delete during gateway shutdown or change the attached CLI's cleanup error if the gateway exits before the CLI can delete. Those behaviors remain for a separate change.Release Notes
Stopping a foreground VM gateway with Ctrl+C now leaves the compute driver available for sandbox shutdown. Driver logs remain visible when terminal background-output stopping is enabled.
Testing
Run on the Linux build box through
dcb up-os-1221withnice -n 15, Rust 1.95.0, and the gateway crate only:cargo fmt -p openshell-gateway -- --check;cargo clippy --locked -p openshell-gateway --features vendored-z3 --all-targets -- -D warnings.cargo test --locked -p openshell-gateway --features vendored-z3passed (37 library tests and 11 integration tests). The new output-pipe test failed against inherited stdio withdriver stdout must be piped, then passed with piped stdio.Checklist
Review follow-up
afb89df65b24dfaca8074132d292869d99155cd0. A managed VM driver now verifies that its parent is the expected gateway before and after arming procguard, and startup fails if the parent changed or procguard cannot arm. The process-level regression test timed out on the original code and passed after the fix.Follow-up checks passed through
dcb up-os-1221withnice -n 15:cargo fmt -p openshell-driver-vm -p openshell-gateway -- --check;cargo clippy --locked -p openshell-gateway -p openshell-driver-vm --features openshell-gateway/vendored-z3 --all-targets -- -D warnings; andcargo test --quiet --locked -p openshell-driver-vm -p openshell-gateway --features openshell-gateway/vendored-z3 --all-targets(345 passed).Manual verification
Result: PASS
Environment
mainatbe5e49499(fix(deps): update russh and pageant security fixes (#5123)), the base of this PR. After: PR headafb89df65b(fix(vm): reject managed driver after gateway exit). Both built on the host with Rust 1.95.0,cargo build --release -p openshell-gateway -p openshell-driver-vm(-p openshell-clifor the base), same embedded VM runtime, driver signed ad hoc with the Hypervisor entitlement.vm-runtimedarwin-aarch64 libkrun/libkrunfw, guestopenshell-sandboxfrom v0.1.6, locally builtopenshell-vm-initand host supervisor. Sandbox image:ubuntu:24.04from the registry.Steps (same for before and after)
compute_driver = "vm"), stdout/stderr on the terminal:openshell-gateway --config gateway.toml --port <port> --compute-driver vm --disable-tlsopenshell sandbox create --name <n> --from ubuntu:24.04 -- sleep 3600(sandbox reaches Ready, supervisor connected).ps -o pid,ppid,pgid,tpgid,stat), then start a sampler that records the gateway, driver and VM launcher processes every 0.2 s.tmux send-keys C-c, so the kernel delivers SIGINT to the terminal foreground process group).stty tostopset in the gateway terminal.Before (main,
be5e49499)The driver and both VM launchers share the gateway process group, which is the terminal foreground group (TPGID):
After Ctrl+C the driver gets the same SIGINT and stops itself at the same instant as the gateway; the shutdown sweep finds no driver:
The sampler shows the gateway, driver and launchers all gone within one 0.2 s sample (18:51:19.532 present, 18:51:19.765 empty). After restarting the gateway the sandbox is left in Error:
After (PR head,
afb89df65b)The driver and its launchers lead their own process group (PGID 52488); only the gateway is in the terminal foreground group (TPGID 52473):
After Ctrl+C the gateway runs its shutdown sweep against a live driver, the sandbox is stopped, and only then is the driver told to stop:
The sampler shows the driver and launchers present in every sample up to 18:55:21.849 and gone at 18:55:22.078; the log order above is what matters: the sweep stopped the sandbox (stopped=1) before the driver was told to stop. Restarting the gateway brings the same sandbox back:
Phase: Ready, new main process instance, no Error.Repeat with
stty tostop(verified withstty -a,tostopset): same result,stopped=1 failed=0, the driver log lines still appear in the gateway terminal (forwarded through the gateway), and nothing was suspended by SIGTTOU:Result
PASS. On main, Ctrl+C on a foreground VM gateway kills the driver first (stop sweep
stopped=0 failed=1, sandbox left in Error after restart). On the PR head the driver survives until the sweep has stopped the sandbox (stopped=1 failed=0), the sandbox restarts Ready, and the same holds withstty tostop.Not covered: the
--no-keepdelete-on-shutdown behaviour the PR description says is out of scope.