fix: reclaim unused encrypted data disk blocks - #1175
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new dstack.storage_discard= kernel cmdline option is parsed but effectively ignored due to unconditional override from app_compose, which is misleading and likely unintended.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Propagates data-disk discard/TRIM end-to-end (guest FS → dm-crypt → virtio-blk → QEMU) and exposes an attested storage_discard control across SDKs and the VMM UI, with documentation of the security trade-off.
Changes:
- Adds
storage_discardto app-compose types (Rust + SDK parity), VMM UI form/payload, and security docs. - Passes discard behavior into QEMU
-driveand guest disk setup (ZFS autotrim + one-time trim on upgrade; ext4mount -o discard; dm-crypt--allow-discards). - Updates changelog and adds/extends targeted tests (types parity + QEMU args).
File summaries
| File | Description |
|---|---|
| sdk/python/src/dstack_sdk/get_compose_hash.py | Adds storage_discard field to Python compose-hash type surface. |
| sdk/js/src/get-compose-hash.ts | Adds storage_discard to JS AppCompose interface with security note. |
| sdk/go/dstack/compose_hash.go | Adds StorageDiscard to Go AppCompose for compose-hash parity. |
| dstack/vmm/ui/src/composables/useVmManager.ts | Wires storage_discard into UI form state and compose payload; defaults to true. |
| dstack/vmm/ui/src/components/CreateVmDialog.ts | Adds UI checkbox + tooltip for reclaiming unused storage blocks. |
| dstack/vmm/src/app/qemu.rs | Adds `discard=unmap |
| dstack/dstack-util/src/system_setup.rs | Propagates discard through ZFS/ext4 and dm-crypt; starts one-time ZFS trim on upgrade if needed. |
| dstack/dstack-types/src/lib.rs | Adds attested storage_discard (default true) + tests and SDK-parity list update. |
| docs/security/cvm-boundaries.md | Documents storage_discard semantics and trade-off. |
| CHANGELOG.md | Records the end-to-end discard propagation and default/opt-out behavior. |
Review details
Suppressed comments (1)
dstack/dstack-util/src/system_setup.rs:2807
- Error messages should start with lowercase per CLAUDE.md; this newly introduced bail message starts with "Failed". Consider lowercasing it for consistency with the repo’s logging/error-message style.
let status = mount.arg(dev).arg(mount_point).status()?;
if !status.success() {
bail!("Failed to mount ext4 filesystem: {status}");
}
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed Copilot feedback and the Rust CI failure in a21d2ff: removed the ineffective cmdline option, inlined the ext4 mount branches for readability, fixed message casing, and initialized |
|
Real-CVM validation completed on a TDX-capable host using the mkosi-built development image from this branch ( Configuration:
Boot evidence: The provider independently logged: Guest discard-path evidence: End-to-end sparse-host-file allocation test:
This verifies that discard reaches the guest block device, passes through dm-crypt, is enabled in ZFS, and deallocates the backing sparse host file in a real local-key-provider-backed TDX CVM. |
|
Correction and stronger end-to-end validation: the previous test explicitly ran Test setup:
The guest confirmed: Workload: dd if=/dev/urandom of=/var/volatile/dstack/persistent/autotrim-e2e.bin bs=4M count=512
sync
sleep 60
rm /var/volatile/dstack/persistent/autotrim-e2e.bin
sync
# No zpool trim, fstrim, or other trim command.Host sparse-file allocation observations: Therefore, ordinary guest file deletion plus |
Summary
storage_discard: falseopt-out for allocation-pattern-sensitive workloadsTesting
cargo check -p dstack-types -p dstack-util -p dstack-vmmcargo test -p dstack-types storage_discard_defaults_on_and_can_be_disabledcargo test -p dstack-vmm qemu_command_builder_does_not_require_prepared_paths_to_existnpm run build(dstack/vmm/ui)PYTHONPATH=src python3 -m pytest -q tests/test_get_compose_hash.pygo test ./dstack -run '^$'npm run test:ci; integration tests requiring/var/run/dstack.sockremain unavailable in this environment