Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Snaps] Drive mounts for block device disk paths during snapshot loading #4014

Open
3 tasks done
CuriousGeorgiy opened this issue Aug 9, 2023 · 8 comments · May be fixed by #4072
Open
3 tasks done

[Snaps] Drive mounts for block device disk paths during snapshot loading #4014

CuriousGeorgiy opened this issue Aug 9, 2023 · 8 comments · May be fixed by #4072
Assignees
Labels
Status: Parked Indicates that an issues or pull request will be revisited later

Comments

@CuriousGeorgiy
Copy link

CuriousGeorgiy commented Aug 9, 2023

Feature Request

I am hacking on firecracker-containerd to support firecracker snapshots, and I am facing the following problems:

Firstly, the addVsocksHandler expectedly fails with the following error:

WARN[2023-08-08T06:28:05.886174648-04:00] Failed handler "fcinit.AddVsocks": [PUT /vsock][400] putGuestVsockBadRequest  &{FaultMessage:The requested operation is not supported after starting the microVM.}  runtime=aws.firecracker

If I comment this handler out, everything works okay and firecracker-containerd is able to connect to the agent. Seems like this handler is redundant, and should be simply removed (I can bring a patch to firecracker-go-sdk).

Secondly, I am trying to figure out a way to restore the disk backing files. The problem I am facing is that container image snapshot device paths are not deterministic (their assignment depend on the containerd snapshotter implementation). To overcome this, I tried to use the drive mounts feature of firecracker-containerd and mount the container snapshot device for the the snapshot reloaded VM to the path that the original device had, but it doesn’t work, since drives cannot be configured1 for snapshot booted VMs.

Describe the desired solution

Do you have any ideas as to how it would be possible to develop the approach with drive mounts?

Describe possible alternatives

Right now I am manually patching the block device path corresponding to container snapshot during micro VM state to a path provided from the LoadSnapshot request (which is obviously unfavorable).

Checks

  • Have you searched the Firecracker Issues database for similar requests?
  • Have you read all the existing relevant Firecracker documentation?
  • Have you read and understood Firecracker's core tenets?

Footnotes

  1. https://github.com/firecracker-microvm/firecracker/blob/807bd5a3ce949121f06234bf6308de33c14eb9ce/src/api_server/swagger/firecracker.yaml#L598-L599

@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] [Feature Request] block device disk paths changing for snapshot loading Aug 9, 2023
@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] block device disk paths changing for snapshot loading [Feature Request] block device disk paths during snapshot loading Aug 9, 2023
@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] block device disk paths during snapshot loading [Feature Request] block device disk paths and snapshot loading Aug 9, 2023
@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] block device disk paths and snapshot loading [Feature Request] Block device disk paths and snapshot loading Aug 9, 2023
@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] Block device disk paths and snapshot loading [Feature Request] Drive mounts for block device disk paths during snapshot loading Aug 9, 2023
@JonathanWoollett-Light
Copy link
Contributor

We think this is more focused on firecracker-containerd and may be able to get better help there, so it may make sense to transfer this issue to the firecracker-containerd repository. Do you see an issue with this?

@CuriousGeorgiy
Copy link
Author

CuriousGeorgiy commented Aug 14, 2023

@JonathanWoollett-Light

I agree, but at the same time I would like to emphasize that, AFAIC, there may be not enough firecracker capabilities to solve this problem right now.

BTW, just to make it clearer, my current hacky solution looks like this:

diff --git a/src/api_server/swagger/firecracker.yaml b/src/api_server/swagger/firecracker.yaml
index 930017e8..0393ab44 100644
--- a/src/api_server/swagger/firecracker.yaml
+++ b/src/api_server/swagger/firecracker.yaml
@@ -1093,6 +1093,7 @@ definitions:
     required:
       - mem_file_path
       - snapshot_path
+      - disk_device_path
     properties:
       enable_diff_snapshots:
         type: boolean
@@ -1108,6 +1109,10 @@ definitions:
         type: boolean
         description:
           When set to true, the vm is also resumed if the snapshot load is successful.
+      disk_device_path:
+        type: string
+        description:
+          Path to the disk device backing the disk state at the time of the snapshot creation.
 
   TokenBucket:
     type: object
