docs/plans-executions/2026-09-04-1404-kvm-provider.md

@@ -69,6 +69,9 @@ Two design choices differ from the plan text:
   `<disk type="volume"><source pool=... volume=.../>`, so only the *base*
   image path is ever looked up (the overlay's `<backingStore>` needs a real
   path). Fewer RPCs and the builder's inputs are all names.
+  **Reverted in Step 7**: volume-referenced disks cannot start on an
+  AppArmor host, see there. The domain XML is back to `type="file"` with
+  the two `StorageVolGetPath` calls the plan had.
 - **Capacities are always `unit="bytes"`.** libvirt's `G` means GiB but
   `GB` means 10^9; spelling the byte count out removes the ambiguity from
   both the overlay and the seed/base volumes.
@@ -389,3 +392,56 @@ Worth noting: the CRD has no rule requiring all three placement fields
 behaviour works without an API change. And with this step the branch is
 feature-complete but **unproven**: Step 7 runs the integration test and
 the end-to-end checklist against a real hypervisor before the MR.
+
+## Step 7 — Manual validation + MR (in progress)
+
+Host: `beelink` (Ubuntu, libvirt 11.6.0, QEMU 10.1.0, pool `default` at
+`/srv/vms/images` on local ext4, bridge `br0`), reached over Tailscale as
+`beelink.tail8c69c.ts.net`, SSH user `novakj`. Not the `nas1.home.lan`
+the plan and samples name; that host does not resolve from the dev box.
+
+```bash
+export KVM_TEST_HOST=beelink.tail8c69c.ts.net:22
+export KVM_TEST_USER=novakj
+export KVM_TEST_SSH_KEY=$HOME/.ssh/proxy-operator_ed25519
+export KVM_TEST_KNOWN_HOSTS=$HOME/.ssh/kvm_known_hosts
+export KVM_TEST_BRIDGE=br0
+export KVM_TEST_IMAGE_URL=https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2
+make test-integration-kvm
+```
+
+### Run 1 and 2: `Could not open '/srv/vms/images/base-test-image.qcow2': Permission denied`
+
+Everything up to `DomainCreate` worked on the first try against a real
+host, which answers the open notes from Steps 2 and 3b: the SSH-forwarded
+libvirt handshake, the raw qcow2 upload plus pool refresh as a backing
+image, the default-namespace metadata element, and `DomainDefineXML` for
+the generated XML were all accepted by libvirt 11.6. QEMU then exited at
+start with `Permission denied` on the base image.
+
+Ownership was ruled out the slow way: the file was `root:root 0600` (the
+dir-pool default; libvirt's DAC driver chowns the chain at start and
+restores it after a failed start, so that observation meant nothing),
+`qemu.conf` was all defaults (`libvirt-qemu`, `dynamic_ownership=1`), and
+after a manual `chown libvirt-qemu:kvm` the second run failed identically.
+
+The actual cause is the Step 2 choice of `<disk type="volume">`.
+libvirt's AppArmor helper builds each domain's profile from the domain XML
+without consulting the storage driver, and its `add_file_path` only emits a
+rule when the source has a `path`; a volume-referenced disk has pool and
+volume names instead, so neither the overlay nor its backing image gets a
+rule and QEMU is denied on the first one it opens. Every stock Debian and
+Ubuntu hypervisor confines QEMU this way. The unit tests could not have
+caught it: the fake accepts either form.
+
+Fix, in this step: `buildDomainXML` takes `DiskPath`/`SeedPath` and emits
+`type="file"` disks; `ensureOverlay` and `writeSeed` return the volume
+handles (the exists-already branch now does a `StorageVolLookupByName`),
+and Create resolves both paths with `StorageVolGetPath` before defining
+the domain. Two extra cheap RPCs per Create. `TestBuildDomainXML_fieldByField`
+now fails on any `type="volume"` or `pool=` attribute so the design cannot
+drift back, and `TestCreate_freshHost` asserts the disk sources equal the
+paths the fake reports.
+
+Worth noting: the first two runs left the base image and its marker in
+the pool as designed, so later runs skip the 435 MB download.

