Repository navigation
Conversation
Signed-off-by: Leechael Yim <yanleech@gmail.com>
4e73819 to
109599d
Compare
Signed-off-by: Leechael Yim <yanleech@gmail.com>
kvinwang
left a comment
There was a problem hiding this comment.
The core fix is right: tokio::spawn doesn't move the synchronous remove_all() off the executor, and unlinking a preallocated qcow2 can take seconds in the kernel. But most of the +445 lines are tests that don't test this change, and the production stall isn't fully explained yet. I'd like to see this trimmed before merging.
1. The cancellation scenario doesn't happen in production
finish_remove_vm is only reached through tokio::spawn (remove_vm, spawn_finish_remove). No request future awaits it, so the only way it gets cancelled is runtime shutdown. start_vm is different: its future belongs to the RPC request, which is why the launch lock has to travel into config_qemu's blocking task. Here, the minimal fix is enough:
if let Err(err) = tokio::task::spawn_blocking(move || vm_path.remove_all()).await? { ... }and the rest of the function stays as it is. If you want to keep the guards in the closure defensively, the comment should say that, not describe a race that can't occur. cancelling_vm_deletion_keeps_guards_and_finishes_cleanup tests that non-existent scenario.
2. Test-only hook on App
The #[cfg(test)] before_remove_workdir: Option<Arc<dyn Fn() -> Result<()> + Send + Sync>> field and the remove_vm_workdir wrapper exist only for injection. I'd prefer not to put test seams into the production struct for this.
3. Test scope
Only slow_vm_deletion_does_not_block_status tests this change, and even it could lose about half of its assertions. The others cover behavior this PR doesn't change: unmarked orphan retention, failed-deletion marker recovery, the malformed .removing marker, delete_image name and not-found validation, and the create_vm load-failure cleanup. Some also assert internals: launch_lock_handle().try_lock(), cid_pool.occupy(), and the panic/JoinError path. That makes them brittle under later refactors. Please drop them or move them to a separate PR.
4. create_vm failure-path offload is unnecessary
When load_vm fails, the workdir only holds the manifest and shared config. The disk is created later, in config_qemu. That remove_dir_all is effectively instant, so the spawn_blocking plus the three-arm match only adds code. The delete_image offload makes sense, since image dirs can be large.
5. The production stall is still unexplained
As the description says, one blocked worker out of 128 shouldn't delay several Status requests if work-stealing were in play. One plausible explanation (unverified): tokio's per-worker LIFO slot can't be stolen (tokio-rs/tokio#4941). A task woken on the blocked worker, such as a hyper connection task, waits until that worker returns. That matches "Status requests were dispatched immediately after the deletion finished". If that's the cause, any synchronous fs work on the async path in VMM can reproduce the stall, not just the deletion. That deserves a follow-up audit, or a look at disable_lifo_slot (still unstable), more than additional tests here.
Suggested shape: offload only remove_all (and delete_image), drop the create_vm change and the test hook, keep at most one regression test, and track the LIFO-slot question separately.
Why
RemoveVmreturns quickly, but its background task calls synchronous filesystem deletion on a Tokio worker.tokio::spawndoes not move that work off the async executor. Unlinking a fully allocated qcow2 disk can spend seconds freeing filesystem extents.Production sampling caught a Rocket worker spending about 3.7 seconds in kernel CPU during one removal. Waiting Status requests were dispatched immediately after the workdir deletion finished. The same timing pattern appeared across multiple removals.
Change
Run workdir deletion in
spawn_blocking, after the VM exits and netd confirms interface release.The blocking task also owns the per-VM lock, the existing removal guard, and final state/CID cleanup. This matters because cancelling the async waiter does not cancel an already-running blocking task. Offloading only the filesystem call would let cancellation release the guards while deletion continues, or leave a deleted VM occupying its CID.
Apply the same offload to failed-create cleanup and image deletion. The existing
removingstate and.removingrecovery mechanism are unchanged; this PR adds no VM state or RPC changes.Tests
cargo test --locked -p dstack-vmm: 224 passed, 0 failed, 3 ignored. The ignored tests require root and a network namespace.The async-progress test covers VM removal. The two other deletion paths have result/error tests, not async-progress tests.
This removes a confirmed source of executor blocking. It does not establish why one blocked worker delayed multiple requests on the production 128-worker runtime, or explain stalls without a removal.