feat(minvmd): resource monitoring, configuration, and warnings (#747) - #775
Merged
Conversation
📝 WalkthroughWalkthroughminvmd adds persisted per-VM vCPU/RAM configuration, host-visible VMM metrics, effective resource resolution, booted-resource snapshots, expanded status output, validation warnings, CLI integration tests, and lifecycle verification. ChangesVM resource configuration and monitoring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Adds the `sysinfo` crate (workspace-pinned, default-features off + the `system` feature) so minvmd can sample per-process CPU / resident memory / disk I/O of the VMM child and probe host capacity, cross-platform on macOS/HVF and Linux/KVM. Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend `minvmd status --json` with live host-visible resource metrics
(CPU %, resident bytes, disk read/written bytes) sampled from the VMM
child PID, and add a `minvmd config` surface that validates and persists
per-VM vcpu/RAM parameters for the next boot.
- metrics: new `metrics` module samples `State.vmm_pid` via sysinfo; a
`StatusReport` DTO replaces the inline JSON, adding `metrics` (null
when stopped) and `warnings`.
- config: new `config.toml` (separate from `State`, which resets on
stop/crash) holds `ResourceConfig { vcpus, ram_mib }`; `config
show`/`set` report and persist them. Resolution is
`env ?? config ?? default`; `vmm_child` boots from the effective
values.
- warnings: proactive over-allocation guardrails at `config set` (host
cores/memory, x86_64 MMIO hole) and reactive memory/disk-pressure
thresholds surfaced by `status`, plus a supervisor post-exit resource
hint.
- The running state records the booted vcpus/ram (`State.booted_*`,
serde-default for back-compat) so status reports and warns against the
live VM's real allocation, not a later `config set`'s next-boot value.
The two `minimal` autospawn test literals that build a `State` are
updated to the new field set via `..State::stopped()`.
Closes: #747
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spec + architecture for the minvmd resource monitoring, configuration,
and warnings work: demoable units with frozen R{1,2,3}.x requirement
IDs, proof artifacts, an assumption ledger, and a verification table.
Refs: #747
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
force-pushed
the
feat/minvmd-resource-monitoring
branch
from
July 16, 2026 00:25
0a8c1cb to
d044177
Compare
Extend the KVM-lane daemon lifecycle proof (scripts/minvmd-lifecycle.sh) to exercise the #747 surface end-to-end against a real booted VM — the part no unit test can reach: - persist `config set --ram-mib 3072` before boot, assert `config show --json` reports it with source "config" (R2.3/R2.4); - assert the running VM's `status --json` reports `ram_mib == 3072`, proving the persisted config is consumed at a real boot (R2.5/R2.6); - assert the same status carries live host-visible `metrics` (numeric cpu/rss/disk) and a `warnings` array (R9.1/R9.9). Teardown removes the persisted config so the session-e2e step that runs next on the same default state dir is unaffected. Record the new proofs in the spec's verification table. Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add crates/minvmd/tests/resource_vm_integration.rs, a gated (#[cfg(minvmd_libkrun)] + #[ignore] + MINVMD_E2E=1) harness that boots a real microVM and asserts the running VM's status --json reports a persisted config's ram_mib (consumed at a real boot, R2.5/R2.6) plus live host-visible metrics (R9.1) and a warnings array (R9.9). Being a *_integration.rs harness it is auto-discovered by both VM lanes' nextest filterset, so unlike scripts/minvmd-lifecycle.sh (KVM-only) this also exercises the feature on the self-hosted macOS/HVF lane — covering the macOS sysinfo sampling path against a live VMM. Isolated to a per-test /tmp state dir (HOME + XDG_STATE_HOME), with a Drop guard that stops the detached daemon. Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address correctness findings from the branch code review: - Cap guest vcpus at a conservative MAX_VM_VCPUS (8): `config set` rejects above it and the boot resolution clamps an over-large env override, restoring the boot-safety the previously hardcoded `2` provided (an unbounded count could exceed the guest kernel's CONFIG_NR_CPUS and panic the boot). - Record booted_vcpus/booted_ram_mib from a single config read (effective_resources) so the recorded pair, and status's reported pair, cannot tear when config.toml changes between two effective_* calls. - Serialize `config set`'s read-modify-write under the lifecycle lock so two concurrent sets cannot lose one field's update. - Suppress the abnormal-exit resource hint on a signal-kill (code == None) — a deliberate `minvmd stop`, not a crash — so it fires only on a real guest-workload exit code. - Log (not silently swallow) a malformed config.toml when resolving effective resources, matching `config show`'s error surface. - Deduplicate the env-parsing helpers into a generic positive_env. - Format the resource_vm_integration harness (rustfmt). Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per code review, the reactive memory_pressure/disk_pressure threshold warnings cannot be measured accurately from the host, so they are removed rather than shipped as misleading signals: - memory_pressure used the VMM's RSS, which includes the guest's reclaimable page cache, so it trends toward the cap on any long-running VM (persistent false alarm). - disk_pressure used the sparse image's host block allocation — a monotonic high-water mark, not the guest ext4's free space — so it fired far later than, or never before, the guest hit ENOSPC, and its MINVMD_VOLUME_BYTES advice was a no-op on an existing volume. Accurate df/cgroup-based warnings need an in-guest agent (out of scope); the reliable reactive signal is the supervisor's abnormal-exit hint, which is retained. Drop `evaluate_warnings`/`data_volume_usage`, the `Warning` type, and the `warnings` field from `status --json`; keep raw metrics, proactive config-set validation, and the exit hint. Spec Unit 3 and the JSON schema are updated to match. Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
marked this pull request as ready for review
July 16, 2026 02:22
norrietaylor
enabled auto-merge (squash)
July 16, 2026 02:38
msample
reviewed
Jul 16, 2026
| /// and an unbounded count from config/env could exceed the guest kernel's | ||
| /// ceiling and panic the boot. Raise once the guest kernel's real limit is | ||
| /// confirmed higher. | ||
| pub const MAX_VM_VCPUS: u8 = 8; |
Member
There was a problem hiding this comment.
It would be more flexible if it would get the number of CPUs on the system and back off a couple as the max. E.g. I'm running this on big iron and want a lot of sandboxes to run concurrently in a VM.
Member
Author
There was a problem hiding this comment.
Done in 0d9d963 — the fixed cap is gone. The ceiling is now host-derived: max_vm_vcpus(logical_cores) = host logical cores minus a 2-core reserve for the host side, floored at the default (2) so tiny hosts keep the baseline, saturated to the u8 vcpu type. Both the boot-time clamp and config set validation use it, so big iron gets cores − 2 instead of 8.
One caveat carried into the spec's assumption ledger: the guest kernel's CONFIG_NR_CPUS still isn't pinned in this repo, so on a very-many-core host the derived ceiling could in theory exceed it — generic configs ship ≥ 64, so it's flagged needs-confirm rather than re-capped.
|
|
||
| /// Live host-visible resource usage of the VMM process. | ||
| #[derive(Debug, Clone, Serialize)] | ||
| pub struct VmMetrics { |
Member
There was a problem hiding this comment.
I think we should do this stuff as an RPC in minimald, so we can get it for the daemon case as well.
Member
Author
There was a problem hiding this comment.
I was thinking I want to stream them to a collector somewhere 😛
RPC sounds good! Do you think I should add here?
Replace the fixed MAX_VM_VCPUS = 8 with max_vm_vcpus(logical_cores): the host's logical core count minus a two-core host reserve, floored at DEFAULT_VM_VCPUS and saturated to the u8 vcpu type, so a many-core host can run a wide VM. Boot-time clamping and config-set validation share the derived ceiling; the guest-kernel CONFIG_NR_CPUS assumption is recorded in the spec's assumption ledger. Refs: #775 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After the guest vcpu ceiling became host-derived (0d9d963, `max_vm_vcpus = logical_cores - 2`, floored at DEFAULT_VM_VCPUS), the config-merge integration test's `config set --vcpus 3` is rejected on a 2-core CI runner (ceiling = 2), so the value never persisted and `set_merges` flaked on the x86_64 `tests`/`test-kvm` lanes. Use values valid on any runner yet still distinct from the defaults: `--ram-mib 3072` (off the 2048/4096 arch defaults, MMIO-hole-safe) and `--vcpus 1` (below even a 2-core ceiling, and != the vcpu default of 2). Also assert each `config set` succeeds, so a future rejection surfaces the command's stderr instead of a downstream value mismatch. Refs: #747 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
force-pushed
the
feat/minvmd-resource-monitoring
branch
from
July 16, 2026 14:48
386c692 to
9e01f41
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minvmd/src/cmd/config.rs`:
- Around line 146-159: Update run_show to resolve vcpus, ram_mib, and their
source labels from a single persisted configuration snapshot, reusing the same
malformed-TOML fallback semantics as boot resolution instead of calling
effective_vcpus(), effective_ram_mib(), or ResourceConfig::read independently.
Keep both JSON and human-readable output consistent with that one resolved
snapshot.
In `@crates/minvmd/src/cmd/run.rs`:
- Around line 410-423: Resolve the effective resource pair before spawning the
VMM child, then pass that snapshot to the child through arguments or dedicated
environment variables. Update the child startup path to use the supplied values,
and persist the same pre-spawn pair in the Running state via the booted_vcpus
and booted_ram_mib fields instead of calling effective_resources() after
readiness.
In `@crates/minvmd/src/cmd/vmm_child.rs`:
- Around line 57-63: Update the VmConfig construction in the child VM startup
flow to obtain vCPU and RAM values through the existing single-read
configuration helper, rather than calling effective_vcpus() and
effective_ram_mib() independently. Pass both values from that single resolved
configuration snapshot into VmConfig::new, preserving the kernel, rootfs, and
initramfs arguments.
In `@crates/minvmd/src/config.rs`:
- Around line 19-31: Revalidate persisted values from ResourceConfig before
effective_vcpus() and effective_ram_mib() pass them into VmConfig::new(...),
rejecting or ignoring vcpus = 0 and RAM values outside the existing safety
constraints so boot falls back to the built-in defaults. Add coverage for
invalid values loaded from config.toml, while preserving valid persisted values
and environment-variable precedence.
In `@crates/minvmd/src/state.rs`:
- Around line 234-247: Update atomic_write_toml to open and fsync the parent
directory after fs::rename completes, ensuring the rename is durable before
returning success. Preserve the existing temporary-file sync and error
propagation behavior.
In `@crates/minvmd/tests/config_cli_integration.rs`:
- Around line 37-79: Update the runner-independent tests
set_then_show_reports_persisted_value_and_source and
set_merges_without_clobbering_the_other_field to use 512 MiB instead of 3072 MiB
in the config set arguments and corresponding assertions/comments, while
preserving the existing persistence and merge checks.
In
`@docs/specs/09-spec-minvmd-resource-monitoring/09-spec-minvmd-resource-monitoring.md`:
- Around line 301-307: Replace the undefined R9.1 references in the Repository
Standards section and verification table with the applicable defined requirement
IDs R1.x, R2.x, or R3.x; do not introduce a new requirement unless R9.1 is
intentionally defined and consistently documented.
- Line 91: Update the wording in the non-VM path requirement from “behaviour” to
the US spelling “behavior,” without changing the surrounding meaning or
behavior.
- Around line 101-104: Remove or rewrite the running-VM resource-pressure
warning user story in the resource monitoring specification so it no longer
promises memory or disk pressure notifications. Keep the configuration-time
host-capacity warning story, and ensure the corresponding duplicate section
around the later referenced requirements is consistent with the stated
non-goals.
In `@scripts/minvmd-lifecycle.sh`:
- Around line 28-33: Update teardown() to avoid deleting a user’s pre-existing
default provider config.toml. Run the proof with an isolated XDG_STATE_HOME, or
capture and restore the original config around the test, while still cleaning up
the proof-created state and WORK directory.
- Around line 37-46: Clear the ambient MINVMD_VM_RAM_MIB override before the
persisted-resource verification in the config set/show block. Update the setup
around minvmd config set and config show so the subsequent jq assertion observes
ram_mib 3072 sourced from config, while preserving the existing proof flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ced538f-9e33-44bc-91c8-a6834ef68d44
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlcrates/minimal/src/autospawn.rscrates/minvmd/Cargo.tomlcrates/minvmd/src/cmd/config.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rscrates/minvmd/src/cmd/status.rscrates/minvmd/src/cmd/stop.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/config.rscrates/minvmd/src/lib.rscrates/minvmd/src/main.rscrates/minvmd/src/metrics.rscrates/minvmd/src/state.rscrates/minvmd/tests/config_cli_integration.rscrates/minvmd/tests/resource_vm_integration.rsdocs/specs/09-spec-minvmd-resource-monitoring/09-spec-minvmd-resource-monitoring.mddocs/specs/09-spec-minvmd-resource-monitoring/architecture.mdscripts/minvmd-lifecycle.sh
Comment on lines
+146
to
+159
| pub fn run_show(json: bool) -> Result<()> { | ||
| let cfg = ResourceConfig::read(&provider_dir()).context("reading resource config")?; | ||
| let vcpus = crate::cmd::effective_vcpus(); | ||
| let ram_mib = crate::cmd::effective_ram_mib(); | ||
| let vcpus_source = source(env_vcpus().is_some(), cfg.vcpus.is_some()); | ||
| let ram_source = source(env_ram_mib().is_some(), cfg.ram_mib.is_some()); | ||
|
|
||
| if json { | ||
| let out = serde_json::json!({ | ||
| "vcpus": vcpus, | ||
| "ram_mib": ram_mib, | ||
| "vcpus_source": vcpus_source, | ||
| "ram_mib_source": ram_source, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve values and sources from one configuration snapshot.
cfg, effective_vcpus(), and effective_ram_mib() perform three independent reads. A concurrent config set can therefore produce mixed values or source labels that do not describe the reported values. Additionally, the direct read makes config show fail on malformed TOML while boot resolution falls back to defaults.
Centralize value/source resolution around one persisted-config read using the same fallback semantics as boot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/cmd/config.rs` around lines 146 - 159, Update run_show to
resolve vcpus, ram_mib, and their source labels from a single persisted
configuration snapshot, reusing the same malformed-TOML fallback semantics as
boot resolution instead of calling effective_vcpus(), effective_ram_mib(), or
ResourceConfig::read independently. Keep both JSON and human-readable output
consistent with that one resolved snapshot.
Comment on lines
+410
to
+423
| // Record the resources the running VM was booted with (R2.6), resolved | ||
| // from a single config read so the (vcpus, ram_mib) pair cannot tear. The | ||
| // VMM child resolves the same effective values from the inherited env + | ||
| // shared config.toml; a `config set` landing inside the brief boot window | ||
| // could still make this parent read diverge from the child's, but the | ||
| // recorded pair itself is always self-consistent. | ||
| let (booted_vcpus, booted_ram_mib) = crate::cmd::effective_resources(); | ||
| state_dir | ||
| .write_state(&State { | ||
| lifecycle: running, | ||
| vmm_pid: Some(child_pid), | ||
| started_at: Some(started_at), | ||
| booted_vcpus: Some(booted_vcpus), | ||
| booted_ram_mib: Some(booted_ram_mib), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Record the exact values passed to the VMM child.
These values are resolved after the VM reaches readiness, independently of the child's earlier resolution. A config set during boot can therefore make booted_* describe the next configuration rather than the running VM.
Resolve the pair before spawning, pass that snapshot to the child via arguments or dedicated environment variables, and persist the same pair at the Running transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/cmd/run.rs` around lines 410 - 423, Resolve the effective
resource pair before spawning the VMM child, then pass that snapshot to the
child through arguments or dedicated environment variables. Update the child
startup path to use the supplied values, and persist the same pre-spawn pair in
the Running state via the booted_vcpus and booted_ram_mib fields instead of
calling effective_resources() after readiness.
Comment on lines
+57
to
+63
| let mut cfg = VmConfig::new( | ||
| crate::cmd::effective_vcpus(), | ||
| crate::cmd::effective_ram_mib(), | ||
| kernel, | ||
| rootfs, | ||
| initramfs, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the boot resource pair atomically.
The two effective_* calls independently read config.toml; a concurrent config set can combine vCPUs from one version with RAM from another. Use the existing single-read helper.
Proposed fix
+ let (vcpus, ram_mib) = crate::cmd::effective_resources();
let mut cfg = VmConfig::new(
- crate::cmd::effective_vcpus(),
- crate::cmd::effective_ram_mib(),
+ vcpus,
+ ram_mib,
kernel,
rootfs,
initramfs,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut cfg = VmConfig::new( | |
| crate::cmd::effective_vcpus(), | |
| crate::cmd::effective_ram_mib(), | |
| kernel, | |
| rootfs, | |
| initramfs, | |
| ); | |
| let (vcpus, ram_mib) = crate::cmd::effective_resources(); | |
| let mut cfg = VmConfig::new( | |
| vcpus, | |
| ram_mib, | |
| kernel, | |
| rootfs, | |
| initramfs, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/cmd/vmm_child.rs` around lines 57 - 63, Update the VmConfig
construction in the child VM startup flow to obtain vCPU and RAM values through
the existing single-read configuration helper, rather than calling
effective_vcpus() and effective_ram_mib() independently. Pass both values from
that single resolved configuration snapshot into VmConfig::new, preserving the
kernel, rootfs, and initramfs arguments.
Comment on lines
+19
to
+31
| /// Persisted resource parameters. A `None` field means "unset — use the | ||
| /// built-in default"; an explicit value overrides the default (but is itself | ||
| /// overridden by the matching environment variable at resolution time, see | ||
| /// [`crate::cmd::effective_ram_mib`]). | ||
| #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[non_exhaustive] | ||
| pub struct ResourceConfig { | ||
| /// Persisted vcpu count, if set. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub vcpus: Option<u8>, | ||
| /// Persisted guest RAM in MiB, if set. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub ram_mib: Option<u32>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 3 'ResourceConfig::read|effective_(vcpus|ram_mib)|validate.*(resource|vcpus|ram_mib)|booted_(vcpus|ram_mib)' \
crates/minvmd/src crates/minvmd/testsRepository: gominimal/minimal
Length of output: 17310
🏁 Script executed:
sed -n '140,240p' crates/minvmd/src/cmd/mod.rs
printf '\n---\n'
sed -n '1,190p' crates/minvmd/src/cmd/config.rs
printf '\n---\n'
sed -n '1,220p' crates/minvmd/tests/config_cli_integration.rsRepository: gominimal/minimal
Length of output: 15665
🏁 Script executed:
rg -n -C 3 'struct VmConfig|impl VmConfig|fn new\(|ram_mib|vcpus' crates/minvmd/src crates/minvmd/testsRepository: gominimal/minimal
Length of output: 49187
🏁 Script executed:
rg -n -C 4 'struct VmConfig|impl VmConfig|fn new\(|ram_mib|vcpus' crates/minvmd/src crates/minvmd/testsRepository: gominimal/minimal
Length of output: 50373
Revalidate persisted resources on boot
config set only protects the write path; effective_vcpus() / effective_ram_mib() still trust config.toml at boot, so a hand-edited or stale file can carry vcpus = 0 or an unsafe RAM value into VmConfig::new(...). Re-check the persisted layer before boot and add coverage for invalid on-disk values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/config.rs` around lines 19 - 31, Revalidate persisted
values from ResourceConfig before effective_vcpus() and effective_ram_mib() pass
them into VmConfig::new(...), rejecting or ignoring vcpus = 0 and RAM values
outside the existing safety constraints so boot falls back to the built-in
defaults. Add coverage for invalid values loaded from config.toml, while
preserving valid persisted values and environment-variable precedence.
Comment on lines
+234
to
+247
| /// Serialise `value` to TOML and write it to `target` atomically: content is | ||
| /// written to a `.toml.tmp` sibling, `fsync`'d, then renamed over the target | ||
| /// (R4.1). Shared by [`StateDir::write_state`] and | ||
| /// [`crate::config::ResourceConfig::write`]. | ||
| pub(crate) fn atomic_write_toml<T: Serialize>(target: &Path, value: &T) -> io::Result<()> { | ||
| let tmp = target.with_extension("toml.tmp"); | ||
| let serialised = | ||
| toml::to_string(value).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; | ||
| { | ||
| let mut f = File::create(&tmp)?; | ||
| f.write_all(serialised.as_bytes())?; | ||
| f.sync_all()?; | ||
| } | ||
| fs::rename(&tmp, target) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Sync the parent directory after the rename.
f.sync_all() persists the temporary file contents, but not the rename’s directory entry. A crash after Line 247 can therefore lose an acknowledged state/config update.
Proposed durability fix
- fs::rename(&tmp, target)
+ fs::rename(&tmp, target)?;
+ if let Some(parent) = target.parent() {
+ File::open(parent)?.sync_all()?;
+ }
+ Ok(())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Serialise `value` to TOML and write it to `target` atomically: content is | |
| /// written to a `.toml.tmp` sibling, `fsync`'d, then renamed over the target | |
| /// (R4.1). Shared by [`StateDir::write_state`] and | |
| /// [`crate::config::ResourceConfig::write`]. | |
| pub(crate) fn atomic_write_toml<T: Serialize>(target: &Path, value: &T) -> io::Result<()> { | |
| let tmp = target.with_extension("toml.tmp"); | |
| let serialised = | |
| toml::to_string(value).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; | |
| { | |
| let mut f = File::create(&tmp)?; | |
| f.write_all(serialised.as_bytes())?; | |
| f.sync_all()?; | |
| } | |
| fs::rename(&tmp, target) | |
| /// Serialise `value` to TOML and write it to `target` atomically: content is | |
| /// written to a `.toml.tmp` sibling, `fsync`'d, then renamed over the target | |
| /// (R4.1). Shared by [`StateDir::write_state`] and | |
| /// [`crate::config::ResourceConfig::write`]. | |
| pub(crate) fn atomic_write_toml<T: Serialize>(target: &Path, value: &T) -> io::Result<()> { | |
| let tmp = target.with_extension("toml.tmp"); | |
| let serialised = | |
| toml::to_string(value).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; | |
| { | |
| let mut f = File::create(&tmp)?; | |
| f.write_all(serialised.as_bytes())?; | |
| f.sync_all()?; | |
| } | |
| fs::rename(&tmp, target)?; | |
| if let Some(parent) = target.parent() { | |
| File::open(parent)?.sync_all()?; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minvmd/src/state.rs` around lines 234 - 247, Update atomic_write_toml
to open and fsync the parent directory after fs::rename completes, ensuring the
rename is durable before returning success. Preserve the existing temporary-file
sync and error propagation behavior.
| values (including a vcpu count above the host-derived ceiling). | ||
| 4. An abnormal VMM-child exit (a guest workload's non-zero exit) prints a resource | ||
| hint. | ||
| 5. No new behaviour on the non-VM (native minimald) path; the change is confined |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use US spelling: behavior.
This matches the repository’s apparent US-English wording and resolves the reported spelling warning.
🧰 Tools
🪛 LanguageTool
[grammar] ~91-~91: Ensure spelling is correct
Context: ... No new behaviour on the non-VM (native minimald) path; the change is confined to the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/specs/09-spec-minvmd-resource-monitoring/09-spec-minvmd-resource-monitoring.md`
at line 91, Update the wording in the non-VM path requirement from “behaviour”
to the US spelling “behavior,” without changing the surrounding meaning or
behavior.
Source: Linters/SAST tools
Comment on lines
+101
to
+104
| - As a developer, I want `minvmd config set` to warn me when I ask for more vcpus | ||
| or RAM than the host has, so that I do not silently misconfigure the VM. | ||
| - As a developer, I want a running VM that is nearly out of memory or disk to tell | ||
| me so, and name the knob to raise, so that I can react before a failure. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove or rewrite the stale reactive-warning user story.
Lines 103–104 promise running pressure warnings, but the same specification explicitly removes them as unreliable and lists them as non-goals. This contradiction can mislead implementation and acceptance criteria.
Also applies to: 256-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/specs/09-spec-minvmd-resource-monitoring/09-spec-minvmd-resource-monitoring.md`
around lines 101 - 104, Remove or rewrite the running-VM resource-pressure
warning user story in the resource monitoring specification so it no longer
promises memory or disk pressure notifications. Keep the configuration-time
host-capacity warning story, and ensure the corresponding duplicate section
around the later referenced requirements is consistent with the stated
non-goals.
Comment on lines
+301
to
+307
| ## Repository Standards | ||
|
|
||
| - CLI/command layer returns `anyhow::Result` with actionable `.context`/`bail!`; | ||
| the hand-rolled `VmError` enum is unchanged (not converted to `thiserror`). | ||
| - `sysinfo` is pinned in the workspace `[workspace.dependencies]`; the crate | ||
| inherits via `workspace = true`. | ||
| - Requirement IDs are anchored in code/tests as `// R9.x` comments. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the undefined R9.1 traceability references.
The requirements defined here are R1.x, R2.x, and R3.x, but the repository-standards section and verification table reference R9.1 without defining it. Either define that requirement or replace those references with the applicable existing IDs.
Also applies to: 342-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/specs/09-spec-minvmd-resource-monitoring/09-spec-minvmd-resource-monitoring.md`
around lines 301 - 307, Replace the undefined R9.1 references in the Repository
Standards section and verification table with the applicable defined requirement
IDs R1.x, R2.x, or R3.x; do not introduce a new requirement unless R9.1 is
intentionally defined and consistently documented.
Comment on lines
28
to
33
| teardown() { | ||
| minvmd stop >/dev/null 2>&1 || true | ||
| # Drop the resource config this proof persisted so it cannot alter the | ||
| # session-e2e boot that runs next on the same default state dir. | ||
| rm -f "${XDG_STATE_HOME:-$HOME/.local/state}/minimal/providers/local-0/config.toml" | ||
| rm -rf "$WORK" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve any pre-existing resource configuration.
Teardown unconditionally deletes the default provider’s config.toml. Running this proof locally can permanently erase the user’s existing VM resource settings. Back up and restore the file, or run the proof under an isolated XDG_STATE_HOME.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/minvmd-lifecycle.sh` around lines 28 - 33, Update teardown() to avoid
deleting a user’s pre-existing default provider config.toml. Run the proof with
an isolated XDG_STATE_HOME, or capture and restore the original config around
the test, while still cleaning up the proof-created state and WORK directory.
Comment on lines
+37
to
+46
| # Persist a resource config BEFORE boot so the running VM demonstrably | ||
| # consumes it (R2.3/R2.4/R2.5). 3072 MiB differs from the x86_64 default | ||
| # (2048) and is MMIO-hole-safe (<=3072), so `status` reporting 3072 while | ||
| # running is unambiguous proof the persisted config reached the VMM child at | ||
| # boot rather than an env override or the default. | ||
| echo "::group::config set + show (persist ram_mib for next boot)" | ||
| minvmd config set --ram-mib 3072 | ||
| minvmd config show --json > "$WORK/config.json" | ||
| cat "$WORK/config.json" | ||
| jq -e '.ram_mib == 3072 and .ram_mib_source == "config"' "$WORK/config.json" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear ambient resource overrides before proving persisted resolution.
An existing MINVMD_VM_RAM_MIB overrides the persisted value, making this CI proof fail for reasons unrelated to the change.
+# Ensure environment overrides cannot mask persisted configuration.
+unset MINVMD_VM_RAM_MIB MINVMD_VM_VCPUS
+
echo "::group::config set + show (persist ram_mib for next boot)"As per coding guidelines, CI coverage in scripts/ should follow the repository CI strategy and contribution contract. <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Persist a resource config BEFORE boot so the running VM demonstrably | |
| # consumes it (R2.3/R2.4/R2.5). 3072 MiB differs from the x86_64 default | |
| # (2048) and is MMIO-hole-safe (<=3072), so `status` reporting 3072 while | |
| # running is unambiguous proof the persisted config reached the VMM child at | |
| # boot rather than an env override or the default. | |
| echo "::group::config set + show (persist ram_mib for next boot)" | |
| minvmd config set --ram-mib 3072 | |
| minvmd config show --json > "$WORK/config.json" | |
| cat "$WORK/config.json" | |
| jq -e '.ram_mib == 3072 and .ram_mib_source == "config"' "$WORK/config.json" | |
| # Persist a resource config BEFORE boot so the running VM demonstrably | |
| # consumes it (R2.3/R2.4/R2.5). 3072 MiB differs from the x86_64 default | |
| # (2048) and is MMIO-hole-safe (<=3072), so `status` reporting 3072 while | |
| # running is unambiguous proof the persisted config reached the VMM child at | |
| # boot rather than an env override or the default. | |
| # Ensure environment overrides cannot mask persisted configuration. | |
| unset MINVMD_VM_RAM_MIB MINVMD_VM_VCPUS | |
| echo "::group::config set + show (persist ram_mib for next boot)" | |
| minvmd config set --ram-mib 3072 | |
| minvmd config show --json > "$WORK/config.json" | |
| cat "$WORK/config.json" | |
| jq -e '.ram_mib == 3072 and .ram_mib_source == "config"' "$WORK/config.json" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/minvmd-lifecycle.sh` around lines 37 - 46, Clear the ambient
MINVMD_VM_RAM_MIB override before the persisted-resource verification in the
config set/show block. Update the setup around minvmd config set and config show
so the subsequent jq assertion observes ram_mib 3072 sourced from config, while
preserving the existing proof flow.
Source: Coding guidelines
twitchyliquid64
approved these changes
Jul 16, 2026
This was referenced Jul 21, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Implements #747: live VM resource monitoring, a runtime configuration surface, and resource warnings for the
minvmdhost daemon.minvmd status --jsongains ametricsobject (cpu_percent,resident_bytes,disk_read_bytes,disk_written_bytes) sampled host-side from the VMM child PID (State.vmm_pid) viasysinfo;nullwhen the VM is stopped. Every pre-existing field keeps its name and type.minvmd config set --vcpus/--ram-mibvalidates and persists per-VM resource params to a newconfig.toml;minvmd config show [--json]reports the effective values and each value's source. Boot resolution isenv ?? persisted config ?? default.config set(host cores/memory, x86_64 MMIO hole) and reactive memory/disk-pressure thresholds surfaced bystatus, plus a supervisor post-exit resource hint. Ports theminimal-vm-macprior art (which samples the host VMM by PID and points users at the knob) to a typed, tested, host-side implementation.Design decisions
config.toml, notState.State::stopped()/StartingGuardreset runtime state on every stop or crash, which would silently wipe config kept there.dfintrospection.Runningstate records the bootedvcpus/ram_mib(State.booted_*,serde-default for back-compat).statusreports and warns against the value the live VM actually booted with, not a laterconfig set's next-boot resolution — otherwiseconfig set --ram-mib 16384on a VM booted at 2048 would silence a genuinememory_pressurewarning. (This came out of an adversarial self-review of the diff.)Out of scope / noted
stop.rsquiesce concern surfaced during review —minvmd stopunconditionally signals the VMM child after the guest Shutdown RPC. Untouched here (belongs to spec Packaging punch list #8); worth a separate issue.Testing
cargo test -p minvmd— 123 lib + 4 CLI-integration tests pass; gated VM harnesses (MINVMD_E2E=1) self-skip.cargo clippy -p minvmd --all-targets -- -D warnings— clean;cargo fmtapplied.tests/config_cli_integration.rsdrives the real binary (config set/show/merge, invalid-value rejection,status --jsonschema) with no VM required.MINVMD_E2E=1) and is documented in the spec's verification table.Spec
SDD spec added at
docs/specs/09-spec-minvmd-resource-monitoring/(spec + architecture, frozenR{1,2,3}.xIDs, proof artifacts, assumption ledger).🤖 Generated with Claude Code
Summary by CodeRabbit
minvmd config showandconfig setfor viewing and configuring VM vCPUs and memory.statusoutput with booted resources, uptime, process ID, and live CPU, memory, and disk metrics.