fix(pool): discard warm firecracker entries whose process died while parked - #181
fix(pool): discard warm firecracker entries whose process died while parked#181epicvinny wants to merge 3 commits into
Conversation
…parked Parked warm processes run with oom_score_adj=1000, so they are the first OOM-kill candidates and can exit while idle in the pool. The acquire path handed whatever it popped straight to snapshot resume, which then failed on the dead process. Add FirecrackerInstance::is_process_running() and make try_acquire pop until a live entry is found. Dead entries skip the graceful-stop path (the process is already gone): the instance is dropped and the network slot is released synchronously, keeping the acquire path free of runtime block_on calls so it stays safe in async context.
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
FirecrackerInstance::is_process_running collapsed every try_wait() I/O error into "process not running", so the pool would tear down an entry (and release its network slot) on an inconclusive probe. Return std::io::Result<bool> instead; on Err the pool logs the failure, returns the entry to the pool keeping its slot, and reports a regular miss. try_acquire() also ran full network teardown for dead entries inline, from the async snapshot-resume path; cleanup_allocated_slot does blocking netlink/ip work that can stall a runtime worker thread. Dead entries now go onto a dead_entries queue drained by the maintenance worker at the start of each cycle, next to the teardown it already owns. Shutdown paths drain the queue too, and with maintenance disabled the cleanup falls back to inline execution since no worker exists.
…failures Address follow-up review on the deferred dead-entry cleanup: - The maintenance-disabled fallback ran blocking teardown inline on the async acquire path; it now spawns a detached cleanup thread instead. - Enqueue was not coordinated with shutdown: an acquire racing drain_all could queue an entry after shutdown had already drained the queue, leaving it without a consumer. The queue now carries a closed flag checked under the same lock; shutdown closes it right after drain_all and late enqueues clean up inline. - cleanup_dead_warm_entries took every entry out of the queue before cleanup and only logged failures, losing track of stale host network state while the slot index was already back in the allocation bitmap. Failed entries are now retained in the queue and retried on later cycles. Retries go through the new NetworkManager::cleanup_slot_resources which redoes only the resource teardown: the allocation bit is released on the first attempt and must never be released twice, since the index may have been reallocated to a live sandbox. cleanup_allocated_slot now borrows the Slot so failed teardown can keep the entry alive for retry; Slot::drop remains the last-resort cleanup.
| let result = if is_retry { | ||
| network.cleanup_slot_resources(&slot, false) | ||
| } else { | ||
| network.cleanup_allocated_slot(&slot, false) | ||
| }; |
There was a problem hiding this comment.
A failed first cleanup releases the allocation bit, but this retained SlotOnly can later retry teardown after the same index has been reallocated. Slot::cleanup operates on index-derived resources (veth-<idx> and the same namespace path), so the retry can delete networking that now belongs to a live sandbox. Not touching the bitmap does not make the resource teardown safe. Keep the slot allocated until teardown completes, or add an ownership/generation mechanism that proves the resources still belong to this entry before retrying.
| if let Err(err) = std::thread::Builder::new() | ||
| .name("firecracker-pool-dead-cleanup".to_string()) | ||
| .spawn(move || { | ||
| if let Err(err) = entry.attempt_cleanup() { |
There was a problem hiding this comment.
This detached thread is not tracked or joined by shutdown, so shutdown can report completion while slot teardown is still running and any cleanup failure cannot be included in its result. Also, if spawning fails, the captured closure (and entry) is dropped on the acquire thread; Slot::drop then performs synchronous cleanup, contradicting the requirement not to block the async path. Use a managed cleanup worker/queue with bounded concurrency whose handle participates in shutdown, and define a non-blocking fallback for spawn failure.
| self.cleanup_dead_warm_entries()?; | ||
|
|
||
| match self.pool.compute_maintenance_action(self.pool.len()) { |
There was a problem hiding this comment.
Because failed entries are deliberately retained, propagating this error prevents every later maintenance action from running. When the dead entry has depleted the pool, WarmPool sees an outstanding Fill action and immediately loops, repeatedly retrying the same failing cleanup without delay while never refilling the pool. Treat dead-entry cleanup and fill/drain as independent work (aggregate/report both errors after attempting maintenance), and consider backoff for retained cleanup failures.
1 participant
What
FirecrackerPool::try_acquirenow skips warm entries whose Firecracker process has already exited, discarding them (and releasing their network slot) instead of handing them to snapshot resume.Why
Parked warm processes run with
oom_score_adj=1000, so they are the first OOM-kill candidates and can die while idle in the pool. The acquire path popped whatever was parked and passed it straight to snapshot resume, which then failed on the dead process. Under host memory pressure this shows up as spurious sandbox-creation failures.Related issue
N/A. Small focused fix submitted directly per the contributing guide.
Scope and non-goals
Design and behavior changes
FirecrackerInstance::is_process_running()based ontry_wait.try_acquirepops until it finds a live entry. Dead entries are logged and discarded.block_on, sotry_acquirestays safe to call from async context.Compatibility and operations
Validation
make fmt(viacargo fmt --check)make clippy(viacargo clippy -p warm-pool; this change touchessrc/sandbox/firecracker/)make test-unit(scoped:cargo test -p warm-pool, 16 passed)make -C services test(required whenservices/changes)maketargetCommands and results:
Skipped checks and reasons: integration tests require root and
/dev/kvm, not available on the Windows dev box where this was written. Happy to run them on a Linux host if reviewers want.Risks and reviewer notes
src/sandbox/firecracker/pool.rs(acquire_live_warm,discard_dead_warm) andinstance.rs(is_process_running).Checklist