feat(instances): a shut-off UEFI VM can be migrated

The move copies the VM's NVRAM file to the destination with libvirt
streams (unless both hosts share the NVRAM directory) and removes the
copy if the move fails; the source's file goes with the existing
cleanup. The undefine keeps the file, as libvirt refuses to undefine a
VM with NVRAM otherwise. A VM with TPM state is still refused.

vrtManager/connection.py

@@ -869,29 +869,34 @@ class wvmConnect(object):
         whether a file was deleted."""
         if os.path.dirname(path) != NVRAM_DIR:
             return False
-        marker = (
-            f"<volume><name>webvirtcloud-check-{uuid.uuid4().hex}</name><capacity>0</capacity>"
-            "<target><format type='raw'/></target></volume>"
-        )
-        with self._nvram_pool() as pool:
-            mine = pool.createXML(marker, 0)
-            try:
-                # Creating the same file on destination fails when both see one
-                # directory: the create is exclusive on the file server, unlike
-                # a directory listing, which an NFS client may cache.
-                with destination._nvram_pool() as other:
-                    try:
-                        other.createXML(marker, 0).delete(0)
-                    except libvirtError:
-                        return False  # shared, or not known to be local
-            finally:
-                mine.delete(0)
+        with self._nvram_pool() as pool, destination._nvram_pool() as other:
+            if self._nvram_shared(pool, other):
+                return False
             vol = self._find_volume(pool, os.path.basename(path))
             if vol is None:
                 return False
             vol.delete(0)
             return True
 
+    @staticmethod
+    def _nvram_shared(pool, other):
+        """Whether pools on NVRAM_DIR of two hosts are one directory, or other
+        is not known to be one of its own: creating the same file in both
+        fails then. The create is exclusive on the file server, unlike a
+        directory listing, which an NFS client may cache."""
+        marker = (
+            f"<volume><name>webvirtcloud-check-{uuid.uuid4().hex}</name><capacity>0</capacity>"
+            "<target><format type='raw'/></target></volume>"
+        )
+        mine = pool.createXML(marker, 0)
+        try:
+            other.createXML(marker, 0).delete(0)
+        except libvirtError:
+            return True
+        finally:
+            mine.delete(0)
+        return False
+
     @contextlib.contextmanager
     def _nvram_pool(self):
         """An active, refreshed pool on NVRAM_DIR: libvirt reads and deletes

vrtManager/instance.py

@@ -1,4 +1,5 @@
 import contextlib
+import io
 import ipaddress
 import json
 import logging
@@ -18,6 +19,7 @@ try:
         VIR_DOMAIN_XML_SECURE,
         VIR_DOMAIN_XML_INACTIVE,
         VIR_DOMAIN_XML_MIGRATABLE,
+        VIR_DOMAIN_UNDEFINE_KEEP_NVRAM,
         VIR_ERR_NO_DOMAIN,
         VIR_ERR_NO_STORAGE_POOL,
         VIR_ERR_NO_STORAGE_VOL,
@@ -158,30 +160,65 @@ class wvmInstances(wvmConnect):
             # Once supported hosts all run a fixed libvirt, this branch and
             # move_definition can go: the flags above are those for
             # dom.migrate.
-            self.move_definition(dom)
+            self.move_definition(dom, conn)
         else:
             dom.migrate(self.wvm, flags, None, uri, 0)
         return ", ".join(mode)
 
     @staticmethod
     def refuse_local_state(xml):
