feat(minvmd,minimald,minimal): persistent, correlatable daemon logs - #833
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds daily-rotated logging for detached minvmd and mounted minimald state volumes, releases volume log writers before shutdown quiescing, improves connection/session diagnostics, and updates boot-log and directory-reporting behavior. ChangesLogging lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Both daemons persist rolling file logs so field diagnostics exist before the incident. minvmd, when detached, routes tracing to a size-rotated <state>/logs/minvmd.log keyed on a MINVMD_DETACHED re-exec marker, and the VMM child defaults the hvc0 console capture to <provider>/boot.log (truncated per boot, MINVMD_BOOT_LOG overrides, wiring failure warns and boots on) — the justfile and session-e2e exports drop in the same change so dev flows keep the console. The in-VM minimald attaches a rolling on-volume appender through a tracing reload layer activated after the state volume mounts; the release is a one-shot VolumeLogRelease owned by ServerState and run by the Shutdown RPC before the quiesce, so the appender's write-open fd never defeats the clean unmount. Connection and binding lifecycle records carry their ids via tracing spans instead of manual per-line fields, and routine peer hangups log as closes, not errors. Rotation is size-based with bounded retention via logroller (10 MiB, keep 5) wrapped in non_blocking with lossy(false); graceful_shutdown joins the archive-publication thread on drop so a release leaves no .pending. orphans. The min dirs daemon-logs note now names both daemons' rotated files. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
83819e8 to
44583b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/run.rs`:
- Around line 73-75: Update the __krun-vmm command construction in run.rs to
remove crate::DETACHED_ENV before spawning the VMM, using env_remove on the
command. Keep the supervisor’s detached marker unchanged while ensuring the VMM
does not inherit it or use the file-backed tracing path.
🪄 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: ee5f5b20-40af-4f8d-b2c7-7d23f0183b15
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/minimal/src/dirs.rscrates/minimald/Cargo.tomlcrates/minimald/src/main.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/session_host.rscrates/minimald/src/test_harness.rscrates/minvmd/Cargo.tomlcrates/minvmd/src/cmd/run.rscrates/minvmd/src/cmd/vmm_child.rscrates/minvmd/src/lib.rscrates/minvmd/src/main.rsjustfilescripts/session-e2e.sh
There was a problem hiding this comment.
Make a new DaemonLogger struct type that encapsulates the log roller, logic for creating it, and the method/logic for releasing it. We want to keep a lot of the complexity there and let the main method and let the server struct/state logic delegate to it.
I also don't see why we cant make the wiring for the minvmd path and the non-minvmd path the same - i dont see why we cant release the log on shutdown in both cases. If we need the tracing subscriber registry thing to be reloadable, do that in both cases (vm and non vm) rather than having a different type hierarchy.
(this is with regards to the minimald logging path/setup)
…logroller Revert the daemon log rotation from logroller's size-based scheme back to tracing-appender's daily rotation (the reference implementation's approach), trading the intra-day size cap for a smaller dependency surface and simpler shutdown. tracing-appender rotates and prunes inline — no background publication thread — so the appender needs no graceful-shutdown handshake and leaves no partial `.pending.` files on an unmounting volume; the volume-log release closes the file with a plain guard drop. Retention stays bounded via `max_log_files` (14 days). Rotated files are date-suffixed again; `newest_rotated`'s mtime ordering already handles that scheme. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A detached `run` supervisor carries MINVMD_DETACHED, which routes tracing to the rolling minvmd.log. The `__krun-vmm` child inherited it and opened a second appender on the same file, interleaving with the supervisor's writes. Clear the marker on the child's command so it does not take the file-backed path; its diagnostics ride the hvc0 console into boot.log. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pull the appender creation, the reloadable tracing layer, and the shutdown release out of init_tracing/main/server and into one `DaemonLogger` (new `logging` module). The native detached daemon and the microVM pid-1 now share one path: install() sets up console + an inert reloadable file slot for both, activate() points it at the log directory once known (immediately for the native daemon, post-mount for the microVM), and both hand the resulting release to ServerState to run at shutdown. This removes the (WorkerGuard, LogActivator) bifurcation and the type_complexity allow. The file-log release (renamed VolumeLogRelease -> DaemonLogRelease) now runs on shutdown in both cases, not only when a volume is mounted; for the microVM it still precedes the quiesce so the appender's fd is gone before the unmount. ServerState owns the release value; DaemonLogger authors its logic. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@twitchyliquid64 both addressed in 6468f878 (merged): the daemon logging is now a |
Unit 3 of the diagnostics series (spec: #802, epic: #801; order-free with Units 1–2 — no dependency on the diagnostics crate).
What
The raw material
min bugcollects has to exist before the incident. After this PR both daemons persist rolling file logs, the VMM console lands somewhere by default, and lifecycle records correlate on span-carried ids instead of timestamps and guesswork.Refs: #801
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes