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

Allow specifying a custom devmapper snapshot device on snapshot boot #3

Open
wants to merge 2 commits into
base: v0.25_reference
Choose a base branch
from

Conversation

amohoste
Copy link

@amohoste amohoste commented Nov 24, 2021

This PR adds the functionality to specify a custom devmapper snapshot block device that contains the container root filesystem contents. The content of this block device must be the exact filesystem contents present at the time the corresponding Firecracker snapshot was taken. Allowing to specify a custom snapshot block devices allows us to more efficiently manage snapshots along with the possibility to boot from a remote snapshot. Additionally, the transport_reset event introduced in v0.25.0 was also removed since this interfered with the ability to restore a snapshot due to the FC agent becoming unreachable.

@amohoste amohoste changed the title V0.25 reference custom snap Allow specifying a custom devmapper snapshot device on snapshot boot Nov 24, 2021
@amohoste amohoste force-pushed the v0.25_reference_custom_snap branch 2 times, most recently from 5e5df1a to e322467 Compare November 24, 2021 20:39
Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that there are quite a few unnecessary changes in this PR. please see my comments and revert the changes that can be avoided. The reason for this is that updating the fork is way simpler if our changes are minimal as compared to upstream.

src/devices/src/virtio/vsock/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/device_manager/persist.rs Outdated Show resolved Hide resolved
src/vmm/src/device_manager/persist.rs Outdated Show resolved Hide resolved
src/devices/src/virtio/vsock/device.rs Outdated Show resolved Hide resolved
src/devices/src/virtio/vsock/device.rs Outdated Show resolved Hide resolved
@amohoste amohoste force-pushed the v0.25_reference_custom_snap branch from e322467 to 9ea5ae5 Compare November 28, 2021 15:24
@amohoste
Copy link
Author

amohoste commented Nov 28, 2021

Regarding the comments, I completely undid the commit introducing the transport_reset_event function, however I agree temporarily commenting the place where it is invoked is a better approach. That way we can reintroduce that function call when someone looks into how to fix the agent becoming unavailable. I have now commented out the place where the transport_reset_event function is invoked.

Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few more minor tweaks please

src/devices/src/virtio/block/persist.rs Show resolved Hide resolved
src/devices/src/virtio/vsock/device.rs Outdated Show resolved Hide resolved
src/vmm/src/device_manager/persist.rs Show resolved Hide resolved
@amohoste amohoste force-pushed the v0.25_reference_custom_snap branch 2 times, most recently from 7f68758 to d824d36 Compare November 28, 2021 15:35
@amohoste amohoste deleted the branch ease-lab:v0.25_reference November 30, 2021 14:41
@amohoste amohoste closed this Nov 30, 2021
@amohoste amohoste reopened this Nov 30, 2021
@ustiugov
Copy link
Member

ustiugov commented Feb 9, 2022

lgtm

@amohoste amohoste force-pushed the v0.25_reference_custom_snap branch from 89843c4 to 52890e5 Compare April 24, 2022 20:19
@amohoste amohoste force-pushed the v0.25_reference_custom_snap branch from 52890e5 to 9b375b6 Compare April 24, 2022 20:23
Copy link
Member

@ustiugov ustiugov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor tweaks

@@ -69,11 +69,11 @@
# would help with reproducible builds (in addition to pinning Cargo.lock)

# Development container image (without tag)
DEVCTR_IMAGE_NO_TAG="docker.io/vhiveease/fcuvm_dev"
DEVCTR_IMAGE_NO_TAG="docker.io/amohoste/fcuvm_dev"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix, update the build image on vhiveease if necessary


# Development container tag
#DEVCTR_IMAGE_TAG="v30"
DEVCTR_IMAGE_TAG="v16_upf"
DEVCTR_IMAGE_TAG="v30"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DEVCTR_IMAGE_TAG="v30"
DEVCTR_IMAGE_TAG="v30_upf"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants