Fix mount handling causing errors in VM #38
Conversation
Currently the vm will fail to mount when the upper directory is a virtiofs mount. Mount locally when configuration is not supported. Signed-off-by: Derek McGowan <derek@mcg.dev>
Adding too many disk may create a startup error, return an error and on Linux do the mount on the host side. Signed-off-by: Derek McGowan <derek@mcg.dev>
Prevent a custom erofs mount handler in containerd from performing erofs mounts. Signed-off-by: Derek McGowan <derek@mcg.dev>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors mount handling logic by extracting mount functionality into a dedicated mountutil package, enabling code reuse across different parts of the codebase. The refactoring also improves mount transformation logic by deferring disk additions until after validation.
- Extracted mount handling logic from
container.goto a newmountutilpackage - Updated
setupMountssignature across all platform-specific implementations to accept a mount directory parameter - Enhanced
transformMountsto validate disk count limits before performing VM operations and added fallback to host-side mounts for unsupported overlay configurations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/vminit/runc/container.go | Removed mount handling code and replaced with call to mountutil.All() |
| internal/shim/task/service.go | Updated setupMounts call to pass mount directory path |
| internal/shim/task/mount_other.go | Added unused mount directory parameter to setupMounts signature |
| internal/shim/task/mount_darwin.go | Added unused mount directory parameter to setupMounts signature |
| internal/shim/task/mount_linux.go | Updated to use mountutil.All() with fallback logic for unsupported mount transformations |
| internal/shim/task/mount.go | Refactored to defer disk additions, validate disk count limits, and return ErrNotImplemented for unsupported overlay configurations |
| internal/shim/manager/manager.go | Added "erofs" to the list of allowed mount types |
| internal/mountutil/mount.go | New package containing extracted mount handling logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| return am, err |
There was a problem hiding this comment.
The variable err is declared at line 44 but never assigned a value, so this will always return nil. This should either be removed or the function should properly capture errors from the disk addition loop.
| @@ -0,0 +1,208 @@ | |||
| //go:build linux | |||
There was a problem hiding this comment.
The file should be *_linux.go ?
There was a problem hiding this comment.
It could be, don't always do that if the whole package is linux only. I might be clearer though.
|
|
||
| return am, nil | ||
| if len(addDisks) > 10 { | ||
| return nil, fmt.Errorf("exceeded maximum virtio disk count: %d > 10: %w", len(addDisks), errdefs.ErrNotImplemented) |
There was a problem hiding this comment.
what was the failure report?
Is the limitation of the mac host (I only have a work laptop which is unallowed to install unauthorized binaries) or the libkrun internal?
There was a problem hiding this comment.
[2025-11-02T07:08:19Z ERROR krun] Building the microVM failed: RegisterBlockDevice(IrqsExhausted) on Linux
[2025-11-02T07:09:32Z ERROR krun] Building the microVM failed: Internal(Vm(VmSetup(VmCreate))) on Darwin
There was a problem hiding this comment.
on x86, it relates to https://github.com/containers/libkrun/blob/v1.16.0/src/arch/src/x86_64/layout.rs#L29
I think it's due to virtio-mmio interrupts (virtio-pci is much better), see firecracker
firecracker-microvm/firecracker#2286
firecracker-microvm/firecracker#5364
Nevertheless, I tend to use merged block device instead to avoid specific vmm implementation limitation.
4 participants
Currently a virtiofs upper or a large number of disks will cause a failure either in the vm start or when creating the container in the vm. On the Linux side we can just perform the mounts locally, on the Mac side we should error out earlier.
We will need to figure out another solution for mounting many disks on the Mac side, there are a few ideas already. In the meantime, we can limit the disk to 10. We can probably raise that limit a bit but needs more testing.