diff --git a/src/devices/src/virtio/block/persist.rs b/src/devices/src/virtio/block/persist.rs
index 3d72bf97..564bc839 100644
--- a/src/devices/src/virtio/block/persist.rs
+++ b/src/devices/src/virtio/block/persist.rs
@@ -91,7 +91,7 @@ pub struct BlockState {
     )]
     cache_type: CacheTypeState,
     root_device: bool,
-    disk_path: String,
+    pub disk_path: String,
     virtio_state: VirtioDeviceState,
     rate_limiter_state: RateLimiterState,
     #[version(start = 3)]
diff --git a/src/vmm/src/persist.rs b/src/vmm/src/persist.rs
index 26317225..baf54ca1 100644
--- a/src/vmm/src/persist.rs
+++ b/src/vmm/src/persist.rs
@@ -442,7 +442,21 @@ pub fn restore_from_snapshot(
 ) -> std::result::Result<Arc<Mutex<Vmm>>, LoadSnapshotError> {
     use self::LoadSnapshotError::*;
     let track_dirty_pages = params.enable_diff_snapshots;
-    let microvm_state = snapshot_state_from_file(&params.snapshot_path, version_map)?;
+    let mut microvm_state = snapshot_state_from_file(&params.snapshot_path, version_map)?;
+
+    let disk_device_path = &params.disk_device_path;
+    if ! disk_device_path.eq("") {
+        // We assume that each microVM is backed by exactly one container image
+        // snapshot device (i.e., that no more than one container is run on each microVM).
+        assert_eq!(microvm_state.device_states.block_devices.len(), 2);
+        for i in 0..2 {
+            // We assume that one of the block devices is the rootfs, the other being the
+            // container image snapshot.
+            if microvm_state.device_states.block_devices[i].device_state.disk_path.contains("snap") {
+                microvm_state.device_states.block_devices[i].device_state.disk_path = disk_device_path.clone();
+            }
+        }
+    }
 
     // Some sanity checks before building the microvm.
     snapshot_state_sanity_check(&microvm_state)?;
diff --git a/src/vmm/src/vmm_config/snapshot.rs b/src/vmm/src/vmm_config/snapshot.rs
index 9662de08..83eda2be 100644
--- a/src/vmm/src/vmm_config/snapshot.rs
+++ b/src/vmm/src/vmm_config/snapshot.rs
@@ -56,6 +56,13 @@ pub struct LoadSnapshotParams {
     /// is successful.
     #[serde(default)]
     pub resume_vm: bool,
+    /// Path to the disk device backing the disk state at the time
+    /// of the snapshot creation.
+    ///
+    /// Since the disk device path is not deterministic (it depends on the
+    /// containerd devmapper snapshotter implementation), we need to manually
+    /// patch the microVM state block device path backing the container snapshot.
+    pub disk_device_path: String,
 }
 
 /// The microVM state options.

@CuriousGeorgiy CuriousGeorgiy changed the title [Feature Request] Drive mounts for block device disk paths during snapshot loading [Snaps] Drive mounts for block device disk paths during snapshot loading Aug 15, 2023
@bchalios
Copy link
Contributor

Hey @CuriousGeorgiy? Is this still an issue?

I think you found out that you can patch the drivers after loading a snapshot and before resuming the microVM (not considering the issues related to #4036).

Is it ok if I close this?

@CuriousGeorgiy
Copy link
Author

CuriousGeorgiy commented Aug 24, 2023

Hey @bchalios

I think you found out that you can patch the drivers after loading a snapshot and before resuming the microVM (not considering the issues related to #4036).

In order for this to work, it would require a hustle with unmounting drives before pausing the VM and then mounting them back after resuming the VM (and I am not sure it is actually achievable, since, AFAIC, unmounting can fail, for instance, because of open files), I would prefer going for a "resource renaming" approach as @roypat mentioned in his comment.

@bchalios
Copy link
Contributor

So, the resource renaming approach is what the PATH /drive API does. Unless @roypat referred to something else, I 'm missing?

@CuriousGeorgiy
Copy link
Author

CuriousGeorgiy commented Aug 24, 2023

@bchalios

It's actually even simpler: AFAIC, what you suggest will only work after boot (i.e., after snapshot loading) — but loading from a snapshot still requires the block devices to be available at the snapshot creation time path.

Technically, this could be solved by creating stub devices for snapshot loading, but again that only adds to the mount/unmount hustle.

@bchalios
Copy link
Contributor

Oh, I see now. You and Patrick are right. As Patrick said, this is a limitation the snapshot API has at the moment. FYI not just for block devices, net and vsock are the same. So, we will need to work to solve the problem for all of these. Will keep you posted.

CuriousGeorgiy added a commit to CuriousGeorgiy/firecracker that referenced this issue Aug 24, 2023
When a snapshot of a VM created by firecracker-containerd is restored,
due to the non-deterministic container snapshot path (it depends
on the containerd snapshotter implementation), the container snapshot
path at the time of the snapshot creation is different from the
container snapshot path at the time of the snapshot loading.

Firecracker does not support renaming resources at snapshot-restore, so
as a workaround we manually substitute the VM state with the path of the
block device backing the container snapshot to the path of the new
container snapshot path received from the LoadSnapshot request.

Closes firecracker-microvm#4014

Signed-off-by: Georgiy Lebedev <[email protected]>
CuriousGeorgiy added a commit to CuriousGeorgiy/firecracker that referenced this issue Aug 24, 2023
When a snapshot of a VM created by firecracker-containerd is restored,
due to the non-deterministic container snapshot path (it depends
on the containerd snapshotter implementation), the container snapshot
path at the time of the snapshot creation is different from the
container snapshot path at the time of the snapshot loading.

Firecracker does not support renaming resources at snapshot-restore, so
as a workaround we manually substitute the VM state with the path of the
block device backing the container snapshot to the path of the new
container snapshot path received from the LoadSnapshot request.

Closes firecracker-microvm#4014

Signed-off-by: Georgiy Lebedev <[email protected]>
@CuriousGeorgiy
Copy link
Author

Opened a draft PR #4072 to illustrate what I am trying to achieve.

CuriousGeorgiy added a commit to CuriousGeorgiy/firecracker that referenced this issue Aug 24, 2023
When a snapshot of a VM created by firecracker-containerd is restored,
due to the non-deterministic container snapshot path (it depends
on the containerd snapshotter implementation), the container snapshot
path at the time of the snapshot creation is different from the
container snapshot path at the time of the snapshot loading.

Firecracker does not support renaming resources at snapshot-restore, so
as a workaround we manually substitute the VM state with the path of the
block device backing the container snapshot to the path of the new
container snapshot path received from the LoadSnapshot request.

Closes firecracker-microvm#4014

Signed-off-by: Georgiy Lebedev <[email protected]>
CuriousGeorgiy added a commit to CuriousGeorgiy/firecracker that referenced this issue Aug 24, 2023
When a snapshot of a VM created by firecracker-containerd is restored,
due to the non-deterministic container snapshot path (it depends
on the containerd snapshotter implementation), the container snapshot
path at the time of the snapshot creation is different from the
container snapshot path at the time of the snapshot loading.

Firecracker does not support renaming resources at snapshot-restore, so
as a workaround we manually substitute the VM state with the path of the
block device backing the container snapshot to the path of the new
container snapshot path received from the LoadSnapshot request.

Closes firecracker-microvm#4014

Signed-off-by: Georgiy Lebedev <[email protected]>
CuriousGeorgiy added a commit to vhive-serverless/firecracker that referenced this issue Sep 8, 2023
When a snapshot of a VM created by firecracker-containerd is restored,
due to the non-deterministic container snapshot path (it depends
on the containerd snapshotter implementation), the container snapshot
path at the time of the snapshot creation is different from the
container snapshot path at the time of the snapshot loading.

Firecracker does not support renaming resources at snapshot-restore, so
as a workaround we manually substitute the VM state with the path of the
block device backing the container snapshot to the path of the new
container snapshot path received from the LoadSnapshot request.

Closes firecracker-microvm#4014

Signed-off-by: Georgiy Lebedev <[email protected]>
@ShadowCurse ShadowCurse added the Status: Parked Indicates that an issues or pull request will be revisited later label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Parked Indicates that an issues or pull request will be revisited later
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants