Allow directly writing snapshots on top of existing files #4301
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roypat
force-pushed
the
snapshot-overwrite
branch
from
dec7b3c to
b93cdcd
Compare
roypat
requested review from
xmarcalx,
kalyazin,
pb8o and
wearyzen
as code owners
roypat
force-pushed
the
snapshot-overwrite
branch
2 times, most recently
from
0cd7cb3 to
5343789
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4301 +/- ##
==========================================
- Coverage 81.71% 81.71% -0.01%
==========================================
Files 240 240
Lines 29332 29355 +23
==========================================
+ Hits 23970 23986 +16
- Misses 5362 5369 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
snapshot-overwrite
branch
from
5343789 to
829a39e
Compare
roypat
added
the
Status: Awaiting review
pb8o
reviewed
Dec 5, 2023
roypat
force-pushed
the
snapshot-overwrite
branch
from
972cf1f to
9fd359b
Compare
kalyazin
reviewed
Dec 6, 2023
Seems like these were missed in the big `this_error` refractoring. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Passing this enough around by reference was making for awkward dereferencing when comparing. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Previously, it would always use the same file names, which makes taking two snapshots from the same microvm slightly tedious (as the first snapshot has to be copied out before making the second snapshot). Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The Microvm.snapshot_*() functions by default always write the snapshot to the same file system location. This means that full_snapshot.mem and diff_snapshot.mem point to the same path, making the final assert in the test trivially true (as a file is the same as itself - the diff snapshot will have overwritten the full snapshot). The fix is to use a different path for the full snapshot. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
snapshot-overwrite
branch
2 times, most recently
from
4bf7d3f to
3baf0b2
Compare
We need to add fstat/newfstatat to the seccomp filter list due to the call to File::metadata() in snapshot_memory_to_file. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Allows the automatic merging of diff snapshots with a base snapshot by directly writing the diff snapshot on top of a base snapshot file of matching size. Signed-off-by: Patrick Roy <roypat@amazon.co.uk> Co-authored-by: Ives van Hoorne <ives@codesandbox.io>
roypat
force-pushed
the
snapshot-overwrite
branch
from
3baf0b2 to
37fde44
Compare
The test first crates a full snapshot, and then takes a diff snapshot with the same memory file, to merge it. We take both snapshots as "diff" snapshots, as taking a full snapshot does not clear the dirty pages tracked (and in this test, we want to exercise the path where the second diff snapshot only writes a few pages). Signed-off-by: Patrick Roy <roypat@amazon.co.uk> Co-authored-by: Ives van Hoorne <ives@codesandbox.io>
This commit adds a test that has a microvm overwrite the snapshot file from which it was loaded with a new snapshot (e.g. a sort of "refresh" operation on the snapshot to write any modifications done since loading back to disk). Prior to this patch series, this caused both the snapshot and the microvm to become corrupted, as the zeroing done by the truncate/set_len pair would be reflected in the mmap'd memory region. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
snapshot-overwrite
branch
from
37fde44 to
12fb4b1
Compare
kalyazin
approved these changes
Dec 7, 2023
pb8o
approved these changes
Dec 7, 2023
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebase of #3002 onto newest main.
Allows Firecracker to write snapshot files directly on top of existing file. This unlocks the ability to write diff snapshots directly on top of their base layer, merging them without having to go through rebase-snap. It additionally fixes a bug where having a VM try to overwrite the snapshot it was loaded from, both the snapshot and the VM got corrupted (now it instead does a "write back" of all memory changed since the snapshot was loaded, leaving the running VM intact)
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
CHANGELOG.md.TODOs link to an issue.rust-vmm.