internal/provider/kvm/domainxml.go

@@ -9,24 +9,29 @@ import (
 const guestAgentChannel = "org.qemu.guest_agent.0"
 
 // domainParams is everything buildDomainXML needs, already resolved from the
-// config and placement so the builder stays a pure function.
+// config and placement so the builder stays a pure function. DiskPath and
+// SeedPath are the volumes' host paths as reported by libvirt.
 type domainParams struct {
 	Name      string
 	UID       string
 	CreatedAt time.Time
 	VCPUs     int32
 	MemoryMiB int64
-	Pool      string
 	Bridge    string
-	DiskVol   string
-	SeedVol   string
+	DiskPath  string
+	SeedPath  string
 }
 
 // buildDomainXML renders a q35 KVM guest: host-passthrough CPU, a virtio
-// overlay disk and a SATA cdrom for the seed (both referenced as pool
-// volumes so no host paths are needed), one virtio NIC on the host bridge,
-// a serial console, the guest-agent channel, a virtio RNG, no graphics, and
-// the operator's metadata element.
+// overlay disk and a SATA cdrom for the seed, one virtio NIC on the host
+// bridge, a serial console, the guest-agent channel, a virtio RNG, no
+// graphics, and the operator's metadata element.
+//
+// Disks are type="file" with host paths, not type="volume" with pool/volume
+// names: libvirt's AppArmor helper (virt-aa-helper) only grants QEMU access
+// to sources that carry a path, so a volume-referenced disk fails to start
+// with "Permission denied" on any AppArmor-confined host (stock Debian and
+// Ubuntu). The paths cost one StorageVolGetPath round-trip each per Create.
 func buildDomainXML(p domainParams) (string, error) {
 	d := domainXML{
 		Type:     "kvm",
@@ -47,17 +52,17 @@ func buildDomainXML(p domainParams) (string, error) {
 		Devices: devicesXML{
 			Disks: []diskXML{
 				{
-					Type:   "volume",
+					Type:   "file",
 					Device: "disk",
 					Driver: driverXML{Name: "qemu", Type: formatQcow2},
-					Source: diskSourceXML{Pool: p.Pool, Volume: p.DiskVol},
+					Source: diskSourceXML{File: p.DiskPath},
 					Target: targetXML{Dev: "vda", Bus: "virtio"},
 				},
 				{
-					Type:     "volume",
+					Type:     "file",
 					Device:   "cdrom",
 					Driver:   driverXML{Name: "qemu", Type: formatRaw},
-					Source:   diskSourceXML{Pool: p.Pool, Volume: p.SeedVol},
+					Source:   diskSourceXML{File: p.SeedPath},
 					Target:   targetXML{Dev: "sda", Bus: "sata"},
 					ReadOnly: &struct{}{},
 				},
@@ -161,8 +166,7 @@ type driverXML struct {
 }
 
 type diskSourceXML struct {
-	Pool   string `xml:"pool,attr"`
-	Volume string `xml:"volume,attr"`
+	File string `xml:"file,attr"`
 }
 
 type targetXML struct {

internal/provider/kvm/domainxml_test.go

@@ -14,10 +14,9 @@ func testDomainParams() domainParams {
 		CreatedAt: time.Date(2026, 9, 4, 12, 0, 0, 0, time.UTC),
 		VCPUs:     2,
 		MemoryMiB: 2048,
-		Pool:      "default",
 		Bridge:    "br0",
-		DiskVol:   "proxy-abc123def456ghij.qcow2",
-		SeedVol:   "proxy-abc123def456ghij-seed.iso",
+		DiskPath:  "/var/lib/libvirt/images/proxy-abc123def456ghij.qcow2",
+		SeedPath:  "/var/lib/libvirt/images/proxy-abc123def456ghij-seed.iso",
 	}
 }
 
@@ -63,16 +62,19 @@ func TestBuildDomainXML_fieldByField(t *testing.T) {
 		t.Fatalf("disks = %d, want 2", len(d.Devices.Disks))
 	}
 	disk, seed := d.Devices.Disks[0], d.Devices.Disks[1]
-	if disk.Type != "volume" || disk.Device != "disk" || disk.Driver.Type != "qcow2" ||
-		disk.Source.Pool != "default" || disk.Source.Volume != "proxy-abc123def456ghij.qcow2" ||
+	if disk.Type != "file" || disk.Device != "disk" || disk.Driver.Type != "qcow2" ||
+		disk.Source.File != "/var/lib/libvirt/images/proxy-abc123def456ghij.qcow2" ||
 		disk.Target.Dev != "vda" || disk.Target.Bus != "virtio" || disk.ReadOnly != nil {
 		t.Errorf("overlay disk = %+v", disk)
 	}
-	if seed.Type != "volume" || seed.Device != "cdrom" || seed.Driver.Type != "raw" ||
-		seed.Source.Pool != "default" || seed.Source.Volume != "proxy-abc123def456ghij-seed.iso" ||
+	if seed.Type != "file" || seed.Device != "cdrom" || seed.Driver.Type != "raw" ||
+		seed.Source.File != "/var/lib/libvirt/images/proxy-abc123def456ghij-seed.iso" ||
 		seed.Target.Bus != "sata" || seed.ReadOnly == nil {
 		t.Errorf("seed cdrom = %+v", seed)
 	}
+	if strings.Contains(raw, `type="volume"`) || strings.Contains(raw, " pool=") {
+		t.Errorf("disks reference pool volumes; virt-aa-helper cannot grant QEMU access to those:\n%s", raw)
+	}
 
 	if len(d.Devices.Interfaces) != 1 {
 		t.Fatalf("interfaces = %d, want 1", len(d.Devices.Interfaces))

internal/provider/kvm/kvm.go

@@ -183,13 +183,23 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri
 		if err != nil {
 			return err
 		}
-		if err := p.ensureOverlay(log, api, pool, req.Name, basePath, src.Format); err != nil {
+		overlay, err := p.ensureOverlay(log, api, pool, req.Name, basePath, src.Format)
+		if err != nil {
 			return err
 		}
-		if err := p.writeSeed(log, api, pool, req.Name, req.CloudInit); err != nil {
+		seed, err := p.writeSeed(log, api, pool, req.Name, req.CloudInit)
+		if err != nil {
 			return err
 		}
-		dom, err = p.defineDomain(log, api, host, req, mt)
+		diskPath, err := api.StorageVolGetPath(overlay)
+		if err != nil {
+			return fmt.Errorf("resolving overlay path: %w", err)
+		}
+		seedPath, err := api.StorageVolGetPath(seed)
+		if err != nil {
+			return fmt.Errorf("resolving seed path: %w", err)
+		}
+		dom, err = p.defineDomain(log, api, host, req, mt, diskPath, seedPath)
 		if err != nil {
 			return err
 		}
@@ -203,60 +213,66 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri
 	return id, nil
 }
 
-func (p *Provider) ensureOverlay(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, basePath, baseFormat string) error {
+// ensureOverlay creates the per-VM overlay volume, or returns the existing
+// one: creation is atomic, so an overlay that exists is complete.
+func (p *Provider) ensureOverlay(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, basePath, baseFormat string) (libvirt.StorageVol, error) {
 	xml, err := buildVolumeXML(overlayVolume(name, p.cfg.DiskSizeGB, basePath, baseFormat))
 	if err != nil {
-		return err
+		return libvirt.StorageVol{}, err
 	}
-	_, err = api.StorageVolCreateXML(pool, xml, 0)
+	vol, err := api.StorageVolCreateXML(pool, xml, 0)
 	switch {
 	case err == nil:
 		log.V(1).Info("overlay volume created", "volume", overlayVolumeName(name), "base", basePath)
+		return vol, nil
 	case hasCode(err, libvirt.ErrStorageVolExist):
 		log.V(1).Info("overlay volume already exists", "volume", overlayVolumeName(name))
+		vol, err := api.StorageVolLookupByName(pool, overlayVolumeName(name))
+		if err != nil {
+			return libvirt.StorageVol{}, fmt.Errorf("looking up existing overlay volume: %w", err)
+		}
+		return vol, nil
 	default:
-		return fmt.Errorf("creating overlay volume: %w", err)
+		return libvirt.StorageVol{}, fmt.Errorf("creating overlay volume: %w", err)
 	}
-	return nil
 }
 
 // writeSeed replaces the seed volume with a freshly built NoCloud ISO.
 // Only called while no domain references it.
-func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, userData string) error {
+func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, userData string) (libvirt.StorageVol, error) {
 	seedName := seedVolumeName(name)
 	if err := deleteIfExists(api, pool, seedName); err != nil {
-		return err
+		return libvirt.StorageVol{}, err
 	}
 	iso, err := buildSeedISO(name, userData, p.cfg.VendorData)
 	if err != nil {
-		return fmt.Errorf("%w: %v", provider.ErrPermanent, err)
+		return libvirt.StorageVol{}, fmt.Errorf("%w: %v", provider.ErrPermanent, err)
 	}
 	xml, err := buildVolumeXML(rawVolume(seedName, int64(len(iso))))
 	if err != nil {
-		return err
+		return libvirt.StorageVol{}, err
 	}
 	vol, err := api.StorageVolCreateXML(pool, xml, 0)
 	if err != nil {
-		return fmt.Errorf("creating seed volume: %w", err)
+		return libvirt.StorageVol{}, fmt.Errorf("creating seed volume: %w", err)
 	}
 	if err := api.StorageVolUpload(vol, bytes.NewReader(iso), 0, 0, 0); err != nil {
-		return fmt.Errorf("uploading seed volume: %w", err)
+		return libvirt.StorageVol{}, fmt.Errorf("uploading seed volume: %w", err)
 	}
 	log.V(1).Info("seed volume uploaded", "volume", seedName, "bytes", len(iso))
-	return nil
+	return vol, nil
 }
 
-func (p *Provider) defineDomain(log logr.Logger, api libvirtAPI, host provider.KVMHost, req provider.CreateRequest, mt provider.KVMMachineType) (libvirt.Domain, error) {
+func (p *Provider) defineDomain(log logr.Logger, api libvirtAPI, host provider.KVMHost, req provider.CreateRequest, mt provider.KVMMachineType, diskPath, seedPath string) (libvirt.Domain, error) {
 	xml, err := buildDomainXML(domainParams{
 		Name:      req.Name,
 		UID:       req.UID,
 		CreatedAt: p.now(),
 		VCPUs:     mt.VCPUs,
 		MemoryMiB: mt.MemoryMiB,
-		Pool:      host.Pool,
 		Bridge:    host.Bridge,
-		DiskVol:   overlayVolumeName(req.Name),
-		SeedVol:   seedVolumeName(req.Name),
+		DiskPath:  diskPath,
+		SeedPath:  seedPath,
 	})
 	if err != nil {
 		return libvirt.Domain{}, err

internal/provider/kvm/kvm_test.go

@@ -210,6 +210,13 @@ func TestCreate_freshHost(t *testing.T) {
 	if d.Devices.Interfaces[0].Source.Bridge != "br0" {
 		t.Errorf("bridge = %q, want br0", d.Devices.Interfaces[0].Source.Bridge)
 	}
+	wantDisk, wantSeed := api.poolPath+"/"+overlayVolumeName(testName), api.poolPath+"/"+seedVolumeName(testName)
+	if got := d.Devices.Disks[0].Source.File; got != wantDisk {
+		t.Errorf("overlay disk source = %q, want the path libvirt reported, %q", got, wantDisk)
+	}
+	if got := d.Devices.Disks[1].Source.File; got != wantSeed {
+		t.Errorf("seed cdrom source = %q, want the path libvirt reported, %q", got, wantSeed)
+	}
 	if m := d.Metadata.Instance; m.UID != testUID || !m.managed() || !m.createdAt().Equal(testNow) {
 		t.Errorf("metadata = %+v, want managed, uid %s, createdAt %s", m, testUID, testNow)
 	}