Repository navigation
T59 — Create a named VM with its own state, socket and daemon, leaving the default VM's paths unchanged - #1702
Conversation
A named VM (NET-052, NET-054) keeps its state, socket and locks under a per-name subdirectory of the provider dir: <state>/providers/local-<kind><N>/<vm>/. provider_instance_dir_named resolves that layout from an explicit VM name; DEFAULT_VM_NAME (default) resolves to the provider-instance dir itself so the default VM's paths are byte-for-byte unchanged (NET-053), and validate_vm_name rejects any name that is not a single path component, so a name can only select a subdirectory under the provider dir, never escape it.
A --vm NAME global argument selects the VM minvmd acts on (NET-052). The name is validated at parse time, before any path resolution, and every path the daemon derives then flows from provider_dir_for: a named VM's state, locks, bridge socket, config, volume, logs and switch socket all live under <state>/providers/local-minvmd0/<name>/ (NET-054), while the default VM resolves to the provider dir itself, byte for byte as before (NET-053). Every re-exec (run --detach, the __krun-vmm child) forwards the name via state::forward_identity, so the detached supervisor and its child keep serving the same VM — and a named VM's processes carry their name on the command line, which is what reap-vms.sh --vm matches on. One info line per VM start and stop names the VM and its state directory; the foreground layer now disables ANSI off-terminal so the name stays searchable in piped logs. scripts/reap-vms.sh gains --vm NAME to reap only that VM's processes, still pinned to this checkout's path (NET-056): minvmd processes by their --vm argument, the VM's gvproxy switch by its per-name socket directory. Bare invocation still reaps every VM this checkout spawned.
`min --vm NAME` now publishes the name at dispatch, so every path the CLI resolves names the same VM (NET-052): - minimal-client grows the process-global VM name plus `resolve_provider_dir_named`/`resolve_socket_path_named`; a named VM nests under `<state>/providers/local-minvmd0/<vm>`, the default VM keeps the bare provider dir (NET-053), and the native minimald backend — which hosts no VMs — ignores the name. The existing two-arg resolvers delegate, so every caller follows the name without edits. - autospawn reads the named VM's state dir and forwards `--vm` to the `minvmd run --detach` it spawns, so `min --vm alpha` boots alpha rather than a second default VM. - `min bug` lists every VM as its own `local-minvmd0/<vm>` bundle entry (the guest payload subdir is not a VM), and each status.json names the VM with its state directory and socket. - GlobalArgs literals in test code gain the new `vm: None` field; the publish hook itself is one call in main.rs before dispatch.
…amed-vm-with-its-own-state-socket-and
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Babysitter review of 60175b5 against the T59 plan block on #1626 (NET-052..056). Design fits and every layer intent is met with evidence; all 7 High —
Medium
Low
Round 1 is in flight on the failed check suite. A |
The macOS and KVM lanes replay every `*_integration` harness from a
nextest archive built on another machine (`--workspace-remap`), so no
path baked into the binary at build time exists there. The new
named_vm_integration harness ignored that contract: it spawned
`env!("CARGO_BIN_EXE_minvmd")` and copied
`CARGO_MANIFEST_DIR/../../scripts/reap-vms.sh`, both of which point at
the build runner. Every test in the harness panicked with ENOENT, which
is what took the macOS e2e job (and with it ci-macos-success) red —
`unit` runs `-E 'not binary(/_integration$/)'` and so never saw it.
Follow the contract every other harness already documents: take the
binary from `MINVMD_BIN`, and embed the reap script in the harness with
`include_str!` so it travels with the archive. Verified by replaying
the archive with both build-time paths removed and `MINVMD_BIN` set,
the exact shape of the lanes' harness step: 0/5 before, 5/5 after.
The run helper's doc still read as if the binary were always the one cargo built beside the harness; since the previous commit it comes from MINVMD_BIN on the VM lanes' split build/test runners.
…amed-vm-with-its-own-state-socket-and
|
Round 1 of 10 on a failed check suite, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. The named-VM proof stands at this head — a second VM named with Notes from the agent: Revise round: the two failed checks were ci-macos-success and e2e, and neither the macOS |
|
/revise Round 1 fixed the archive-lane failure (
Keep the seven plan test lines green. |
Review round 2 on the named-VM PR, three findings: - validate_vm_name now holds VM names to an allowlist instead of a four-case blocklist: ASCII lowercase letters, digits and `-`, starting with a letter or digit, at most 24 bytes so the per-name socket path still fits the 104-byte macOS sun_path, and `guest` reserved for the diag bundle's in-guest provider subdirectory. The error names the rule it enforces. - A named VM against the native `local-minimald` backend is refused with "named VMs need the minvmd provider" instead of silently falling back to the default daemon's directory, which would have `min --vm alpha` talking to the default VM while claiming to talk to alpha. The default VM still resolves - it is the only VM that backend has. The refusal test is Linux-only, since client_provider_kind forces Minvmd on macOS. - The default-resolver steering test no longer leaks the process-global VM name into the rest of the crate under plain `cargo test`: the real assertion runs in a spawned child behind an env marker, so both runners see the same green crate.
Review round 2, the low finding: a VM's start/stop pair must be attributable in the logs. - Every stop funnels through one helper that logs "stopping VM" with the VM name and state directory: the `minvmd stop` CLI before it signals, the run supervisor at VMM-child exit - which is where a `min stop` surfaces - and foreground `boot`. - The detached supervisor now runs inside a `vm` info span, so its records carry the VM they belong to even in the shared <state>/logs/minvmd.log every VM writes; the JSON layer flattens span fields onto the record, matching minimald's `conn` span. - The autospawn shutdown hint names the VM when one is in use: `minvmd --vm alpha stop`.
…amed-vm-with-its-own-state-socket-and
|
Round 2 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. A named VM still boots with its own state directory, socket and daemon under a per-name subdirectory while the default VM's paths stay unchanged — and a reviewer can now also see that Notes from the agent: Round 2 (directive): all four findings addressed; no threads section in the directive, so no thread verdicts. (1) The native-backend refusal lives at the top of |
|
Babysitter verification of round 2 (3353f9c, deb65f3; head 81b0431) against the
test-kvm and ci-macos are green at 81b0431. Out-of-layer files are all in scope for the named-VM plumbing this task adds, so the PR is taken out of draft for the bot review; merge follows minimal-review and CodeRabbit. |
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none: the base was merged in and |
Review round 3, the low finding: `minvmd stop` logged "stopping VM" unconditionally, so a stop of a supervised VM put the line in the log twice - once from the CLI before it signals, once from the `run` supervisor when the signalled VMM child exits - and an idempotent stop of an already-stopped VM logged a stop that never happened, the opposite of the "one line per VM stop" the comment claimed. The line now comes only from the process that observes the VM die: the `run` supervisor at VMM-child exit, or a foreground `boot`. That witness is alive whenever `stop` actually stops something - it holds the alive lock `stop` requires before it signals - so no stop loses its line, and a no-op stop (already stopped, stale state) logs none. `cmd/mod.rs` is outside this task's layers; only its helper doc changed, because it described the CLI logging this round removes. The named-VM harness cannot run a supervisor on a libkrun-less host, so it now pins the CLI-side invariants that keep the line one-per-stop: the stop CLI emits no "stopping VM" line itself, and a no-op stop logs none.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A second named VM boots with its own state directory, socket and daemon under a per-name subdirectory, the default VM's paths are unchanged, stopping one VM leaves the other running, and reaping touches only this checkout's VMs; and every VM stop now yields exactly one 'stopping VM' line naming the VM and its state directory, logged by the witness that observes the VM die — a supervised stop is no longer double-logged, and a no-op stop logs nothing. Notes from the agent: Round 3 addressed the single review thread (the duplicate/false 'stopping VM' line). The stop CLI no longer logs the one-per-stop line at all: the |
|
/revise Round 3 (646f44b) is verified: the supervisor is the stop line's only witness, the CLI logs none, and the integration test pins both the supervised and the no-op case. One item remains, minimal-review's Low on
Keep the plan's seven test lines green. The PR stays a draft until the round lands and is then un-drafted. |
The default VM's processes carry no --vm flag (minvmd's re-exec and the CLI's autospawn omit it for the default name) and its switch socket sits in no per-name subdirectory, so the name-pinned patterns matched nothing and the script exited 0 having reaped no VM. Clearing the name makes --vm default reap the default VM like the bare invocation does.
…amed-vm-with-its-own-state-socket-and
|
Round 4 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A second named VM boots with its own state directory, socket and daemon under a per-name subdirectory, the default VM's paths are unchanged, stopping one VM leaves the other running, and reaping touches only this checkout's VMs — and as of this round Notes from the agent: Round 4 addressed the one remaining item: the minimal-review LOW on scripts/reap-vms.sh. The finding's premise was verified against the code (state::reexec_vm_args and minimal's autospawn omit --vm for DEFAULT_VM_NAME; paths::provider_instance_dir_named gives the default VM no per-name subdirectory), so it was fixed as the note directed: the script clears the VM name when it equals |
|
Babysitter verification of round 4 (a09cb99, head 80e64eb): |
There was a problem hiding this comment.
Babysitter approval: T59 verified against the plan block through four rounds, all lanes green at 80e64eb including test-kvm and ci-macos, minimal-review APPROVE on this head, CodeRabbit passed, no open threads.
T59 — Create a named VM with its own state, socket and daemon, leaving the default VM's paths unchanged
slice: S15
kind: task
covers: NET-052, NET-053, NET-054, NET-055, NET-056
size: L
est: 400 code, 420 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: the bundle lists every VM by name with its state directory and socket
observability: one info log line per VM start and stop naming the VM and its state directory
layers:
- vmhost:
- crates/minvmd/src/state.rs: a VM name selects a per-name subdirectory of the provider directory; the default stays where it is
- crates/minvmd/src/main.rs: a global VM-name argument
- crates/minvmd/src/cmd/run.rs: forwards the name on re-exec
- crates/minvmd/src/cmd/boot.rs: forwards the name on re-exec
- scripts/reap-vms.sh: reaps only this checkout's VMs, by name
test: cargo nextest run -p minvmd named_vm_has_own_state_socket_daemon
test: cargo nextest run -p minvmd default_vm_paths_unchanged
test: cargo nextest run -p minvmd named_vm_under_per_name_subdirectory
test: cargo nextest run -p minvmd stop_one_vm_leaves_other_running
test: cargo nextest run -p minvmd reap_scoped_per_checkout
- paths:
- crates/paths/src/lib.rs: a per-name subdirectory helper
test: cargo nextest run -p paths provider_instance_dir_accepts_a_name
- client:
- crates/minimal-client/src/lib.rs: resolve_provider_dir and resolve_socket_path take a VM name
- crates/minimal/src/autospawn.rs: passes the VM name and uses its state directory
- crates/minimal/src/cli.rs: a way to ask for a named VM
- crates/minimal/src/diag/collect.rs: provider_dirs lists named VMs
test: cargo nextest run -p minimal named_vm_reaches_autospawn
produces: none
consumes: none
proof: A second named VM boots with its own state directory, socket and daemon under a per-name subdirectory, the default VM's paths are unchanged, stopping one VM leaves the other running, and reaping touches only this checkout's VMs.
Proof
A second named VM boots with its own state directory, socket and daemon under a per-name subdirectory (
minvmd --vm NAME runserves<state>/providers/local-minvmd0/<name>/, andmin --vm NAMEreads and autospawns that same VM), the default VM's every path is byte-for-byte unchanged, stopping one VM leaves the other running, and reaping (scripts/reap-vms.sh, bare or--vm NAME) touches only this checkout's VMs; themin bugbundle lists every VM by name with its state directory and socket, and each VM start and stop logs one info line naming the VM and its state directory.Notes from the agent: Layers held, with files outside them to name: (1) crates/minimal/src/main.rs — one hook call,
cli.global_args.publish_vm_name()before dispatch, because the parsed--vmmust reach minimal-client's process-global before any path resolution; the accessor lives on GlobalArgs in the named layer file cli.rs. (2) crates/minimal/src/loadouts.rs, crates/minimal/src/tests.rs, crates/minimal/tests/bug.rs, crates/minimal/tests/cli.rs, crates/minimal/tests/common/mod.rs — mechanicalvm: Noneadded to 20 GlobalArgs test literals (the new field is on a struct a layer names; all 20 sites are test code, found by the compiler). (3) Test files created for the test: lines themselves: crates/minvmd/tests/named_vm_integration.rs (named per the crate's enforced_integrationsuffix convention) and crates/minimal/tests/named_vm.rs. Also inside layers but worth knowing: minvmd's foreground tracing layer now disables ANSI off-terminal so the start/stop lines are greppable in piped logs. This box has no libkrun, so minvmd's real VM-boot code (run/bootbodies) compiles as runtime-bailing stubs and is not exercised here: the five minvmd tests drive the real binary for status/stop/log-line behavior and use in-process alive-lock holders for daemons, so they pass on any host and stay meaningful under libkrun; a real boot is owed to the KVM lane (just test-vm / just e2e). Also…[truncated]Tests
cargo nextest run -p minvmd named_vm_has_own_state_socket_daemoncargo nextest run -p minvmd default_vm_paths_unchangedcargo nextest run -p minvmd named_vm_under_per_name_subdirectorycargo nextest run -p minvmd stop_one_vm_leaves_other_runningcargo nextest run -p minvmd reap_scoped_per_checkoutcargo nextest run -p paths provider_instance_dir_accepts_a_namecargo nextest run -p minimal named_vm_reaches_autospawnVerify
min task run agent-verifypassed ([tasks.verify]).Base
The base branch moved while the coder worked and was merged in before the tests ran. These files changed on BOTH sides since the branch point — read them first, because a rewrite from a stale reading can undo a merge without a conflict:
crates/minimal/tests/cli.rsOut of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimal/src/loadouts.rscrates/minimal/src/main.rscrates/minimal/src/tests.rscrates/minimal/tests/bug.rscrates/minimal/tests/cli.rscrates/minimal/tests/common/mod.rscrates/minimal/tests/named_vm.rscrates/minvmd/tests/named_vm_integration.rsCloses #1626