fix(sandbox): clean up empty serial logs and expire inactive logs - #265
fix(sandbox): clean up empty serial logs and expire inactive logs#265yingdi-shan wants to merge 1 commit into
Conversation
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| lock_file(&file, libc::LOCK_SH) | ||
| .with_context(|| format!("lock firecracker log {}", path.display()))?; |
There was a problem hiding this comment.
This performs a blocking flock(LOCK_SH) inside the async launch path. If another process holds an exclusive lock (especially plausible for caller-supplied stdout_path/stderr_path files), spawn_with_netns() can block a Tokio worker indefinitely and cannot be cancelled or timed out. Use LOCK_NB and return/retry asynchronously, and consider applying managed-log locking only to files under the managed serial directory.
| if let Err(err) = clean_directory(&self.path, None, SystemTime::now()) { | ||
| warn!(path = %self.path.display(), error = %err, "failed to remove empty serial logs"); | ||
| } |
There was a problem hiding this comment.
Immediate cleanup assumes every child-owned log descriptor carries a shared lock, but firecracker.log is opened by Firecracker itself via set_logger() and never passes through open_log_stdio(). On an abnormal sandbox drop, FirecrackerInstance::drop only calls start_kill() without reaping; with explicit stdout/stderr overrides, this managed directory may contain only the unlocked logger file, so this call can unlink an empty firecracker.log while the process is still exiting and discard late diagnostics. Defer cleanup to the background sweep (which checks /proc), or ensure the process is reaped/the logger is otherwise protected before cleaning.
| if *len == 0 || expired { | ||
| fs::remove_file(file_path)?; | ||
| removed += 1; | ||
| } |
There was a problem hiding this comment.
The directory inode was validated and locked, but this deletion resolves file_path again from the namespace. A process able to rename/replace the sandbox directory after validation can redirect cleanup to a different directory (or symlink traversal through an replaced ancestor), so the files unlinked need not be the files that were opened and locked. Perform file validation and deletion relative to the locked directory FD (for example, openat2/unlinkat with no-symlink resolution), and similarly remove the sandbox directory relative to a locked root FD.
| for process in fs::read_dir("/proc")? { | ||
| let process = process?; |
There was a problem hiding this comment.
Every 60-second sweep walks every visible process and all of its descriptors, even when the serial root has no candidate directories. On hosts with high PID/FD counts this creates substantial recurring /proc work, and shutdown() must await this spawn_blocking scan because cancellation is not checked anywhere in open_log_directories. First determine whether candidate log directories exist, and make the /proc inventory cancellable (checking between processes/descriptors), or replace it with a bounded per-candidate check.
| Err(err) | ||
| if matches!( | ||
| err.kind(), | ||
| io::ErrorKind::NotFound | io::ErrorKind::PermissionDenied | ||
| ) => | ||
| { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Treating PermissionDenied as “not active” makes the legacy compatibility safeguard fail open: if /proc/<pid>/fd cannot be inspected (for example due to procfs/LSM restrictions), a still-running pre-upgrade Firecracker process is omitted from active, and an expired log can be unlinked while that process is still writing it. Preserve cleanup safety by aborting/skipping expiry for the sweep when descriptor visibility is incomplete, rather than interpreting inaccessible processes as inactive.
4356426 to
431a41a
Compare
| Ok(status) => { | ||
| status.context("wait for firecracker exit")?; | ||
| } |
There was a problem hiding this comment.
Child::wait() returns Result<ExitStatus>, so in this match arm status is an ExitStatus, not a Result/Option; it has no context method and this change does not compile. Preserve the Result context before matching (or simply treat the Ok(status) value as the exit status and handle it explicitly if nonzero exits are meant to be errors).
Suggestion:
| Ok(status) => { | |
| status.context("wait for firecracker exit")?; | |
| } | |
| Ok(_status) => {} |
| Ok(status) => { | ||
| status.context("wait for firecracker exit")?; | ||
| } |
There was a problem hiding this comment.
Unlike the previous implementation, this propagates a non-success ExitStatus from a normal wait. If Firecracker exits nonzero during teardown (for example after a startup failure or signal), stop returns here before self.process.take() and before removing the socket. FirecrackerSandbox::stop consequently also returns before dropping serial_log_dir and releasing the remaining resources. Please ensure process/socket and sandbox cleanup happen even when the exit status is unsuccessful, while still preserving the status as diagnostic information if desired.
Suggestion:
| Ok(status) => { | |
| status.context("wait for firecracker exit")?; | |
| } | |
| Ok(status) => { | |
| let _ = status.context("wait for firecracker exit")?; | |
| } |
| lock_file(&file, libc::LOCK_SH) | ||
| .with_context(|| format!("lock firecracker log {}", path.display()))?; |
There was a problem hiding this comment.
LOCK_SH is a blocking flock, and this synchronous helper runs inside the async launch path. If the configured log file (especially an explicit custom stdout/stderr path, which has no managed-directory coordination) is held with an exclusive lock by another process, sandbox startup can block a Tokio worker indefinitely and cannot be cancelled. There is no expected legitimate contention here because managed launches coordinate through the directory lock, so use LOCK_SH | LOCK_NB and return a contextual WouldBlock error instead.
| launch: LaunchMode, | ||
| work_dir: TempDir, | ||
| fc_instance: FirecrackerInstance, | ||
| serial_log_dir: Option<SerialLogDir>, |
There was a problem hiding this comment.
This guard is dropped after fc_instance during abnormal FirecrackerSandbox destruction, but FirecrackerInstance::drop only calls start_kill() and does not wait for the child to exit. The stdout/stderr descriptors carry file locks, whereas firecracker.log is opened directly by Firecracker and carries no lock. Thus, when custom stdout/stderr paths are configured and Firecracker logging is enabled, SerialLogDir::drop can see an empty firecracker.log, unlink it, and remove the directory while the child is still alive; late diagnostics then go to a deleted inode. Ensure the process is synchronously reaped before releasing this guard, or provide an inherited/cooperating lock for the Firecracker logger path and test this abnormal-drop case.
| let active = open_log_directories(root)?; | ||
| let now = SystemTime::now(); | ||
| let mut removed = 0; | ||
| for entry in entries { |
There was a problem hiding this comment.
This one-time /proc snapshot does not actually make legacy-process cleanup race-safe. During a rolling upgrade, an older server can launch Firecracker and open an initially empty log directory after open_log_directories returns but before clean_directory handles that entry. Because that older process holds none of the new advisory locks, cleanup can unlink its open empty logs and directory, causing subsequent output to be written only to deleted inodes. Avoid pruning unmarked legacy directories while old launchers may coexist, or introduce a protocol/marker that lets cleanup distinguish safely lock-protected directories; a one-time descriptor inventory cannot close this check-then-act race.
| let entry = entry?; | ||
| if !entry.file_type()?.is_dir() |
There was a problem hiding this comment.
A normal concurrent sandbox stop can remove an empty directory after read_dir returned its entry but before entry.file_type() runs. The resulting NotFound is propagated by ?, aborting the entire sweep and skipping retention cleanup for every later directory. This race is likely under launch/stop churn. Treat NotFound from entry inspection as continue (as already done for disappearing procfs entries), and similarly avoid letting one vanished root entry terminate the sweep.
| for process in fs::read_dir("/proc")? { | ||
| let process = process?; |
There was a problem hiding this comment.
This maintenance pass enumerates every host process and then every file descriptor under each process once per minute. Since SerialLogCleanup::start is unconditionally started for normal servers with a configured serial directory, this creates system-wide O(processes × FDs) synchronous I/O even when there are no abandoned logs. Consider tracking managed directories/owners or only invoking a narrowly scoped fallback scan when needed; otherwise this can add substantial recurring host overhead.
Suggestion:
| for process in fs::read_dir("/proc")? { | |
| let process = process?; | |
| // Avoid a system-wide /proc/<pid>/fd scan on every maintenance interval; use | |
| // an application-maintained inventory and reserve proc inspection for recovery. |
| if let Some(task) = self.task.take() { | ||
| let _ = task.await; | ||
| } |
There was a problem hiding this comment.
Shutdown awaits the cleanup task without a timeout, while the task is currently inside a blocking /proc and filesystem sweep (spawn_blocking). If procfs or the serial-log filesystem stalls, server termination can wait indefinitely for maintenance that is not required for correctness. Consider bounding this wait (and aborting/detaching the task if the bound expires), or otherwise making the sweep cancellation/timeout-aware.
Suggestion:
| if let Some(task) = self.task.take() { | |
| let _ = task.await; | |
| } | |
| if let Some(task) = self.task.take() { | |
| let _ = tokio::time::timeout(CLEANUP_INTERVAL, task).await; | |
| } |
431a41a to
6b14029
Compare
| fs::create_dir_all(&path)?; | ||
| let directory = match open_directory(&path) { |
There was a problem hiding this comment.
acquire runs synchronous directory creation/open/metadata/removal operations directly on a Tokio worker, and finish similarly invokes synchronous cleanup from the async sandbox stop path. A slow configured serial filesystem can therefore stall unrelated async work. Move each filesystem attempt/finish cleanup to spawn_blocking (while retaining the async retry/backoff outside it).
| match lock_file(&directory, libc::LOCK_SH | libc::LOCK_NB) { | ||
| Err(err) if err.kind() == io::ErrorKind::WouldBlock => { | ||
| tokio::time::sleep(Duration::from_millis(1)).await; | ||
| continue; | ||
| } | ||
| result => result?, | ||
| } |
There was a problem hiding this comment.
The shared lock permits multiple active SerialLogDir guards for the same sandbox directory. If guard A calls finish(), it leaves .inactive; if guard B then drops abnormally, that marker remains and a later sweep can expire the logs even though B never confirmed process exit (and its child may still be shutting down). This contradicts the retention invariant and is possible across processes/restarts sharing a serial root. Use an exclusive lock for the active guard so a sandbox ID has a single lifecycle owner, or track active owners so .inactive is created only when the last one finishes normally.
Suggestion:
| match lock_file(&directory, libc::LOCK_SH | libc::LOCK_NB) { | |
| Err(err) if err.kind() == io::ErrorKind::WouldBlock => { | |
| tokio::time::sleep(Duration::from_millis(1)).await; | |
| continue; | |
| } | |
| result => result?, | |
| } | |
| match lock_file(&directory, libc::LOCK_EX | libc::LOCK_NB) { | |
| Err(err) if err.kind() == io::ErrorKind::WouldBlock => { | |
| tokio::time::sleep(Duration::from_millis(1)).await; | |
| continue; | |
| } | |
| result => result?, | |
| } |
|
|
||
| /// Call only after the process has exited and closed every log descriptor. | ||
| pub(super) fn finish(self) -> io::Result<()> { | ||
| File::create(self.path.join(INACTIVE_MARKER))?; |
There was a problem hiding this comment.
Creating the marker this way follows an existing .inactive symlink. If the managed directory is writable by another local principal (or the entry is replaced during the process lifetime), shutdown can truncate an arbitrary file reachable by the service account. Create the marker with no-follow/exclusive semantics (ideally relative to the already-open directory fd) and treat an unexpected existing entry as an error.
| if let Some(task) = self.task.take() { | ||
| let _ = task.await; | ||
| } |
There was a problem hiding this comment.
This await is unbounded while the worker may be waiting on spawn_blocking(sweep). The cancellation flag is only checked between directory entries, so one stalled filesystem operation can prevent the rest of server shutdown indefinitely. Bound this join with a shutdown timeout (and abort the async task on expiry), as is already done for other background runtimes.
| let serial_dir = ConfigManager::global_config() | ||
| .firecracker | ||
| .serial_dir | ||
| .as_ref() | ||
| .expect("managed serial directory") | ||
| .join(sandbox_id.to_string()); |
There was a problem hiding this comment.
This makes the integration test fail under the repository's default configuration: firecracker.serial_dir is optional and remains None because config/default.toml only shows it as a commented example, while common::setup() does not configure it. The lifecycle test previously supported that default. Either arrange an isolated managed serial directory before global config initialization, or conditionally run this retained-log assertion when serial logging is configured.
|
The failure mode in #260 isn't cumulative growth per se — it's the cost of inserting into and looking up in a huge parent directory, paid by every create. There are three paths to the same degraded state intact:
Other notes worth mentioning:
I think the cleanup can be a nice addition to #261, which fixes the issue, not masking the symptoms |
|
Thanks for the detailed feedback. Our production workload generally uses long-lived sandboxes, so the creation rate in #260 is not representative of our primary use case. Our preferred direction is to delete obsolete logs and reclaim their directories so historical sandbox count does not permanently affect the node. Sharding would distribute the entries, but the log tree would still grow without bound, so we do not see it as the ultimate solution. Another solution is to combine logs from different sandboxes into a single log, which makes it easier to inspect serial output while also improving performance and simplifying log size control. |
2 participants
What
Remove empty managed Firecracker logs after confirmed process exit and expire inactive nonempty logs after a configurable retention period. Keep the existing
{serial_dir}/{sandbox_id}/layout.Why
Silent snapshot launches leave permanent directories and empty log files behind, growing the serial root with every sandbox created. Cleaning stopped sandboxes prevents this accumulation during normal operation.
Related issue
Closes #260. Alternative to #261.
Scope and non-goals
Includes cleanup after stop, retention, live-writer protection, documentation, and regression tests. No sharding, lazy capture, active-log rotation, or throughput improvement claim. Legacy directories and logs left after an abnormal drop or server crash require offline cleanup.
Design and behavior changes
.inactivemarker under the lock and remove empty known logs. Resuming an ID clears the marker before opening logs. Abnormal drop leaves logs unmarked, preserving late internal-logger output even with custom stdout/stderr paths.firecracker.serial_log_retention_secs, default 604800 (seven days). Zero disables expiry. Retention uses the newest directory/log modification time, refreshed after confirmed exit.Compatibility and operations
firecracker.serial_log_retention_secsandAENV_FIRECRACKER_SERIAL_LOG_RETENTION_SECS, default seven days..inactivemarker after stop.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
All builds and Rust tests run as the non-root workspace user. Test state and dependencies are isolated under
/tmp/aenv-serial-retention-tests; privileged tests use the repository capability runner.Skipped checks and reasons:
Risks and reviewer notes
The main review area is
src/sandbox/firecracker/serial_logs.rs: eligibility after confirmed exit, directory locking, and retention. Regression coverage includes late unlocked logger writes after abnormal drop, interrupted resume, cancellable lock contention, collection during repeated launches/stops, symlinks/custom entries, startup failure, retained history, and warm-log relocation.Unmarked logs need offline cleanup. Retention bounds eligible inactive-log lifetime; it does not limit active-log size, peak directory count, or compact an already expanded filesystem directory inode. Filesystem syscalls themselves cannot be interrupted by worker cancellation.
Checklist