-        """A shut-off VM moves without its UEFI variables (NVRAM) and TPM
-        state, which libvirt keeps in files of the source host: on the
-        destination it would start with new ones (lost boot entries and
-        enrolled keys, a new TPM that no longer unlocks disks). A live
-        migration carries both."""
-        tree = ElementTree.fromstring(xml)
-        state = []
-        if tree.find("os/nvram") is not None:
-            state.append("NVRAM")
-        if tree.find("devices/tpm/backend[@type='emulator']") is not None:
-            state.append("TPM")
-        if state:
+        """A shut-off VM moves without its TPM state, a file of the source
+        host: on the destination it would get a new TPM that no longer
+        unlocks its disks. A live migration carries it. (The UEFI variables
+        are copied: copy_nvram.)"""
+        if ElementTree.fromstring(xml).find("devices/tpm/backend[@type='emulator']") is not None:
             raise util.OperationError(
-                f"A shut-off VM with {' and '.join(state)} state cannot be migrated: the state stays on the source "
-                "host. Start the VM and migrate it live."
+                "A shut-off VM with TPM state cannot be migrated: the state stays on the source host. "
+                "Start the VM and migrate it live."
             )
 
+    def copy_nvram(self, conn):
+        """Copies the UEFI variables file of the shut-off VM of conn (on the
+        source) from libvirt's NVRAM directory there to the same path here,
+        unless both hosts share that directory. Returns the copy's name."""
+        path = conn.get_nvram()
+        if not path or os.path.dirname(path) != NVRAM_DIR:
+            return None
+        name = os.path.basename(path)
+        with conn._nvram_pool() as source, self._nvram_pool() as target:
+            vol = self._find_volume(source, name)
+            if vol is None:
+                return None  # never started: libvirt creates it at the first start
+            if self._nvram_shared(source, target):
+                if self._find_volume(target, name) is None:
+                    raise util.OperationError(f"The destination host cannot see the NVRAM file {path}")
+                return None
+            chunks = []
+            stream = conn.wvm.newStream(0)
+            vol.download(stream, 0, 0, 0)
+            stream.recvAll(lambda _stream, data, out: out.append(data), chunks)
+            stream.finish()
+            data = b"".join(chunks)
+            copy = target.createXML(
+                f"<volume><name>{util.xml_escape(name)}</name><capacity>{len(data)}</capacity>"
+                "<target><format type='raw'/></target></volume>",
+                0,
+            )
+            try:
+                stream = self.wvm.newStream(0)
+                copy.upload(stream, 0, len(data), 0)
+                stream.sendAll(lambda _stream, size, buffer: buffer.read(size), io.BytesIO(data))
+                stream.finish()
+            except Exception:
+                copy.delete(0)
+                raise
+        return name
+
+    def delete_nvram(self, name):
+        if name:
+            with self._nvram_pool() as pool:
+                pool.storageVolLookupByName(name).delete(0)
+
     def missing_disks(self, definitions):
         """(target, source) of the disks in the VM definitions (XML) that are
         not volumes of this host's active storage pools: the VM's own files,
@@ -252,10 +289,11 @@ class wvmInstances(wvmConnect):
                 return None
             raise
 
-    def move_definition(self, dom):
+    def move_definition(self, dom, conn):
         """Migrates a shut-off VM: that is only its definition, as the disks
-        are on storage both hosts share (moveto refuses a VM with NVRAM or
-        TPM state). The destination's qemu migration hook does not run for it.
+        are on storage both hosts share, and its UEFI variables file
+        (copy_nvram; moveto refuses a VM with TPM state). The destination's
+        qemu migration hook does not run for it.
 
         Workaround, to be removed in a later version: libvirt's own offline
         migration (VIR_MIGRATE_OFFLINE) fails with "operation failed: domain
@@ -278,9 +316,15 @@ class wvmInstances(wvmConnect):
 
         xml = dom.XMLDesc(VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_MIGRATABLE)
         autostart = dom.autostart()
-        new = self.wvm.defineXML(xml)
+        nvram = self.copy_nvram(conn)
         try:
-            dom.undefineFlags(0)
+            new = self.wvm.defineXML(xml)
+        except Exception:
+            self.delete_nvram(nvram)
+            raise
+        try:
+            # libvirt refuses to undefine a VM with NVRAM without this; the file stays for the cleanup
+            dom.undefineFlags(VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)
             try:
                 started = dom.isActive()
             except libvirtError as error:
