You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CloudHypervisor.BuildExecCmd appended a separate --disk flag for each block
device. Cloud Hypervisor expects every disk as its own value after a single --disk flag, and rejects the flag outright if it appears more than once:
error: the argument '--disk <disk>...' cannot be used multiple times
So any Linux container on cloud-hypervisor with more than one block device failed at argument
parsing and the VM was never created. A single disk worked, which is why the
existing e2e cases, all single-disk, never caught it.
This collects the generated disk specs and appends them behind one --disk.
Reproduced and verified end-to-end on urunc 3395292, cloud-hypervisor v50.0.0
(the version pinned in ci.yml), x86_64. No containerd or devmapper needed: a
hand-written OCI bundle using a com.urunc.unikernel.block rootfs plus one
loop-backed ext4 volume bind-mounted in.
Before the fix, that 2-disk container:
--disk path=/rootfs.img,id=rootfs --disk path=/dev/loop0,id=vol0
error: the argument '--disk <disk>...' cannot be used multiple times
No argument error. Cloud Hypervisor proceeds to kernel loading and stops only on
the placeholder kernel I used (KernelLoad(Bzimage(InvalidBzImage))), matching
what a single-disk container did before the change.
Also ran go build ./..., go vet ./pkg/unikontainers/hypervisors/..., gofmt
and the ./pkg/unikontainers/... unit tests. All pass.
The linter passes locally (make lint). golangci-lint v2.9 reports no new issues from this change; the 4 it flags (1 ineffassign in cmd/urunc/create.go, 3 unused in pkg/containerd-shim/guest_rootfs.go) are pre-existing on main.
The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
I was going through this PR and the change looks correct to me. On main the block loop adds a --disk for every device, so once there's more than one disk you get multiple --disk flags and cloud-hypervisor just refuses to start. Collecting them under a single --disk is the right fix.
While I was in that folder I noticed cloud-hypervisor doesn't have a test file yet. qemu and hyperlight both have one, so I followed that same pattern.
So I tried writing a small regression test for BuildExecCmd that checks multiple block devices end up under exactly one --disk flag. It fails on main (two flags) and passes with your change, and gofmt and go vet are both clean.
If it's useful, I can send it over. Do you want me to open a PR against your fix/ch-single-disk-flag branch so the fix and the test land together, or should I add it as a follow-up once this merges? Either works for me.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
CloudHypervisor.BuildExecCmdappended a separate--diskflag for each blockdevice. Cloud Hypervisor expects every disk as its own value after a single
--diskflag, and rejects the flag outright if it appears more than once:So any Linux container on cloud-hypervisor with more than one block device failed at argument
parsing and the VM was never created. A single disk worked, which is why the
existing e2e cases, all single-disk, never caught it.
This collects the generated disk specs and appends them behind one
--disk.Before:
After:
Related issues
Fixes #988
How was this tested?
Reproduced and verified end-to-end on urunc
3395292, cloud-hypervisor v50.0.0(the version pinned in
ci.yml), x86_64. No containerd or devmapper needed: ahand-written OCI bundle using a
com.urunc.unikernel.blockrootfs plus oneloop-backed ext4 volume bind-mounted in.
Before the fix, that 2-disk container:
After the fix, same container:
No argument error. Cloud Hypervisor proceeds to kernel loading and stops only on
the placeholder kernel I used (
KernelLoad(Bzimage(InvalidBzImage))), matchingwhat a single-disk container did before the change.
Also ran
go build ./...,go vet ./pkg/unikontainers/hypervisors/...,gofmtand the
./pkg/unikontainers/...unit tests. All pass.make lint). golangci-lint v2.9 reports no new issues from this change; the 4 it flags (1ineffassignincmd/urunc/create.go, 3unusedinpkg/containerd-shim/guest_rootfs.go) are pre-existing on main.make test_ctr,make test_nerdctl,make test_docker,make test_crictl).