Initial support for VMs - #911
Conversation
5129f33 to
c5ebeb3
Compare
194cc9f to
fdc3922
Compare
83f9a66 to
ac4fd67
Compare
ba3ac38 to
7156eec
Compare
7156eec to
d59e416
Compare
d59e416 to
92e81d1
Compare
|
@dmitry-lyfar updated New commits:
Major changes:
Minor changes:
|
c21dbbc to
b703e7e
Compare
b703e7e to
e0e71ea
Compare
ac3fc2c to
57ed681
Compare
There was a problem hiding this comment.
Pull request overview
Adds experimental LXD virtual-machine workshops while preserving container behavior.
Changes:
- Adds confinement-aware definitions, APIs, CLI output, image handling, and VM launch support.
- Reworks readiness detection and introduces filesystem freezing and ID mapping.
- Refactors unit and integration coverage for multiple bases and confinement types.
Reviewed changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/main/start/task.yaml |
Updates readiness tests. |
tests/main/launch/workshop.yaml.in |
Parameterizes base and confinement. |
tests/main/launch/task.yaml |
Splits launch variants and validates ID maps. |
tests/main/launch/.workshop/ws-24.yaml |
Removes duplicated fixture. |
tests/main/launch/.workshop/ws-22.yaml |
Removes duplicated fixture. |
tests/main/launch-multiple/task.yaml |
Adds DNS validation and cleanup. |
tests/lib/utils.sh |
Enables experimental VMs in tests. |
tests/integration/workshop-snapshots/task.yaml |
Splits snapshot test variants. |
snap/local/commands/run_daemon |
Exports the VM feature setting. |
internal/workshop/workshop_file.go |
Adds confinement parsing. |
internal/workshop/workshop_file_test.go |
Tests confinement parsing. |
internal/workshop/lxd/tests/integration/workshop_test.go |
Updates LXD integration coverage. |
internal/workshop/lxd/tests/integration/workshop_exec_test.go |
Reuses shared test devices. |
internal/workshop/lxd/tests/integration/snapshot-format.yaml |
Updates snapshot expectations. |
internal/workshop/lxd/tests/integration/snapshot_test.go |
Adds VM snapshot comparisons. |
internal/workshop/lxd/tests/integration/project_test.go |
Enables multicall test binaries. |
internal/workshop/lxd/tests/helper/helper.go |
Adds shared VM test helpers. |
internal/workshop/lxd/start_command.sh |
Removes legacy startup script. |
internal/workshop/lxd/lxd_base_manager.go |
Selects images by confinement. |
internal/workshop/lxd/lxd_backend.go |
Implements VM launch and readiness behavior. |
internal/workshop/lxd/lxd_backend_test.go |
Tests container and VM configuration. |
internal/workshop/lxd/lxd_backend_snapshots.go |
Adds VM filesystem freezing. |
internal/workshop/lxd/lxd_backend_project.go |
Includes VMs in project operations. |
internal/workshop/fakebackend/backend.go |
Propagates confinement through tests. |
internal/workshop/backend.go |
Extends backend contracts for confinement. |
internal/waitready/waitready.go |
Makes timeout test-configurable. |
internal/overlord/workshopstate/request.go |
Adjusts refresh stops and execution lookup. |
internal/overlord/workshopstate/request_test.go |
Updates snapshot fixtures. |
internal/overlord/workshopstate/manifest.go |
Gates VM use and rejects VM SDKs. |
internal/overlord/workshopstate/manifest_test.go |
Tests VM manifest restrictions. |
internal/overlord/workshopstate/manager.go |
Warns about legacy workshop formats. |
internal/overlord/workshopstate/handlers_test.go |
Updates confinement fixtures. |
internal/overlord/sdkstate/handlers_test.go |
Updates confinement fixtures. |
internal/overlord/ifacestate/ifacemgr_test.go |
Updates confinement fixtures. |
internal/overlord/hookstate/handlers_test.go |
Updates confinement fixtures. |
internal/overlord/healthstate/healthstate_test.go |
Updates confinement fixtures. |
internal/interfaces/lxd_device/tests/integration/backend_test.go |
Adapts device integration setup. |
internal/idmap/structs.go |
Defines ID-map structures. |
internal/idmap/idmapset_linux.go |
Implements Linux ID-map operations. |
internal/fsfreeze/sys/zsysnum_linux.go |
Defines generated ioctl constants. |
internal/fsfreeze/sys/sysnum_linux.go |
Provides ioctl generation input. |
internal/fsfreeze/sys/syscall_linux.go |
Wraps freeze/thaw ioctls. |
internal/fsfreeze/sys/generate.sh |
Generates ioctl constants. |
internal/fsfreeze/fsfreeze.go |
Implements guest filesystem freezing. |
internal/fsfreeze/fsfreeze_test.go |
Tests filesystem freezing. |
internal/fsfreeze/export_test.go |
Exposes freeze test hooks. |
internal/dirs/dirs.go |
Adds the guest fsfreeze path. |
internal/daemon/snapshot-ingredients.yaml |
Includes confinement in snapshots. |
internal/daemon/api_workshops.go |
Exposes confinement through the API. |
internal/daemon/api_workshops_test.go |
Updates API expectations. |
internal/daemon/api_sdks_test.go |
Updates snapshot fixtures. |
internal/daemon/api_exec_test.go |
Updates snapshot fixtures. |
internal/daemon/api_connections.go |
Locks state during workshop lookup. |
internal/daemon/api_connections_test.go |
Updates snapshot fixtures. |
docs/reference/definition-files/schema.json |
Adds the confinement schema field. |
cmd/workshopctl/main.go |
Adds the fsfreeze multicall mode. |
cmd/workshop/init.go |
Adds workshop init --vm. |
cmd/workshop/init_test.go |
Tests VM initialization. |
cmd/workshop/info.go |
Displays confinement. |
cmd/workshop/info_test.go |
Updates info output expectations. |
client/workshop.go |
Exposes confinement to clients. |
Files not reviewed (1)
- internal/fsfreeze/sys/zsysnum_linux.go: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (e *IdmapEntry) Intersects(i IdmapEntry) bool { | ||
| if (e.Isuid && i.Isuid) || (e.Isgid && i.Isgid) { | ||
| switch { | ||
| case isBetween(e.Hostid, i.Hostid, i.Hostid+i.Maprange-1): | ||
| return true | ||
| case isBetween(i.Hostid, e.Hostid, e.Hostid+e.Maprange-1): | ||
| return true | ||
| case isBetween(e.Hostid+e.Maprange-1, i.Hostid, i.Hostid+i.Maprange-1): | ||
| return true | ||
| case isBetween(i.Hostid+i.Maprange-1, e.Hostid, e.Hostid+e.Maprange-1): | ||
| return true | ||
| case isBetween(e.Nsid, i.Nsid, i.Nsid+i.Maprange-1): | ||
| return true | ||
| case isBetween(i.Nsid, e.Nsid, e.Nsid+e.Maprange-1): | ||
| return true | ||
| case isBetween(e.Nsid+e.Maprange-1, i.Nsid, i.Nsid+i.Maprange-1): | ||
| return true | ||
| case isBetween(i.Nsid+i.Maprange-1, e.Nsid, e.Nsid+e.Maprange-1): | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
@tomponline we took this code from LXD, this comment looks relevant for you as well
8d2855b to
01c516c
Compare
|
Summary of changes since my last comment:
|
dmitry-lyfar
left a comment
There was a problem hiding this comment.
I need to finish tests review, these are the points, I've come across so far.
395810e to
b3be6dc
Compare
Initially this field doesn't do much; optionally users can add `confinement: container` to their workshop definition files. But soon we'll add support for `confinement: virtual-machine`.
Add support for LXD VMs in the backend, but keep them disabled in workshop definition files for now. VMs don't currently support SDKs, due to [1], so we even disable the system SDK. [1] canonical/lxd#18686
Without this, fsck.ext4 is likely to report something like this:
cloudimg-rootfs: recovering journal
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Free blocks count wrong (1843126, counted=1843130).
Fix? yes
Free inodes count wrong (1094312, counted=1094318).
Fix? yes
cloudimg-rootfs: ***** FILE SYSTEM WAS MODIFIED *****
cloudimg-rootfs: 85330/1179648 files (0.0% non-contiguous), 515905/2359035 blocks
VM instance templates are applied by the LXD agent, which is a systemd service and therefore can't modify the machine ID. When /etc/machine-id is missing, systemd creates one based on the SMBIOS UUID. For us, this means launchOrRebuildFromImage works fine. However, launchOrRebuildFromSnapshot doesn't. Since VMs only support SFTP while running, it's not easy to just delete /etc/machine-id after taking the snapshot. Instead we can tell systemd to always use the UUID via a kernel parameter. Since VM images use GRUB as a bootloader, the kernel command line needs to be configured through GRUB. We accomplish this with a custom GRUB script and drop-in config file.
ac1acf6 to
140352b
Compare
140352b to
8bbe22b
Compare
| return nil, err | ||
| } | ||
|
|
||
| idmapSet, err := workshopIdmap(file.Confinement, userid, groupid) |
There was a problem hiding this comment.
Can I ask you to summarise the idmap logic and problems we address with calculating idmaps here? Links to the existing issues work too.
There was a problem hiding this comment.
Problem it addresses
Containers and VMs have inconsistent idmaps by default. The ideal solution is to fix this problem in LXD: canonical/lxd#18875. But for now, this workaround does the same thing with some minor caveats.
I don't know of any real world cases that require consistency, but suppose you want to limit your agent to a certain subdirectory of /project/. If you try that with a VM before the idmap commit, you'll see something like this:
$ mkdir slop
$ chmod 777 slop
$ workshop exec -- sudo useradd -m agent
$ workshop exec -- sudo -u agent touch /project/slop/example.go
touch: cannot touch '/project/slop/example.go': Invalid argumentWhereas it works in containers.
I added a test, which should pass for both containers and VMs, here.
Summary of the logic
For containers, the outcome is the same as before (just different whitespace). The implementation differs in that we express the idmap for the workshop user and group as a []idmap.IdmapEntry and render it using formatIdMap.
For VMs, the outcome is an extended raw.idmap, but volatile.idmap.next should be the same as containers.
The logic to generate raw.idmap for VMs is modelled on how LXD computes the container idmap here, but:
- we skip parsing
raw.idmapand just start with the resulting list ofIdmapEntrys. - LXD merges this into a copy of
state.OS.IdmapSet.Idmap, we useidmap.KernelDefaultMap()instead.
When using the LXD snap, the results are identical:
state.OS.IdmapSet.Idmapis constructed usingidmap.GetIdmapSet().idmap.GetIdmapSet()is just some logging and error handling on top ofidmap.DefaultIdmapSet("", ""); we copy the error handling.idmap.DefaultIdmapSet("", "")returnsidmap.kernelDefaultMap()unless it finds/etc/subuidor/etc/subgid; these don't exist in the confined LXD snap.
If we wanted to support non-snap LXD installations, the most reliable way would be to call idmap.DefaultIdmapSet("/proc/<LXD PID>/root", "root"), but this would make it harder to run workshopd as an unprivileged user. Instead we could try to detect whether LXD is a snap or not; I opted not to bother implementing this because it's not a case we support anyway.
Another difference is that LXD also tries to extend raw.idmap for VMs here, but only does it when the existing idmap doesn't handle UID 0 or GID 0. I think this is possible, but it would require running LXD in a user namespace with lots of available UIDs and few available GIDs, or vice versa; I don't think it's likely to occur in practice.
Description
Introduces VM workshops: after running
snap set workshop workshop.experimental-vms=1, addconfinement: virtual-machineto the workshop definition file. Only limited functionality is supported (e.g. no SDKs).Major chages
workshop-waitready.service. This is installed in workshops launched or refreshed with Workshop 0.9.5 or later. Older workshops are still supported, but will be dropped in future. After we drop support,lxc startcan be run manually as a workaround.Freeze limitations
I think the
fsfreezeimplementation is slightly better than QEMU's guest agent, but has some of the same limitations: no ZFS support and potential timeout issues in obscure cases like mounting a loop file stored on a network drive. The latter is probably not worth worrying about; the former is something we could potentially support but there's no reason to right now. LXD VMs typically use ext4 for the / partition even when the block device is backed by ZFS.Minor changes
--vmflag toworkshop init.confinementinworkshop infooutput.stderrbuffer.tests/main/launchandtests/integration/workshop-snapshotsinto multiple spread variants. This is a step towards enabling them for VMs, but we need to clean up snapshots more aggressively or the GitHub runners will run out of space.Self-review quick check
Docs
Procedure:
Content:
tutorial/andhow-to/sections).docs/.coverage.yamlupdated, coverage tags added (.. artefact).Or: