fix(instances): a UEFI clone gets its own NVRAM without a new pool

Cloning a UEFI VM defined a persistent storage pool on the NVRAM's
directory when no pool held it, and dropped <nvram> from the clone,
which found its copy only because libvirt chose the same file name.
The copy is now made through the NVRAM directory's pool (a transient one
when there is none) or the pool that holds the file, with the original's
format, owner and mode, and the clone's definition names it. A failed
definition removes it with the copied disks. An NVRAM that is not a file
is refused before anything is copied.

README.md

@@ -539,6 +539,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat
 - An ISO uploads to an SSH compute about 3.5 times faster (SFTP writes no longer wait for each 32 KiB); a failed write still fails the upload.
 - The VM page shows each NIC's own QoS and each CD-ROM's own pool (they showed the previous device's), and a NIC without a source or model no longer breaks it.
 - A volume of an LVM or iSCSI pool can be attached to a VM, and bridge and direct NICs can be boot devices. The NIC model `default` lets libvirt choose, and `rt18139` is `rtl8139`: a VM with either did not start; a migration corrects the default NIC type setting.
+- Cloning a UEFI VM no longer defines a storage pool on the host's NVRAM directory; the clone gets its own copy of the VM's UEFI variables.
 - A running VM without a guest agent shows the IPv4 address the host learned by ARP (the lookup always failed before).
 - After a live migration the VM's NVRAM file is deleted on the source, when it is in libvirt's `/var/lib/libvirt/qemu/nvram` and the destination does not share that directory (a VM created there later with the same name would start with those UEFI variables).
 - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3).

instances/test_live_clone.py

@@ -2,6 +2,7 @@
 
 import os
 import unittest
+from xml.etree import ElementTree
 
 import libvirt
 from django.test import SimpleTestCase
@@ -84,6 +85,30 @@ class LiveCloneTestCase(SimpleTestCase):
 
         self.assertEqual(self.pool_volumes(), before)
 
+    def test_clone_of_a_uefi_vm_has_its_own_copy_of_the_nvram(self):
+        disk = livetest.create_volume(self.conn, P + "clone-uefi-a")
+        dom = livetest.define_vm(self.conn, P + "clone-uefi", [disk], uefi=True)
+        dom.createWithFlags(libvirt.VIR_DOMAIN_START_PAUSED)  # libvirt makes the NVRAM file at the first start
+        dom.destroy()
+        vm = wvmInstance(*self.args, None, uuid=dom.UUIDString())
+        source = vm.get_nvram()
+
+        uuid = vm.clone_instance(
+            {"name": P + "clone-uefi-copy", "disk-vda": P + "clone-uefi-copy-a.qcow2", "disk_owner_uid": 0, "disk_owner_gid": 0}
+        )
+
+        clone = self.conn.lookupByUUIDString(uuid)
+        path = ElementTree.fromstring(clone.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)).findtext("os/nvram")
+        self.assertEqual(path, os.path.join(os.path.dirname(source), P + "clone-uefi-copy_VARS.fd"))
+        with vm._nvram_pool() as pool:
+            copy = pool.storageVolLookupByName(os.path.basename(path))
+            self.assertEqual(copy.info()[1], pool.storageVolLookupByName(os.path.basename(source)).info()[1])
+        clone.createWithFlags(libvirt.VIR_DOMAIN_START_PAUSED)  # qemu opens the copy
+        self.assertEqual(
+            ElementTree.fromstring(clone.XMLDesc(0)).findtext("os/nvram"), path, "libvirt replaced the copy"
+        )
+        clone.destroy()
+
     def test_clone_of_a_stopped_vm(self):
         _, vm = self.source("clone-ok")
         uuid = vm.clone_instance(self.clone_data("clone-ok-copy"))

vrtManager/connection.py

@@ -906,7 +906,9 @@ class wvmConnect(object):
         try:
             yield pool
         finally:
-            pool.destroy()
+            # a transient pool left by a failed destroy goes with libvirtd's next restart
+            with contextlib.suppress(libvirtError):
+                pool.destroy()
 
     @staticmethod
     def _find_volume(pool, name):

vrtManager/instance.py

@@ -53,7 +53,7 @@ from xml.etree import ElementTree
 from lxml import etree
 
 from vrtManager import util
-from vrtManager.connection import wvmConnect
+from vrtManager.connection import NVRAM_DIR, wvmConnect
 from vrtManager.storage import wvmStorage, wvmStorages
 
 # Edits that redefine the domain start from this: the persistent definition,
@@ -1794,8 +1794,9 @@ class wvmInstance(wvmConnect):
         return ":".join(mac_tuples)
 
     def clone_instance(self, clone_data):
-        """Copy a shut-off VM. Inputs are checked before anything is allocated,
-        and on failure only the volumes this call created are removed."""
+        """Copy a shut-off VM, with its disks and UEFI variables (NVRAM).
+        Inputs are checked before anything is allocated, and on failure only
+        the volumes this call created are removed. Returns the clone's UUID."""
         if self.get_status() != 5:
             # A running guest keeps writing while its disks are copied.
             raise util.OperationError("Shut the VM down before cloning it")
@@ -1839,25 +1840,17 @@ class wvmInstance(wvmConnect):
                 raise ValueError(f"No MAC address for network interface {num}")
             net.find("mac").set("address", self.fix_mac(mac))
 
+        # the UEFI variables: a path as text, or a <source file=...> since libvirt 8.5
+        nvram = tree.find("os/nvram")
+        nvram_source = nvram.find("source") if nvram is not None else None
+        nvram_path = None
+        if nvram is not None:
+            if nvram.get("type", "file") != "file":
+                raise util.OperationError("Cannot clone a VM whose NVRAM is not a file")
+            nvram_path = nvram_source.get("file") if nvram_source is not None else (nvram.text or "").strip()
+
         created = []  # paths of volumes this call created
         try:
-            src_nvram_path = self.get_nvram()
-            if src_nvram_path:
-                nvram = tree.find("os/nvram")
-                nvram.getparent().remove(nvram)
-
-                # NVRAM CLONE: create pool if nvram is not in a pool. then clone it
-                src_nvram_name = os.path.basename(src_nvram_path)
-                nvram_dir = os.path.dirname(src_nvram_path)
-                nvram_pool_name = os.path.basename(nvram_dir)
-                try:
-                    self.get_volume_by_path(src_nvram_path)
-                except libvirtError:
-                    self.get_wvmStorages().create_storage("dir", nvram_pool_name, None, nvram_dir)
-                nvram_stg = self.get_wvmStorage(nvram_pool_name)
-                name = nvram_stg.clone_volume(src_nvram_name, f"{clone_data['name']}_VARS", file_suffix="fd")
-                created.append(os.path.join(nvram_dir, name))
-
             for source, dev in disks:
                 target_file = clone_data["disk-" + dev]
                 meta_prealloc = bool(clone_data.get("meta-" + dev))
@@ -1910,7 +1903,39 @@ class wvmInstance(wvmConnect):
                     created.append(new_path)
                     source.set("dev", new_path)
 
-            self._defineXML(etree.tostring(tree).decode())
+            if not nvram_path:
+                clone = self._defineXML(etree.tostring(tree).decode())
+            else:
+                # the clone starts with the VM's UEFI variables: boot entries, enrolled keys
+                if os.path.dirname(nvram_path) == NVRAM_DIR:
+                    pools = self._nvram_pool()
+                else:  # a volume of a pool; libvirtError when no pool holds it
+                    pools = contextlib.nullcontext(self.get_volume_by_path(nvram_path).storagePoolLookupByVolume())
+                with pools as pool:
+                    original = pool.storageVolLookupByName(os.path.basename(nvram_path))
+                    # same format, owner and mode as the original: the domain names the format,
+                    # and qemu may run without libvirt changing the file's owner
+                    target = etree.fromstring(original.XMLDesc(0)).find("target")
+                    volume = etree.Element("volume")
+                    etree.SubElement(volume, "name").text = clone_data["name"] + "_VARS" + os.path.splitext(nvram_path)[1]
+                    etree.SubElement(volume, "capacity").text = "0"
+                    copy_target = etree.SubElement(volume, "target")
+                    for tag in ("format", "permissions"):
+                        if target.find(tag) is not None:
+                            copy_target.append(target.find(tag))
+                    copy = pool.createXMLFrom(etree.tostring(volume).decode(), original, 0)
+                    if nvram_source is not None:
+                        nvram_source.set("file", copy.path())
+                    else:
+                        nvram.text = copy.path()
+                    try:
+                        clone = self._defineXML(etree.tostring(tree).decode())
+                    except Exception:
+                        try:
+                            copy.delete(0)
+                        except libvirtError:
+                            created.append(copy.path())
+                        raise
         except Exception as err:
             leftovers = []
             for path in reversed(created):
