Skip to content

feat(snapshot): allow block device path override on restore#5774

Open
meAmitPatil wants to merge 1 commit intofirecracker-microvm:mainfrom
superserve-ai:feat/snapshot-drive-overrides
Open

feat(snapshot): allow block device path override on restore#5774
meAmitPatil wants to merge 1 commit intofirecracker-microvm:mainfrom
superserve-ai:feat/snapshot-drive-overrides

Conversation

@meAmitPatil
Copy link
Copy Markdown

@meAmitPatil meAmitPatil commented Mar 19, 2026 •
edited
Loading

Allow overriding block device host paths when restoring from a snapshot. This is useful when disk paths are non-deterministic (e.g. container runtimes using devmapper) or when restoring on a different host where the backing file is at a different location.

Adds a drive_overrides parameter to the snapshot load API, following the same pattern as the existing network_overrides (#4731) and vsock_override (#5323).

Closes #4014

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.
  • This functionality cannot be added in rust-vmm.

@meAmitPatil meAmitPatil marked this pull request as ready for review March 19, 2026 19:12
@meAmitPatil meAmitPatil mentioned this pull request Mar 19, 2026
Open
3 tasks
@JamesC1305 JamesC1305 self-assigned this Mar 25, 2026
@JamesC1305
Copy link
Copy Markdown
Contributor

JamesC1305 commented Mar 25, 2026

Hi @meAmitPatil,

Thanks for the contribution. This is something we'd be interested in supporting, I'll take a look at it in the coming days. In the meantime, would you be able to add python integration tests to this? Something similar to those in #4731 and #5323 should work. Thank you!

@meAmitPatil
Copy link
Copy Markdown
Author

meAmitPatil commented Mar 29, 2026

Hi @meAmitPatil,

Thanks for the contribution. This is something we'd be interested in supporting, I'll take a look at it in the coming days. In the meantime, would you be able to add python integration tests to this? Something similar to those in #4731 and #5323 should work. Thank you!

@JamesC1305 Thanks for the reply I've updated the PR with Python integration tests.

JamesC1305
JamesC1305 reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@JamesC1305 JamesC1305 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall changes LGTM. A few very small nits, and one suggestion for improving the integration test coverage. The documentation is in a non-obvious place, but we'll plan to refactor it later and merge all the 'overrides' into their own docfile, so no need to worry about it for this PR.

Once you've responded to my feedback RE integ tests, I'll approve a run and we can go from there. Thanks!

src/vmm/src/devices/virtio/block/persist.rs Outdated
BlockState::Virtio(virtio_block_state) => virtio_block_state.virtio_state.activated,
BlockState::VhostUser(vhost_user_block_state) => false,
BlockState::Virtio(state) => state.virtio_state.activated,
BlockState::VhostUser(_state) => false,
Copy link
Copy Markdown
Contributor

@JamesC1305 JamesC1305 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be state rather than _state?

CHANGELOG.md Outdated

### Added

- [#4014](https://github.com/firecracker-microvm/firecracker/issues/4014): Add
Copy link
Copy Markdown
Contributor

@JamesC1305 JamesC1305 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this should probably link to the resolving PR rather than the issue.

tests/integration_tests/functional/test_snapshot_basic.py Outdated
Comment on lines +625 to +634
# The rootfs disk already exists in the snapshot; override its path
# to the same file (just proving the override mechanism works).
rootfs_path = str(snapshot.disks["rootfs"])
restored_vm.restore_from_snapshot(
snapshot,
drive_overrides=[
{"drive_id": "rootfs", "path_on_host": rootfs_path},
],
resume=True,
)
Copy link
Copy Markdown
Contributor

@JamesC1305 JamesC1305 Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expand on this? Maybe it would be good to use cp to make a copy of the rootfs with a similar name. That way we can verify that the override behaviour works with a different file (+ inode).

@meAmitPatil
Copy link
Copy Markdown
Author

meAmitPatil commented Mar 31, 2026

Overall changes LGTM. A few very small nits, and one suggestion for improving the integration test coverage. The documentation is in a non-obvious place, but we'll plan to refactor it later and merge all the 'overrides' into their own docfile, so no need to worry about it for this PR.

Once you've responded to my feedback RE integ tests, I'll approve a run and we can go from there. Thanks!

@JamesC1305 Thanks for the review!

  • Changed _state to _ (wildcard, since the value is unused in that arm)
  • CHANGELOG now links to this PR instead of the issue
  • Test now copies the rootfs to a separate file to verify the override with a different inode

@JamesC1305
Copy link
Copy Markdown
Contributor

JamesC1305 commented Mar 31, 2026

Hi @meAmitPatil,

I triggered a run of the integration tests on your PR, here. It seems like there are some issues with style, build and functional tests. Would you be able to address the errors please?

You can run the integration tests locally using tools/devtool test -- integration_tests/{style,build,functional}

Thanks,
James

@meAmitPatil meAmitPatil force-pushed the feat/snapshot-drive-overrides branch 2 times, most recently from 5664f60 to 83a278b Compare March 31, 2026 16:47
@meAmitPatil
feat(snapshot): allow block device path override on restore
29d2c6c 
Allow overriding block device host paths when restoring from a snapshot.
This is useful when disk paths are non-deterministic (e.g. container
runtimes using devmapper) or when restoring on a different host where
the backing file is at a different location.

Adds a drive_overrides parameter to the snapshot load API, following the
same pattern as the existing network_overrides and vsock_override.
Includes Python integration tests that verify the override works with a
different file and that an unknown drive_id is rejected.

Closes firecracker-microvm#4014

Signed-off-by: Amit Patil <iamitpatil2001@gmail.com>
@meAmitPatil meAmitPatil force-pushed the feat/snapshot-drive-overrides branch from 83a278b to 29d2c6c Compare March 31, 2026 16:57
@meAmitPatil
Copy link
Copy Markdown
Author

meAmitPatil commented Mar 31, 2026

@JamesC1305 I've squashed the commits, fixed the style issues (gitlint, markdown formatting), and fixed the integration_tests.rs:375 compile error and a jailer path issue in the integration test. All verified passing on a bare metal instance. Could you trigger a new CI run?
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesC1305 JamesC1305 JamesC1305 left review comments

@kalyazin kalyazin Awaiting requested review from kalyazin kalyazin is a code owner

@pb8o pb8o Awaiting requested review from pb8o pb8o is a code owner

@Manciukic Manciukic Awaiting requested review from Manciukic Manciukic is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@JamesC1305 JamesC1305

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Snaps] Drive mounts for block device disk paths during snapshot loading

2 participants

@meAmitPatil @JamesC1305