feat(snapshot): allow block device path override on restore#5774
feat(snapshot): allow block device path override on restore#5774meAmitPatil wants to merge 1 commit intofirecracker-microvm:mainfrom
Conversation
|
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
left a comment
JamesC1305
left a comment
There was a problem hiding this comment.
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!
| 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, |
There was a problem hiding this comment.
Should this be state rather than _state?
CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - [#4014](https://github.com/firecracker-microvm/firecracker/issues/4014): Add |
There was a problem hiding this comment.
nit: this should probably link to the resolving PR rather than the issue.
| # 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, | ||
| ) |
There was a problem hiding this comment.
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).
@JamesC1305 Thanks for the review!
|
|
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 Thanks, |
meAmitPatil
force-pushed
the
feat/snapshot-drive-overrides
branch
2 times, most recently
from
5664f60 to
83a278b
Compare
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
force-pushed
the
feat/snapshot-drive-overrides
branch
from
83a278b to
29d2c6c
Compare
|
@JamesC1305 I've squashed the commits, fixed the style issues (gitlint, markdown formatting), and fixed the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5774 +/- ##
==========================================
- Coverage 83.04% 82.97% -0.08%
==========================================
Files 276 276
Lines 29466 29494 +28
==========================================
+ Hits 24471 24473 +2
- Misses 4995 5021 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Changes all LGTM. Only thing is the patch coverage is slightly low. Would you be able to extend the unit tests a little bit to increase the coverage and make codecov happy? Seems like the biggest gap is in the newly introduced |
2 participants
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_overridesparameter to the snapshot load API, following the same pattern as the existingnetwork_overrides(#4731) andvsock_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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.