@@ -292,7 +336,8 @@ class wvmInstances(wvmConnect):
                 dom.connect().defineXML(xml).setAutostart(autostart)
                 raise util.OperationError("The VM was started on the source host during the migration")
         except Exception as error:
-            self.undo_definition(dom, new, error)
+            self.undo_definition(dom, new, error)  # raises when the destination keeps the VM
+            self.delete_nvram(nvram)
             raise
 
     @staticmethod
@@ -308,7 +353,7 @@ class wvmInstances(wvmConnect):
                 f"{error}; the VM may now be defined only on the destination host: check both hosts"
             ) from error
         try:
-            new.undefineFlags(0)
+            new.undefineFlags(VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)
         except libvirtError as undo_error:
             raise util.OperationError(
                 f"{error}; the VM is now defined on both hosts, remove it from the destination: {undo_error}"
@@ -447,7 +492,8 @@ class wvmInstance(wvmConnect):
         return util.get_xml_path(self._XMLDesc(0), "/domain/devices/emulator")
 
     def get_nvram(self):
-        return util.get_xml_path(self._XMLDesc(0), "/domain/os/nvram")
+        xml = self._XMLDesc(0)  # a path as text, or a <source file=...> since libvirt 8.5
+        return util.get_xml_path(xml, "/domain/os/nvram/source/@file") or util.get_xml_path(xml, "/domain/os/nvram")
 
     def get_loader(self):
         xml = self._XMLDesc(0)

vrtManager/tests/test_migration.py

@@ -1,7 +1,9 @@
 """Migration: the source definition never stays behind; a shut-off VM's
 definition is moved by the app, not by libvirt's offline migration."""
 
+import contextlib
 import unittest
+from unittest import mock
 
 import libvirt
 from django.conf import settings
@@ -156,10 +158,14 @@ class FakeHost:
 
 
 class FakeSource:
-    def __init__(self, state, **domain):
+    def __init__(self, state, nvram=None, **domain):
         self.state = state
+        self.nvram = nvram
         self.instance = FakeDomain(**domain)
 
+    def get_nvram(self):
+        return self.nvram
+
     reads = 0
 
     def get_status(self):
@@ -209,23 +215,18 @@ class ShutOffMigrationTestCase(unittest.TestCase):
         destination(host).moveto(source, "vm", live=False, unsafe=False, offline=True)
         self.assertEqual(host.defined, ["<domain><name>vm</name></domain>"])
         self.assertEqual(source.instance.calls, [])
-        self.assertEqual(source.instance.undefined, 0)
+        self.assertEqual(source.instance.undefined, libvirt.VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)
 
-    def test_a_vm_with_nvram_or_tpm_state_is_refused(self):
+    def test_a_vm_with_tpm_state_is_refused(self):
         """That state stays on the source host: the VM would start on the
-        destination with new UEFI variables and a new TPM."""
-        cases = {
-            "nvram": "<os><nvram>/var/lib/libvirt/qemu/nvram/vm_VARS.fd</nvram></os>",
-            "tpm": "<devices><tpm model='tpm-crb'><backend type='emulator' version='2.0'/></tpm></devices>",
-        }
-        for case, xml in cases.items():
-            with self.subTest(case=case):
-                host = FakeHost()
-                source = FakeSource(5, xml=f"<domain>{xml}</domain>")
-                with self.assertRaisesRegex(util.OperationError, "migrate it live"):
-                    destination(host).moveto(source, "vm", live=False, unsafe=False, offline=True)
-                self.assertEqual(host.defined, [])
-                self.assertIsNone(source.instance.undefined)
+        destination with a new TPM."""
+        host = FakeHost()
+        xml = "<domain><devices><tpm model='tpm-crb'><backend type='emulator' version='2.0'/></tpm></devices></domain>"
+        source = FakeSource(5, xml=xml)
+        with self.assertRaisesRegex(util.OperationError, "migrate it live"):
+            destination(host).moveto(source, "vm", live=False, unsafe=False, offline=True)
+        self.assertEqual(host.defined, [])
+        self.assertIsNone(source.instance.undefined)
 
     def test_a_tpm_without_local_state_moves(self):
         xml = "<domain><devices><tpm model='tpm-tis'><backend type='passthrough'/></tpm></devices></domain>"
@@ -425,3 +426,48 @@ class DestinationDisksTestCase(unittest.TestCase):
         xml = with_disks("<disk type='volume' device='disk'><source pool='p' volume='v'/><target dev='vdc'/></disk>")
         with self.assertRaisesRegex(util.OperationError, "vdc \\(p/v\\)"):
             self.migrate(host, xml)
+
+
+class NvramCopyTestCase(unittest.TestCase):
+    """A shut-off move copies the UEFI variables file to the destination."""
+
+    PATH = "/var/lib/libvirt/qemu/nvram/vm_VARS.fd"
+
+    def setUp(self):
+        self.source = FakeSource(5, nvram=self.PATH, xml="<domain><name>vm</name></domain>")
+        self.source.wvm = mock.MagicMock()
+        self.source.wvm.newStream.return_value.recvAll.side_effect = lambda fn, out: fn(None, b"vars", out)
+        self.source_pool = mock.MagicMock()
+        self.source._nvram_pool = lambda: contextlib.nullcontext(self.source_pool)
+        self.host = FakeHost()
+        self.dest = destination(self.host)
+        self.dest_pool = mock.MagicMock()
+        self.dest._nvram_pool = lambda: contextlib.nullcontext(self.dest_pool)
+        self.dest.wvm.newStream = mock.MagicMock()
+        self.shared = mock.patch.object(wvmInstances, "_nvram_shared", return_value=False)
+        self.shared.start()
+        self.addCleanup(self.shared.stop)
+
+    def move(self):
+        self.dest.moveto(self.source, "vm", live=False, unsafe=False, offline=True)
+
+    def test_the_file_is_copied_before_the_definition(self):
+        self.move()
+        copy = self.dest_pool.createXML.return_value
+        self.assertIn("<name>vm_VARS.fd</name><capacity>4</capacity>", self.dest_pool.createXML.call_args[0][0])
+        copy.upload.assert_called_once_with(self.dest.wvm.newStream.return_value, 0, 4, 0)
+        self.assertEqual(self.host.defined, ["<domain><name>vm</name></domain>"])
+
+    def test_a_shared_directory_is_not_copied(self):
+        self.shared.stop()
+        with mock.patch.object(wvmInstances, "_nvram_shared", return_value=True):
+            self.move()
+        self.dest_pool.createXML.assert_not_called()
+        self.shared.start()
+
+    def test_the_copy_is_deleted_when_the_definition_fails(self):
+        self.host.defineXML = mock.MagicMock(side_effect=libvirt.libvirtError("define failed"))
+        with self.assertRaisesRegex(libvirt.libvirtError, "define failed"):
+            self.move()
+        self.dest_pool.storageVolLookupByName.assert_called_once_with("vm_VARS.fd")
+        self.dest_pool.storageVolLookupByName.return_value.delete.assert_called_once_with(0)

vrtManager/tests/test_remove_nvram.py

@@ -109,7 +109,7 @@ class RemoveNvramTestCase(TestCase):
         with self.assertRaises(libvirt.libvirtError):
             connect([source]).remove_nvram(PATH, destination)
         self.assertFalse(source.vols["vm_VARS.fd"].delete.called)
-        source.created[0].delete.assert_called_once_with(0)
+        self.assertEqual(source.created, [])  # both pools are opened before the check
 
     def test_the_transient_pool_is_stopped_when_the_delete_fails(self):
         transient = pool(NVRAM_DIR, ["vm_VARS.fd"])