Allow directly writing snapshots on top of existing files #3002

CompuIves · 2022-05-25T11:57:36Z

Reason for This PR

We're exploring diff snapshots, but found out that our underlying filesystem (btrfs) doesn't perform well with the rebase-snap tool due to performance issues with finding holes in files (related: #2948). Initially we tried using another syscall (fiemap), but that also was too slow (10s for 4GB). Because of this, we cannot apply diff snapshots quickly on top of a base snapshot.

Instead, we're now taking another approach where we directly save the diff snapshot on top of the base memory. Firecracker already supports this, because it only writes changed blocks. This has worked out well for us, and this might be interesting to merge into upstream.

There's no issue opened for this right now, and it might well be that this change cannot be merged into Firecracker, but if you're interested I'd be happy to make the required changes to make it fit for the repo!

Description of Changes

Instead of truncating the memory file we write to, we write on top of it.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The issue which led to this PR has a clear conclusion.
This PR follows the solution outlined in the related issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes follow the Runbook for Firecracker API changes.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

acatangiu · 2022-05-25T12:29:40Z

My 2c is this is a valid use-case that firecracker could support in addition to existing behavior of creating new snapshot containing just the diff bits.

There are two different use-cases here:

(existing) the user is interested in the diff pages of memory, the diff snapshot - for example, multiple layers of "diffs" can be used in a distributed system to build snapshots at varying stages in the VM's lifetime, while minimizing the amount of "data" to distribute across the system.
(what this PR proposes) the user is interested in a single snapshot instance, but wants to accelerate mutations to it - take snapshots as the guest workload makes progress (but only interested in latest snapshot), so instead of creating slow(er) full snapshots, directly create fast(er) diff snapshots on top of each other.

I think there's value in both use-cases, and the proposed changes in this PR seem to support both of them 👍

CompuIves · 2022-06-01T10:19:34Z

I think there's value in both use-cases, and the proposed changes in this PR seem to support both of them

Yep, that's right. This would allow both for creating diff snapshots and saving snapshots directly on top of a base snapshot. The only breaking change that you could consider is that if people have been saving their snapshot on top of existing files, we don't truncate those files anymore, instead we write the changed pages on top of the file. That said, I don't expect that people are writing snapshots on top of existing files at the moment.

CompuIves · 2022-06-01T10:44:50Z

It seems like the tests are failing on test coverage, should I change a value here?

luminitavoicu · 2022-06-06T14:02:08Z

@CompuIves, thanks for your contribution! Your changes increase ARM coverage. Please rebase your PR and update the coverage value for the failing tests.

luminitavoicu

Looks good, I like the idea! Could you add an integration test for this, as well?

luminitavoicu · 2022-06-06T14:04:08Z

src/vmm/src/persist.rs

    let mem_size_mib = mem_size_mib(vmm.guest_memory());
-    file.set_len((mem_size_mib * 1024 * 1024) as u64)
-        .map_err(|e| MemoryBackingFile("set_length", e))?;
+    let expected_size = (mem_size_mib * 1024 * 1024) as u64;


Not necessarily in scope of this PR, but I would like us to have a constant for 1024 * 1024.

CompuIves · 2022-06-15T12:55:33Z

Rebased the PR, but I'm not sure what the code coverage is now with the changes that came from the rebase. Should I wait for a test run?

CompuIves · 2022-06-15T13:31:39Z

Added integration test as well!

luminitavoicu

Looks good! Please make sure that all of your commits are signed.
The coverage test is failing, you will have to update the numbers once again. Also take a look at the errors output from the python style test.

tests/integration_tests/functional/test_snapshot_basic.py

luminitavoicu · 2022-06-16T10:39:03Z

tests/integration_tests/functional/test_snapshot_basic.py

+    ssh_connection = net_tools.SSHConnection(microvm.ssh_config)
+
+    # Run command to do something
+    exit_code, _, _ = ssh_connection.execute_command("sync")


It would be nice to also run a workload inside the guest to dirty some pages.

Signed-off-by: Ives van Hoorne <ives@codesandbox.io>

CompuIves · 2022-06-21T10:03:57Z

Addressed the feedback, squashed the commits, signed it and updated coverage! I think the tests should now pass, though I did see other perf tests fail, so that's something we should check.

dianpopa · 2023-03-16T12:39:32Z

Hi!

I do like the proposal! However, I am concerned about the possibility of a breaking change since I do expect that users of the rebase-snap tool to have the expectation to obtain diff layers containing dirty memory since last retrieval.
Shouldn't we let rebase-snap tool users know that from now in order to obtain a diff layer they need to make sure to remove any previous (name conflicting) snapshots?

roypat · 2023-07-25T10:05:47Z

Hi!

I do like the proposal! However, I am concerned about the possibility of a breaking change since I do expect that users of the rebase-snap tool to have the expectation to obtain diff layers containing dirty memory since last retrieval. Shouldn't we let rebase-snap tool users know that from now in order to obtain a diff layer they need to make sure to remove any previous (name conflicting) snapshots?

Mh, yes, for successive diff snapshots, this essentially does "rebase-snap" functionality at snapshot-taking-time, we might want to add a note about that somewhere. For full snapshots however, this "fixes" the scenario where currently if a full snapshot from which the microvm is loaded is being used as the target of a full/diff snapshot, it just gets zeroed out.

roypat · 2023-12-07T11:33:14Z

Hey @CompuIves, we have implemented this functionality in #4301 based on your proposal. I am thus closing this PR. Thank you again for your contribution!

dianpopa assigned CompuIves Jun 1, 2022

luminitavoicu reviewed Jun 6, 2022

View reviewed changes

CompuIves force-pushed the codesandbox/memory-save-snapshot branch from f3b8fa6 to 62db03e Compare June 15, 2022 12:53

luminitavoicu reviewed Jun 16, 2022

View reviewed changes

allow writing snapshot on top of existing files

18c70af

Signed-off-by: Ives van Hoorne <ives@codesandbox.io>

CompuIves force-pushed the codesandbox/memory-save-snapshot branch from 94030d2 to 18c70af Compare June 21, 2022 10:02

dianpopa added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Aug 18, 2022

dianpopa requested a review from a team September 27, 2022 08:40

dianpopa added Status: Blocked Indicates that an issue or pull request cannot currently be worked on and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Mar 16, 2023

ShadowCurse assigned roypat Oct 23, 2023

roypat mentioned this pull request Dec 5, 2023

Allow directly writing snapshots on top of existing files #4301

Merged

9 tasks

roypat closed this Dec 7, 2023

maggie-lou mentioned this pull request Feb 7, 2025

[SS] Don't cache chunks of all 0s buildbuddy-io/buildbuddy#8308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow directly writing snapshots on top of existing files #3002

Allow directly writing snapshots on top of existing files #3002

Uh oh!

CompuIves commented May 25, 2022

Uh oh!

acatangiu commented May 25, 2022 •

edited

Loading

Uh oh!

CompuIves commented Jun 1, 2022

Uh oh!

CompuIves commented Jun 1, 2022

Uh oh!

luminitavoicu commented Jun 6, 2022

Uh oh!

luminitavoicu left a comment

Uh oh!

luminitavoicu Jun 6, 2022

Uh oh!

CompuIves commented Jun 15, 2022

Uh oh!

CompuIves commented Jun 15, 2022

Uh oh!

luminitavoicu left a comment

Uh oh!

Uh oh!

Uh oh!

luminitavoicu Jun 16, 2022

Uh oh!

CompuIves commented Jun 21, 2022

Uh oh!

dianpopa commented Mar 16, 2023

Uh oh!

roypat commented Jul 25, 2023

Uh oh!

roypat commented Dec 7, 2023

Uh oh!

Uh oh!

Allow directly writing snapshots on top of existing files #3002

Allow directly writing snapshots on top of existing files #3002

Uh oh!

Conversation

CompuIves commented May 25, 2022

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

acatangiu commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CompuIves commented Jun 1, 2022

Uh oh!

CompuIves commented Jun 1, 2022

Uh oh!

luminitavoicu commented Jun 6, 2022

Uh oh!

luminitavoicu left a comment

Choose a reason for hiding this comment

Uh oh!

luminitavoicu Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

CompuIves commented Jun 15, 2022

Uh oh!

CompuIves commented Jun 15, 2022

Uh oh!

luminitavoicu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luminitavoicu Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

CompuIves commented Jun 21, 2022

Uh oh!

dianpopa commented Mar 16, 2023

Uh oh!

roypat commented Jul 25, 2023

Uh oh!

roypat commented Dec 7, 2023

Uh oh!

Uh oh!

acatangiu commented May 25, 2022 •

edited

Loading