Load Microvm snapshot support #1907

acatangiu · 2020-05-18T08:00:15Z

Reason for This PR

Fixes #1847
Fixes #1870

Description of Changes

Splits microvm building logic in boot-specific code and generic code. The latter is to be reused in the snapshot path.

Adds code to load microvm state from snapshot file and to build microvm from state definition.

Implements missing plumbing to have end-to-end functionality.

Integration tests will be added as part of #1850

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 reason for this PR is clearly provided (issue no. or explanation).
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 are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

src/vmm/src/builder.rs

ioanachirca · 2020-05-20T13:11:31Z

src/vmm/src/lib.rs

+            .map(|handle| {
+                handle
+                    .response_receiver()
+                    .recv_timeout(Duration::from_millis(1000))


Do we expect that restoring Vcpus takes longer than saving them?

I've increased all timeouts to 1000ms to give a better chance of success to highly oversubscribed scenarios/machines.

We've seen scenarios in the past where people were running stress tests and where channel timeouts would happen simply because CPUs were so contended that threads of a particular Firecracker process were scheduled to run very sparsely.

We do not enforce in Firecracker a time-limit for these operations, they will run only as fast as the host CPU runs them. The timeout's purpose here is to catch error cases where things go wrong and the thread on the other end of the channel is not responding anymore.

I'm not sure even 1000ms is enough, but it is what was used to fix #1555 so we can continue with that for now.

src/vmm/src/memory_dump.rs

src/vmm/tests/integration_tests.rs

lauralt

Just a few comments, will continue the review tomorrow.

src/vmm/src/vstate.rs

src/vmm/src/lib.rs

src/vmm/src/memory_dump.rs

src/vmm/tests/integration_tests.rs

src/vmm/src/lib.rs

src/vmm/src/memory_dump.rs

tests/framework/microvm.py

tests/integration_tests/functional/test_snapshot.py

tests/framework/resources.py

tests/integration_tests/functional/test_snapshot.py

src/api_server/swagger/firecracker.yaml

src/vmm/src/builder.rs

src/vmm/tests/integration_tests.rs

src/vmm/src/persist.rs

tests/framework/resources.py

tests/integration_tests/functional/test_snapshot.py

src/vmm/src/builder.rs

src/vmm/src/persist.rs

src/vmm/tests/integration_tests.rs

src/vmm/src/rpc_interface.rs

src/vmm/src/lib.rs

src/vmm/src/memory_snapshot.rs

src/vmm/src/persist.rs

src/vmm/tests/integration_tests.rs

src/vmm/src/rpc_interface.rs

tests/framework/microvm.py

tests/framework/resources.py

src/firecracker/src/main.rs

sandreim · 2020-05-19T14:56:17Z

src/vmm/src/builder.rs

+    vmm.start_vcpus(vcpus, seccomp_filter).map_err(Internal)?;
+
+    // Restore vcpus kvm state.
+    vmm.restore_vcpu_states(microvm_state.vcpu_states).unwrap();


Do we need to explicitly call resume from the API to resume the VCPUs ? Can't we just do that here so a new API call is not necessary. IMO the only logical action after load is to resume.

I guess this unwrap is not in the final version of the PR. Error needs to travel up the stack and presented to the user.

Do we need to explicitly call resume from the API to resume the VCPUs ? Can't we just do that here so a new API call is not necessary. IMO the only logical action after load is to resume.

Hmm, it's a good point and would simplify things a bit. But since CreateSnapshot involves pausing and resuming to be done explicitly, we would lose I guess some kind of a symmetry this way :-??.
Maybe we should start thinking more about the config file option also for load, as you already proposed in another thread. We could add a new cmd line parameter, --snapshot-file or --snapshot-config-file which would include the load config and also the optional logger and metrics configs.

I guess this unwrap is not in the final version of the PR. Error needs to travel up the stack and presented to the user.

Do we need to explicitly call resume from the API to resume the VCPUs ? Can't we just do that here so a new API call is not necessary. IMO the only logical action after load is to resume.

Yes, that is the proposed and approved API flow.

We can change that outside this PR, as it implies getting buy-in from users and updating documentation as well.

sandreim · 2020-05-19T15:01:17Z

src/vmm/src/builder.rs

+    // Restore vcpus kvm state.
+    vmm.restore_vcpu_states(microvm_state.vcpu_states).unwrap();
+
+    // Load seccomp filters for the VMM thread.


Would it make sense to persist the seccomp filters as well ? And allow the user to override it.

I guess there could be usecases for that. But let's treat it as an enhancement. It is not required for our current milestone.

src/vmm/src/builder.rs

src/vmm/src/lib.rs

acatangiu · 2020-05-28T13:37:30Z

@sandreim @lauralt @ioanachirca I've addressed all your comments, please review.

acatangiu · 2020-06-02T06:53:44Z

@sandreim @lauralt @ioanachirca this PR is up-to-date. Please review.

ioanachirca

Overall LGTM.

tests/framework/microvm.py

Add support to restore specific kvm states to the owned Vcpus. Signed-off-by: Adrian Catangiu <[email protected]>

The trait exposes methods to dump the guest memory to file, describe the guest memory regions and restore the guest memory. This commit also renames the memory_dump module to memory_snapshot. Signed-off-by: Ioana Chirca <[email protected]>

Split microVM building logic in boot-specific code and generic code. The latter is to be reused in the snapshot path. Signed-off-by: Adrian Catangiu <[email protected]>

Signed-off-by: Adrian Catangiu <[email protected]>

Adds the missing plumbing to support the '/snapshot/load' command. Signed-off-by: Adrian Catangiu <[email protected]>

Signed-off-by: Adrian Catangiu <[email protected]>

Added KVM Vm state and Vcpu state related ioctl numbers to the allowed list of ioctls. Added 'libc::ftruncate' used by 'File::set_len'. Signed-off-by: Adrian Catangiu <[email protected]>

Signed-off-by: Adrian Catangiu <[email protected]>

acatangiu · 2020-06-05T10:31:04Z

I removed that dumb integ test from this PR. A highly improved version of it will be added by @sandreim as part of #1850

lauralt

I noticed this PR was merged while i was reviewing :(, it was an approve anyway, but I had 2 nits (or questions) and I will add them though, maybe we will keep them in mind for future PRs (especially the coverage one).

tests/integration_tests/build/test_binary_size.py

tests/integration_tests/build/test_coverage.py

acatangiu self-assigned this May 18, 2020

acatangiu force-pushed the load_snap branch 2 times, most recently from a506af4 to 982de5a Compare May 19, 2020 17:02

lauralt mentioned this pull request May 20, 2020

Remove backtrace dependency #1841

Merged

8 tasks

acatangiu force-pushed the load_snap branch from 982de5a to 1cc1b03 Compare May 20, 2020 09:37

acatangiu added the Feature: Snapshotting label May 20, 2020

ioanachirca reviewed May 20, 2020

View reviewed changes

acatangiu force-pushed the load_snap branch from 1cc1b03 to e38b258 Compare May 21, 2020 09:39

acatangiu changed the title ~~[RFC] Load Microvm snapshot support~~ Load Microvm snapshot support May 21, 2020

acatangiu marked this pull request as ready for review May 21, 2020 10:05

acatangiu force-pushed the load_snap branch 2 times, most recently from ddf710b to 120c4d5 Compare May 21, 2020 13:17

lauralt self-requested a review May 21, 2020 13:56

lauralt reviewed May 21, 2020

View reviewed changes

ioanachirca reviewed May 22, 2020

View reviewed changes

src/vmm/src/lib.rs Outdated Show resolved Hide resolved

ioanachirca reviewed May 22, 2020

View reviewed changes

src/vmm/src/memory_dump.rs Outdated Show resolved Hide resolved

ioanachirca reviewed May 22, 2020

View reviewed changes

acatangiu added Status: Author Status: Awaiting review Indicates that a pull request is ready to be reviewed labels May 22, 2020

ioanachirca force-pushed the load_snap branch from 120c4d5 to c5f1ef1 Compare May 22, 2020 13:39

lauralt reviewed May 22, 2020

View reviewed changes

ioanachirca force-pushed the load_snap branch from c5f1ef1 to c9e8612 Compare May 22, 2020 14:40

acatangiu force-pushed the load_snap branch from c9e8612 to 3b03d8d Compare May 22, 2020 16:46

acatangiu removed the Status: Author label May 22, 2020

lauralt reviewed May 25, 2020

View reviewed changes

src/vmm/src/builder.rs Show resolved Hide resolved

src/vmm/src/persist.rs Show resolved Hide resolved

src/vmm/tests/integration_tests.rs Show resolved Hide resolved

src/vmm/tests/integration_tests.rs Show resolved Hide resolved

src/vmm/src/rpc_interface.rs Show resolved Hide resolved

ioanachirca mentioned this pull request May 25, 2020

Extend snapshotted memory state #1870

Closed

acatangiu force-pushed the load_snap branch from 3b03d8d to b5e6c67 Compare May 25, 2020 11:47

lauralt mentioned this pull request May 25, 2020

Update Rust toolchain to 1.43.1 #1908

Merged

8 tasks

acatangiu force-pushed the load_snap branch 2 times, most recently from e03d718 to 28ac5d8 Compare May 26, 2020 13:59

lauralt reviewed May 26, 2020

View reviewed changes

sandreim reviewed May 27, 2020

View reviewed changes

acatangiu force-pushed the load_snap branch 2 times, most recently from d389b0d to daf3cfc Compare May 27, 2020 10:33

acatangiu force-pushed the load_snap branch 2 times, most recently from 6b37662 to a30bb29 Compare May 29, 2020 16:16

ioanachirca reviewed Jun 2, 2020

View reviewed changes

tests/framework/microvm.py Outdated Show resolved Hide resolved

acatangiu force-pushed the load_snap branch 3 times, most recently from a1a4796 to 4935b94 Compare June 3, 2020 16:06

sandreim previously approved these changes Jun 4, 2020

View reviewed changes

acatangiu and others added 9 commits June 5, 2020 13:26

vmm: add restore vcpu states command

b0029df

Add support to restore specific kvm states to the owned Vcpus. Signed-off-by: Adrian Catangiu <[email protected]>

vmm: add SnapshotMemory trait

3df96d6

The trait exposes methods to dump the guest memory to file, describe the guest memory regions and restore the guest memory. This commit also renames the memory_dump module to memory_snapshot. Signed-off-by: Ioana Chirca <[email protected]>

vmm: split off boot-specific builder logic

afc3b54

Split microVM building logic in boot-specific code and generic code. The latter is to be reused in the snapshot path. Signed-off-by: Adrian Catangiu <[email protected]>

vmm: build microvm from snapshot

8a77340

Signed-off-by: Adrian Catangiu <[email protected]>

vmm: dedup some code in integ tests

788a1cd

Signed-off-by: Adrian Catangiu <[email protected]>

implement snapshot load API call

81abf4d

Adds the missing plumbing to support the '/snapshot/load' command. Signed-off-by: Adrian Catangiu <[email protected]>

vmm: test persist errors display/debug

aa2d1bd

Signed-off-by: Adrian Catangiu <[email protected]>

seccomp: update seccomp whitelist for snapshot

d61e3c6

Added KVM Vm state and Vcpu state related ioctl numbers to the allowed list of ioctls. Added 'libc::ftruncate' used by 'File::set_len'. Signed-off-by: Adrian Catangiu <[email protected]>

tests: increase binary size limit and update coverage

c7e56e0

Signed-off-by: Adrian Catangiu <[email protected]>

acatangiu dismissed sandreim’s stale review via c7e56e0 June 5, 2020 10:27

acatangiu force-pushed the load_snap branch from 4935b94 to c7e56e0 Compare June 5, 2020 10:27

ioanachirca approved these changes Jun 5, 2020

View reviewed changes

sandreim approved these changes Jun 5, 2020

View reviewed changes

sandreim merged commit 3fecd27 into firecracker-microvm:master Jun 5, 2020

lauralt reviewed Jun 5, 2020

View reviewed changes

tests/integration_tests/build/test_binary_size.py Show resolved Hide resolved

tests/integration_tests/build/test_coverage.py Show resolved Hide resolved

acatangiu deleted the load_snap branch July 27, 2020 09:08

Load Microvm snapshot support #1907

Load Microvm snapshot support #1907

Uh oh!

Conversation

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

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

Uh oh!

ioanachirca May 20, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lauralt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandreim May 19, 2020

Choose a reason for hiding this comment

Uh oh!

lauralt May 27, 2020

Choose a reason for hiding this comment

Uh oh!

acatangiu May 27, 2020

Choose a reason for hiding this comment

Uh oh!

sandreim May 19, 2020

Choose a reason for hiding this comment

Uh oh!

acatangiu May 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

acatangiu commented May 28, 2020

Uh oh!

acatangiu commented Jun 2, 2020

Uh oh!

ioanachirca left a comment

acatangiu commented May 18, 2020 •

edited

Loading

acatangiu May 21, 2020 •

edited

Loading