@@ -1922,7 +1947,7 @@ class wvmInstance(wvmConnect):
                 raise util.OperationError(f"Clone failed ({err}); remove these copies by hand: {', '.join(leftovers)}") from err
             raise
 
-        return self.get_instance(clone_data["name"]).UUIDString()
+        return clone.UUIDString()
 
     def get_bridge_name(self, source, source_type="net"):
         if source_type == "iface":

vrtManager/test_clone_nvram.py

@@ -0,0 +1,68 @@
+"""A clone of a UEFI VM gets its own copy of the VM's NVRAM, with the
+original's format, owner and mode, wherever the definition names the file."""
+
+import contextlib
+import unittest
+from unittest.mock import MagicMock
+
+from django.conf import settings
+
+if not settings.configured:
+    settings.configure(MAC_OUI="52:54:10")
+
+from lxml import etree
+
+from vrtManager import util
+from vrtManager.instance import wvmInstance
+
+ORIGINAL = """<volume><name>vm_VARS.qcow2</name><key>k</key><capacity>540672</capacity>
+<target><path>/var/lib/libvirt/qemu/nvram/vm_VARS.qcow2</path><format type='qcow2'/>
+<permissions><mode>0600</mode><owner>107</owner><group>107</group></permissions></target></volume>"""
+
+
+def domain(nvram):
+    return f"""<domain><name>vm</name><uuid>u</uuid><os><type>hvm</type>
+<loader readonly='yes' type='pflash'>/usr/share/edk2/ovmf/OVMF_CODE.fd</loader>{nvram}</os><devices/></domain>"""
+
+
+def proxy(nvram):
+    p = wvmInstance.__new__(wvmInstance)  # no connection: only these calls
+    p.get_status = lambda: 5
+    p.get_instances = lambda: []
+    p._XMLDesc = lambda flags: domain(nvram)
+    p._defineXML = MagicMock()
+    p.pool = MagicMock()
+    p.pool.storageVolLookupByName.return_value.XMLDesc.return_value = ORIGINAL
+    p.pool.createXMLFrom.return_value.path.return_value = "/var/lib/libvirt/qemu/nvram/copy_VARS.qcow2"
+    p._nvram_pool = lambda: contextlib.nullcontext(p.pool)
+    return p
+
+
+class CloneNvramTestCase(unittest.TestCase):
+    def test_the_copy_keeps_the_format_owner_and_mode(self):
+        p = proxy("<nvram format='qcow2'>/var/lib/libvirt/qemu/nvram/vm_VARS.qcow2</nvram>")
+        p.clone_instance({"name": "copy"})
+        xml, original, _ = p.pool.createXMLFrom.call_args.args
+        volume = etree.fromstring(xml)
+        self.assertEqual(volume.findtext("name"), "copy_VARS.qcow2")
+        self.assertEqual(volume.find("target/format").get("type"), "qcow2")
+        self.assertEqual(volume.findtext("target/permissions/owner"), "107")
+        self.assertIsNone(volume.find("target/path"))
+        self.assertIs(original, p.pool.storageVolLookupByName.return_value)
+        (defined,), _ = p._defineXML.call_args
+        self.assertEqual(etree.fromstring(defined).findtext("os/nvram"), "/var/lib/libvirt/qemu/nvram/copy_VARS.qcow2")
+
+    def test_an_nvram_source_element_points_at_the_copy(self):
+        p = proxy("<nvram type='file'>\n  <source file='/var/lib/libvirt/qemu/nvram/vm_VARS.qcow2'/>\n</nvram>")
+        p.clone_instance({"name": "copy"})
+        p.pool.storageVolLookupByName.assert_called_once_with("vm_VARS.qcow2")
+        (defined,), _ = p._defineXML.call_args
+        self.assertEqual(
+            etree.fromstring(defined).find("os/nvram/source").get("file"), "/var/lib/libvirt/qemu/nvram/copy_VARS.qcow2"
+        )
+
+    def test_an_nvram_that_is_not_a_file_is_refused_before_any_copy(self):
+        p = proxy("<nvram type='network'><source protocol='iscsi' name='x'/></nvram>")
+        with self.assertRaises(util.OperationError):
+            p.clone_instance({"name": "copy"})
+        p.pool.createXMLFrom.assert_not_called()