Repository navigation
block: add read-only VMDK disk image support - #5741
ChengyuZhu6 wants to merge 5 commits into
Conversation
|
@ChengyuZhu6 what prevents this as a draft? |
|
Hi @ChengyuZhu6, unfortunately we don't have enough bandwidth to review this for now. |
@ShadowCurse I guess this one is not complicated and it seems almost half the change is test cases. Also I guess the commit message should be improved to make the usage clear at least. |
|
Hey @hsiangkao, @ChengyuZhu6, first of all, thank you very much for the contribution! I think this is a very promising solution for container workloads. Do you have any examples on how this would integrate with containerd? However, we take every new feature very seriously and we need to undergo an internal security review for every new feature, which means that despite the change itself being small, it requires a non-trivial amount of time from our side that we don't have at the moment. In any case, having a very quick look at the PR:
|
Hi @Manciukic, thank you very much for the reply! currently I don't have some free slot to run an end-to-end example using containerd + kata-containers + firecracker, but I will find (or I hope @ChengyuZhu6 could finish the pull request message at least) time to run an e2e later. but the story is much similar to what we did for nerdbox + libkrun: and my final interest is that to apply VMDK to virtio-pmem as well so that each backing file can be passthrough into guests and shared in a finer way: of course, virtio-pmem could bring some security concern, but depends on the workload, it could also be useful if memory sharing is not an issue.
I will defer this question to @ChengyuZhu6 .
yes, imago has published on crates.io with qcow2 and this limited vmdk support. |
2d5fc9b to
726beb2
Compare
Good catch. I removed the direct |
Agreed. I switched imago from |
a42850f to
86a6e35
Compare
|
@ChengyuZhu6 @ShadowCurse @Manciukic could we consider enabling qcow2 using imago too? |
I think this is a good idea. I suggest including it in the following PR, since it would make review easier. |
|
Thank you @ChengyuZhu6 for your contribution, left a few comment :) |
806ea23 to
72745fa
Compare
There was a problem hiding this comment.
Can you change the dependencies in a single commit? Otherwise we have an intermediary step in our repo with a set of dependencies we've never checked and never audited
| } | ||
|
|
||
| /// Detects the format of a disk image by reading its first bytes. | ||
| pub fn detect_disk_format(file: &mut File) -> io::Result<DiskImageFormat> { |
There was a problem hiding this comment.
This is overwritten in later commits. Can you refactor your commits in a way that each change builds on top of each other?
PS: this is a mechanical tasks where AI agents are really good at
| let can_reuse_engine = format == block_io::DiskImageFormat::Raw | ||
| && !matches!(self.file_engine, FileEngine::Vmdk(_)); |
There was a problem hiding this comment.
What about letting the engine return an UnsupportedOperation error or something on update_file_path?
|
|
||
| let disk_size = if can_reuse_engine { | ||
| let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?; | ||
| match &mut self.file_engine { |
There was a problem hiding this comment.
I don't understand why we're replacing a call to update_file_path with this
There was a problem hiding this comment.
The VMDK engine owns state built when the image was opened (descriptor + extent fds), and the size comes from the descriptor — the descriptor file itself is a few hundred bytes, so file_size() would report a bogus capacity.
| if config.blk_size.is_none() && config.topology.is_none() { | ||
| if config.blk_size.is_none() | ||
| && config.topology.is_none() | ||
| && !matches!(disk_properties.file_engine, FileEngine::Vmdk(_)) |
There was a problem hiding this comment.
This condition should be more along the lines of "the DiskImageFormat is raw".
There was a problem hiding this comment.
it's now just if image_format == DiskImageFormat::Raw
| stem, _ = os.path.splitext(self.path) | ||
| return stem + "-flat.vmdk" | ||
|
|
||
| def _build(self): |
There was a problem hiding this comment.
TBH: I'm not sure about this: we're creating ourselves a file in this format, so we're risking validating in this test our wrong assumptions. For example, I'm not a VMDK expert, but isn't the extent supposed to have an header?
Is it possible to test against a real file?
|
Hey, I looked at the PR. I think we should also add some performance tests for this, since now the read speed is completely dictated by the imago library. We need a way to keep an eye on it. |
Added |
3b51540 to
8e07b90
Compare
I guess raw disk performance shouldn't be impacted by imago? but yes, for vmdk it's better to have a test for this. |
|
kindly ping @JackThomson2 @marco-marangoni |
... @marco-marangoni does it look good to you now? |
|
Sorry, I came back from Holidays today. I'll take a look today or tomorrow. Thanks for your patience :) |
Thanks what I am interested is also the following qcow2 support since I would like to use it for diskfile incremental updates as a Part of the vm sandbox solution. |
|
Thanks, the change looks good for me. I think supporting qcow2 would be a nice addition |
yes, I want to use qcow2 as a local writable block delta, and use it to form multi-layer EROFS VM templates (disk as a file inside; memory and other states as other files). @marco-marangoni could you trigger the remaining tests as well? |
|
@JackThomson2 could you approve again and merge this? or could others help?.. many thanks! |
| def _build(self): | ||
| """Create the image with ``qemu-img`` and fill the extent with the pattern.""" | ||
| utils.check_output( | ||
| f"qemu-img create -f vmdk -o subformat=monolithicFlat {self.path} {self.size}M" |
There was a problem hiding this comment.
Ah, I see the CI fails because qemu-img isn't available.
Maybe you can generate the file offline, and include it as a fixture, with a comment explaining how it was generated, so I can validate.
Alternatively we could include the utility in the container, but this approach seems easier.
|
@JackThomson2 @marco-marangoni I have fixed ci fails for the dependency of qemu-img. PTAL, thank you! |
| .get_slice(addr, count as usize) | ||
| .map_err(VmdkIoError::GuestMemory)?; | ||
| let slices = [slice]; | ||
| let (bufv, _guard) = IoVectorMut::from_volatile_slice(&slices); |
There was a problem hiding this comment.
Ah, sorry, there's an half-bug in imago that I didn't see before: on error, it marks the whole range as dirty.
That's okay for other project, but we're working on a feature that requires for the dirty tracking to be correct (as in, only mark pages that were actually written to).
SyncFileEngine::read had a similar problem, and we ended up passing a bitmap-less slice, and did the marking ourselves.
Unfortunately, that's a blocker
There was a problem hiding this comment.
We should also add this and other offending calls of imago to disallowed-methods in clippy.toml (we already have some there for the same reason)
| impl TryFrom<&BlockDeviceConfig> for VhostUserBlockConfig { | ||
| type Error = VhostUserBlockError; | ||
|
|
||
| fn try_from(value: &BlockDeviceConfig) -> Result<Self, Self::Error> { |
There was a problem hiding this comment.
We should probably also check for image_format here
| // size of the descriptor file, so the engine has to be rebuilt instead. | ||
| let image_format = DiskImageFormat::from(&self.file_engine); | ||
| let disk_size = if image_format == DiskImageFormat::Raw { | ||
| let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?; |
There was a problem hiding this comment.
We're returning an error here, however we already mutated self.image_id a few lines above. We should restructure the code such that errors result in no mutations
| } | ||
|
|
||
| fn open_extent(&self, path: &Path) -> io::Result<ImagoFile> { | ||
| // VMDK descriptors are untrusted input. Extents are read-only, direct children of the |
There was a problem hiding this comment.
I would rephrase the "VMDK descriptors are untrusted input" claim. It's a bit strong, since it's the system administrator that supplies them
| ) | ||
| })?; | ||
| // SAFETY: descriptor_dir is an open directory FD and filename is a NUL-terminated component. | ||
| let fd = unsafe { |
There was a problem hiding this comment.
Could we also check that it's a "normal" file, not a FIFO/block/char/etc.?
I'm asking because if that's the case, then Firecracker will either hang or panic because of syscalls not allowed
There was a problem hiding this comment.
Added O_NONBLOCK to openat and the returned FD is immediately wrapped in a File, and its metadata is checked to ensure it is a regular file.
|
Sorry, I had another look with fresh eyes, and found some other issues. Most are small, however the dirty tracking is pretty important. |
Add the imago crate (sync and vm-memory features) used to read VMDK disk images, and allow the syscalls the backend needs after sandboxing: openat, readlink/readlinkat and preadv. Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
Add a VMDK file engine backed by imago and select it through a new `image_format` field on the drive API, which defaults to Raw. VMDK images are read-only and only work with the Sync IO engine, as imago reads them synchronously; anything else is rejected when the drive is configured. The engine opens the descriptor and its extents through a gate that only allows read-only, direct children of the descriptor directory, so an untrusted descriptor cannot escape it. Snapshots record the format so a restored drive is reopened as VMDK. Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
Patching a raw drive swaps the file the engine points at. A VMDK engine instead owns state parsed when the image was opened, and its size comes from the descriptor rather than from the descriptor file, so rebuild it on patch. Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
Cover booting with a read-only VMDK data disk: the guest must read back exactly the extent contents, see the expected device size, and fail writes. The image is created with qemu-img, so the descriptor and the extent are what a real VMDK producer emits. Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
Reads from a VMDK image are served by imago instead of a plain file, so track their throughput and latency the same way as the raw block device benchmark. Signed-off-by: Chengyu Zhu <hudsonzhu@tencent.com>
This change adds read-only VMDK support to the virtio-block path, and the primary motivation is the EROFS fsmerge scenario described in containerd/nerdbox#30. In that workflow, multiple layer blobs are referenced through one VMDK descriptor, so the guest can consume them as one logical block device instead of attaching many devices.
Why this is needed:
The main driver is the EROFS fsmerge workflow (see containerd/nerdbox#30). In that setup, many layer blobs are referenced by a small metadata image. For VM-based runtimes, attaching one block device per layer does not scale well.
What is included in this patch:
This patch adds read-only VMDK support so those layers can be presented as one logical disk through a standard descriptor format. That removes the need to pass a large number of block devices or add an extra image conversion step before boot.
How this solves the problem:
Changes
...
Reason
...
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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.