feat(snapshot): allow pmem backing file path override on restore#5896
feat(snapshot): allow pmem backing file path override on restore#5896MohibShaikh wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Please upload reports for the commit 9c67102 to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #5896 +/- ##
==========================================
- Coverage 83.03% 82.86% -0.18%
==========================================
Files 277 277
Lines 30204 30076 -128
==========================================
- Hits 25079 24921 -158
- Misses 5125 5155 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Allows for changing the backing host file of a virtio-pmem device | ||
| during snapshot restore. | ||
| required: | ||
| - pmem_id |
There was a problem hiding this comment.
nit: perhaps we should use id for uniformity with the rest of the pmem API.
|
Hi @MohibShaikh, Thanks for the PR. Overall it looks in pretty good shape to me. I've left one minor nit regarding parameter names. I'm going to checkout the code and just perform a sanity check to confirm that it works as intended, and double check the unit/integ tests are reasonable. I'll get back to you soon edit: I forgot to mention that the buildkite run failed: https://buildkite.com/firecracker/firecracker-pr/builds/17149/canvas Can you please fix the style issue, and the integration test issue? It looks to be related to permissions on the pmem backing file. When you think it's ready, can you please run Thanks :) |
MohibShaikh
force-pushed
the
feat/pmem-snapshot-override
branch
from
e53e035 to
af64c99
Compare
|
Thanks for the review, @JamesC1305!
Re-ran |
JamesC1305
left a comment
JamesC1305
left a comment
There was a problem hiding this comment.
A few small things, mostly regarding documentation and testing.
| /// Unknown Vsock Device. | ||
| UnknownVsockDevice, | ||
| /// Unknown Pmem Device. | ||
| UnknownPmemDevice, |
There was a problem hiding this comment.
Can we convert this to provide information about the unknown device? I.e. something like:
// Unknown Pmem device: {0}
UnknownPmemDevice(String)
(We have similar feedback for the currently open block device PR)
| each `virtio-pmem` device at a different host file. The new file must be at | ||
| least as large as the backing file used when the snapshot was taken; using a | ||
| smaller file will result in the device failing to restore. |
There was a problem hiding this comment.
The new file must be at
least as large as the backing file used when the snapshot was taken; using a
smaller file will result in the device failing to restore.
I think this misstates the purpose of path-renaming. Path renaming is only intended to be used for when the same pmem file is at a different location on the host. It is not intended for arbitrarily swapping out the backing file on snapshot restore. The backing file should therefore be the same size as the pre-snapshot version.
| # Hardlink the override into the restored VM's chroot and chown it to the | ||
| # jailer's uid:gid so Firecracker can open it. | ||
| override_jailed = restored_vm.create_jailed_resource(override_src) | ||
|
|
There was a problem hiding this comment.
We should have a similar modification to microvm.py as the Block Override PR, where, when pmem_overrides is not None, the original pmem file is not copied to the restored VM's jail. Combined with using shutil.move (rather than copy), we can ensure that if restoring from the overridden path didn't work, the test should fail.
There was a problem hiding this comment.
Our test framework has changed since this PR was opened, and fixtures such as uvm_plain_any no longer exist. Could you please update your tests accordingly? More information about the new uvm fixture and dimension pinning are available in tests/README.md.
| .map(|state| state.config.path_on_host.clone_from(&entry.path_on_host)) | ||
| .ok_or(SnapshotStateFromFileError::UnknownPmemDevice)?; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is it maybe worth having small tests to ensure the 'happy path' works as intended, and also that an error is returned for bad conditions? This is the crux of the change
Allow overriding virtio-pmem device backing file paths when restoring from a snapshot. This is useful when the host file path baked into the snapshot is no longer valid, for example when restoring on a different host or under a different jailer chroot. Add a pmem_overrides parameter to the snapshot load API, following the same pattern as the existing network_overrides and vsock_override mechanisms. Each entry identifies a pmem device by its id and provides a new path_on_host. Both MMIO and PCI transports are covered. An override targeting an unknown id fails the restore with a clear error. Part of firecracker-microvm#4992. Signed-off-by: MohibShaikh <mohibuddin9@gmail.com>
MohibShaikh
force-pushed
the
feat/pmem-snapshot-override
branch
from
af64c99 to
9c67102
Compare
|
Thanks for the detailed review @JamesC1305! I've pushed an update addressing all the points. The branch was also rebased onto latest
Locally I've run |
2 participants
Allow overriding virtio-pmem device backing file paths when restoring from a snapshot. This is useful when the host path baked into the snapshot is no longer valid, for example when restoring on a different host or under a different jailer chroot.
Adds a
pmem_overridesparameter to the snapshot load API, following the same pattern as the existingnetwork_overrides(#4731) andvsock_override(#5323). Each entry identifies a pmem device by itspmem_idand supplies a newpath_on_host. Both MMIO and PCI transports are covered. Overrides targeting an unknownpmem_idfail the restore with a clear error.Part of #4992.
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 passes build checks on all supported architectures.tools/devtool checkstyleto verify that the PR passes the automated style checks.CHANGELOG.md.TODO.rust-vmm.