From 975b7f0baf75a9c1f92df0a42a23dd242fd0c8d2 Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 9 Oct 2026 05:54:30 +0300 Subject: [PATCH] fix(migration): remove the NVRAM file a live migration leaves on the source libvirt undefines the source domain after a migration but keeps its NVRAM file. A VM created later on that host with the same name reuses the file and starts with the old VM's UEFI variables (boot entries, enrolled keys): checked on libvirt 11.10, where only VIR_DOMAIN_START_RESET_NVRAM recreates it from the template. After a successful migration the file is now deleted on the source, as a volume of the pool on libvirt's NVRAM directory or of a transient one. Only a file in /var/lib/libvirt/qemu/nvram, and only when the destination cannot create a marker file the source just created there (an exclusive create also fails on shared storage whose listing an NFS client caches). A failed removal is logged with the migration, which stands and is recorded. --- README.md | 1 + instances/test_migration_log.py | 46 +++++++++++- instances/utils.py | 20 +++++ vrtManager/connection.py | 66 +++++++++++++++++ vrtManager/test_live_nvram.py | 88 ++++++++++++++++++++++ vrtManager/test_remove_nvram.py | 126 ++++++++++++++++++++++++++++++++ 6 files changed, 345 insertions(+), 2 deletions(-) create mode 100644 vrtManager/test_live_nvram.py create mode 100644 vrtManager/test_remove_nvram.py diff --git a/README.md b/README.md index a69ce36f..e64be2f5 100644 --- a/README.md +++ b/README.md @@ -534,6 +534,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat - A VM that vanishes from its compute keeps its owners for `INSTANCE_OWNERSHIP_RETENTION_DAYS` (30) in case the same UUID comes back; superusers list and remove these records on the "Removed VMs" admin page. - The log rotates (10 MB × 5) in `data/webvirtcloud.log`. - A disk can be grown while its VM runs or is paused, not only when it is shut off. +- 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). - Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80). diff --git a/instances/test_migration_log.py b/instances/test_migration_log.py index bd378181..4b8722ab 100644 --- a/instances/test_migration_log.py +++ b/instances/test_migration_log.py @@ -7,6 +7,7 @@ from computes.models import Compute from django.contrib.auth import get_user_model from django.test import TestCase from django.urls import reverse +from libvirt import libvirtError from logs.models import Logs from rest_framework.test import APIClient @@ -22,11 +23,18 @@ class MigrationLogTests(TestCase): self.dest = Compute.objects.create(name="ml-dst", hostname="198.51.100.9", login="root", password="", type=2) self.vm = Instance.objects.create(compute=self.source, name="vm", uuid="u-ml") - def migrate(self, outcome, **request): + def migrate(self, outcome, nvram=None, remove_nvram=True, **request): + self.proxy = MagicMock() + self.proxy.get_autostart.return_value = False + self.proxy.get_nvram.return_value = nvram + if isinstance(remove_nvram, Exception): + self.proxy.remove_nvram.side_effect = remove_nvram + else: + self.proxy.remove_nvram.return_value = remove_nvram with patch.object(Compute, "status", True), \ patch("instances.utils.wvmInstances") as instances, \ patch("instances.utils.wvmInstance"), \ - patch.object(Instance, "proxy", MagicMock()), \ + patch.object(Instance, "proxy", self.proxy), \ patch.object(Instance, "autostart", False): if isinstance(outcome, Exception): instances.return_value.moveto.side_effect = outcome @@ -34,6 +42,40 @@ class MigrationLogTests(TestCase): instances.return_value.moveto.return_value = outcome return migrate_instance(self.dest, self.vm, self.admin, **request) + def test_the_nvram_file_left_on_the_source_is_removed(self): + self.migrate("live", nvram="/var/lib/libvirt/qemu/nvram/vm_VARS.fd", live=True) + path, destination = self.proxy.remove_nvram.call_args[0] + self.assertEqual(path, "/var/lib/libvirt/qemu/nvram/vm_VARS.fd") + self.assertIsNotNone(destination) + self.assertEqual(Logs.objects.get().message, "Instance is migrated(live) to 198.51.100.9") + + def test_a_vm_without_nvram_removes_nothing(self): + self.migrate("live", live=True) + self.assertFalse(self.proxy.remove_nvram.called) + + def test_a_failed_nvram_removal_is_logged_but_the_migration_stands(self): + self.migrate("live", nvram="/var/lib/libvirt/qemu/nvram/vm_VARS.fd", + remove_nvram=libvirtError("cannot delete"), live=True) + self.assertEqual( + Logs.objects.get().message, + "Instance is migrated(live) to 198.51.100.9; removing its NVRAM file " + "/var/lib/libvirt/qemu/nvram/vm_VARS.fd on the source failed: cannot delete", + ) + self.vm.refresh_from_db() + self.assertEqual(self.vm.compute, self.dest) + + def test_any_nvram_removal_error_still_records_the_migration(self): + self.migrate("live", nvram="/var/lib/libvirt/qemu/nvram/vm_VARS.fd", + remove_nvram=AttributeError("unexpected"), live=True) + self.vm.refresh_from_db() + self.assertEqual(self.vm.compute, self.dest) + self.assertIn("on the source failed: unexpected", Logs.objects.get().message) + + def test_a_failed_migration_removes_nothing(self): + with self.assertRaises(util.OperationError): + self.migrate(util.OperationError("no"), nvram="/var/lib/libvirt/qemu/nvram/vm_VARS.fd", live=True) + self.assertFalse(self.proxy.remove_nvram.called) + def test_success_names_the_mode_used(self): self.migrate("non-live, compressed", live=False, compress=True) log = Logs.objects.get() diff --git a/instances/utils.py b/instances/utils.py index 454dd70f..880d3e7e 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -190,6 +190,7 @@ def migrate_instance( ) autostart = instance.proxy.get_autostart() + nvram = instance.proxy.get_nvram() mode = conn_migrate.moveto( instance.proxy, instance.name, @@ -236,6 +237,25 @@ def migrate_instance( if conn_new is not None: conn_new.close() + # libvirt keeps the NVRAM file on the source; a VM created there + # later with the same name would start with these variables. + if nvram: + conn_new = None + try: + conn_new = wvmInstances( + new_compute.hostname, new_compute.login, new_compute.password, new_compute.type + ) + instance.proxy.remove_nvram(nvram, conn_new) + except Exception as error: # the migration stands and is recorded + msg = _("%(msg)s; removing its NVRAM file %(path)s on the source failed: %(error)s") % { + "msg": msg, + "path": nvram, + "error": error, + } + finally: + if conn_new is not None: + conn_new.close() + try: record_migration(instance, new_compute) except Exception as error: diff --git a/vrtManager/connection.py b/vrtManager/connection.py index 81220cd2..c52c024b 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -1,8 +1,10 @@ +import contextlib import os import re import socket import threading import time +import uuid from urllib.parse import quote import libvirt @@ -87,6 +89,10 @@ def _after_fork_in_child(): os.register_at_fork(after_in_child=_after_fork_in_child) +# libvirt's per-host directory of the VMs' UEFI variables (NVRAM) +NVRAM_DIR = "/var/lib/libvirt/qemu/nvram" + + class wvmConnection(object): """ class representing a single connection stored in the Connection Manager @@ -832,6 +838,66 @@ class wvmConnect(object): def get_volume_by_path(self, path): return self.wvm.storageVolLookupByPath(path) + def remove_nvram(self, path, destination): + """Deletes the NVRAM file of a VM that migrated from this host to + destination (a wvmConnect). Only a file in libvirt's per-host NVRAM + directory, and only when destination does not see that directory too + (shared storage would hold the migrated VM's own file). Returns + whether a file was deleted.""" + if os.path.dirname(path) != NVRAM_DIR: + return False + marker = ( + f"webvirtcloud-check-{uuid.uuid4().hex}0" + "" + ) + 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) + vol = self._find_volume(pool, os.path.basename(path)) + if vol is None: + return False + vol.delete(0) + return True + + @contextlib.contextmanager + def _nvram_pool(self): + """An active, refreshed pool on NVRAM_DIR: libvirt reads and deletes + files only as volumes of a pool. A transient one when none exists.""" + for pool in self.wvm.listAllStoragePools(libvirt.VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE): + # a pool without a target path (rbd, gluster, ...) holds no files + if (util.get_xml_path(pool.XMLDesc(0), "/pool/target/path") or "").rstrip("/") == NVRAM_DIR: + pool.refresh(0) + yield pool + return + pool = self.wvm.storagePoolCreateXML( + f"webvirtcloud-nvram-{uuid.uuid4().hex[:8]}" + f"{NVRAM_DIR}", + 0, + ) + try: + yield pool + finally: + pool.destroy() + + @staticmethod + def _find_volume(pool, name): + try: + return pool.storageVolLookupByName(name) + except libvirtError as err: + if err.get_error_code() == libvirt.VIR_ERR_NO_STORAGE_VOL: + return None + raise + def get_network(self, net): return self.wvm.networkLookupByName(net) diff --git a/vrtManager/test_live_nvram.py b/vrtManager/test_live_nvram.py new file mode 100644 index 00000000..79b36c25 --- /dev/null +++ b/vrtManager/test_live_nvram.py @@ -0,0 +1,88 @@ +"""Live check that the NVRAM file a migrated VM leaves behind is removed, +and kept when the destination sees the same directory. + +Runs only with TEST_LIBVIRT_HOST set (see instances/livetest.py). The +deletion needs a second host: TEST_LIBVIRT_SSH_HOST (and _LOGIN). +""" + +import os +import unittest + +import libvirt +from django.test import SimpleTestCase +from lxml import etree + +from instances import livetest +from vrtManager.connection import CONN_SSH, NVRAM_DIR, connection_manager, wvmConnect + +NAME = livetest.PREFIX + "nvram" +PATH = f"{NVRAM_DIR}/{NAME}_VARS.fd" + + +class LiveRemoveNvramTestCase(SimpleTestCase): + @classmethod + def setUpClass(cls): + if not livetest.enabled(): + raise unittest.SkipTest("Set TEST_LIBVIRT_HOST to run the live libvirt tests") + cls.args = ( + os.environ["TEST_LIBVIRT_HOST"], + os.environ.get("TEST_LIBVIRT_LOGIN", ""), + os.environ.get("TEST_LIBVIRT_PASSWORD", ""), + int(os.environ.get("TEST_LIBVIRT_TYPE", 4)), + ) + cls.conn = connection_manager.get_connection(*cls.args) + livetest.cleanup(cls.conn) + cls.inventory = livetest.inventory(cls.conn) + cls.pools = {p.name() for p in cls.conn.listAllStoragePools()} + super().setUpClass() + + @classmethod + def tearDownClass(cls): + livetest.cleanup(cls.conn) + super().tearDownClass() + if livetest.inventory(cls.conn) != cls.inventory: + raise AssertionError("Live tests changed objects outside the test namespace") + if {p.name() for p in cls.conn.listAllStoragePools()} != cls.pools: + raise AssertionError("A storage pool was left behind") + + def leave_nvram(self): + """What a migration with VIR_MIGRATE_UNDEFINE_SOURCE leaves: the file + without a domain.""" + dom = self.conn.defineXML( + f"{NAME}1281" + "hvm" + "" + ) + dom.createWithFlags(libvirt.VIR_DOMAIN_START_PAUSED) # creates the file + self.assertEqual(etree.fromstring(dom.XMLDesc(0)).findtext("os/nvram"), PATH) + dom.destroy() + dom.undefineFlags(libvirt.VIR_DOMAIN_UNDEFINE_KEEP_NVRAM) + + def tearDown(self): + livetest.cleanup(self.conn) + host = wvmConnect(*self.args) + with host._nvram_pool() as pool: + vol = host._find_volume(pool, os.path.basename(PATH)) + if vol is not None: + vol.delete(0) + + def exists(self): + host = wvmConnect(*self.args) + with host._nvram_pool() as pool: + return host._find_volume(pool, os.path.basename(PATH)) is not None + + def test_a_directory_both_connections_see_is_kept(self): + self.leave_nvram() + self.assertFalse(wvmConnect(*self.args).remove_nvram(PATH, wvmConnect(*self.args))) + self.assertTrue(self.exists()) + + def test_the_file_is_deleted_when_the_destination_is_another_host(self): + other_host = os.environ.get("TEST_LIBVIRT_SSH_HOST") + if not other_host or other_host == self.args[0]: + self.skipTest("Set TEST_LIBVIRT_SSH_HOST to a second host") + other = wvmConnect(other_host, os.environ.get("TEST_LIBVIRT_SSH_LOGIN", "root"), "", CONN_SSH) + other_pools = {p.name() for p in other.wvm.listAllStoragePools()} + self.leave_nvram() + self.assertTrue(wvmConnect(*self.args).remove_nvram(PATH, other)) + self.assertFalse(self.exists()) + self.assertEqual({p.name() for p in other.wvm.listAllStoragePools()}, other_pools) diff --git a/vrtManager/test_remove_nvram.py b/vrtManager/test_remove_nvram.py new file mode 100644 index 00000000..aac09f6d --- /dev/null +++ b/vrtManager/test_remove_nvram.py @@ -0,0 +1,126 @@ +"""After a migration took a VM away, its NVRAM file is removed from the +source when it is in libvirt's per-host NVRAM directory and the destination +does not share that directory; a VM later created on the source with the same +name would otherwise start with the old UEFI variables.""" + +from unittest import TestCase +from unittest.mock import MagicMock + +import libvirt + +from vrtManager.connection import NVRAM_DIR, wvmConnect + +PATH = NVRAM_DIR + "/vm_VARS.fd" + + +def error(code): + err = libvirt.libvirtError("error %d" % code) + err.err = (code, libvirt.VIR_FROM_STORAGE, str(err), 2, None, None, None, 0, 0) + return err + + +def pool(path, volumes=(), create_error=None): + p = MagicMock() + p.XMLDesc.return_value = f"p{path}" + p.vols = {name: MagicMock() for name in volumes} + p.created = [] + + def lookup(name): + if name not in p.vols: + raise error(libvirt.VIR_ERR_NO_STORAGE_VOL) + return p.vols[name] + + def create(xml, flags): + if create_error is not None: + raise create_error + vol = MagicMock() + p.created.append(vol) + return vol + + p.storageVolLookupByName.side_effect = lookup + p.createXML.side_effect = create + return p + + +def connect(pools=(), transient=None): + conn = wvmConnect.__new__(wvmConnect) + conn.wvm = MagicMock() + conn.wvm.listAllStoragePools.return_value = list(pools) + conn.wvm.storagePoolCreateXML.return_value = transient + return conn + + +def local_destination(): + return connect([pool(NVRAM_DIR)]) + + +class RemoveNvramTestCase(TestCase): + def test_a_file_outside_the_nvram_directory_is_kept(self): + conn = connect() + for path in ("/srv/shared/nvram/vm_VARS.fd", NVRAM_DIR + "/sub/vm_VARS.fd", "relative_VARS.fd"): + with self.subTest(path=path): + self.assertFalse(conn.remove_nvram(path, local_destination())) + self.assertFalse(conn.wvm.method_calls) + + def test_a_pool_on_the_directory_is_used(self): + existing = pool(NVRAM_DIR + "/", ["vm_VARS.fd"]) + conn = connect([pool("/var/lib/libvirt/images"), existing]) + self.assertTrue(conn.remove_nvram(PATH, local_destination())) + existing.vols["vm_VARS.fd"].delete.assert_called_once_with(0) + self.assertFalse(conn.wvm.storagePoolCreateXML.called) + self.assertFalse(existing.destroy.called) + + def test_pools_without_a_target_path_are_skipped(self): + rbd = MagicMock() + rbd.XMLDesc.return_value = "cephrbd" + transient = pool(NVRAM_DIR, ["vm_VARS.fd"]) + conn = connect([rbd], transient) + self.assertTrue(conn.remove_nvram(PATH, local_destination())) + transient.vols["vm_VARS.fd"].delete.assert_called_once_with(0) + + def test_without_a_pool_a_transient_one_is_used_and_stopped(self): + transient = pool(NVRAM_DIR, ["vm_VARS.fd"]) + conn = connect([pool("/var/lib/libvirt/images")], transient) + self.assertTrue(conn.remove_nvram(PATH, local_destination())) + self.assertIn(f"{NVRAM_DIR}", conn.wvm.storagePoolCreateXML.call_args[0][0]) + transient.vols["vm_VARS.fd"].delete.assert_called_once_with(0) + transient.destroy.assert_called_once_with() + + def test_a_directory_the_destination_shares_is_left_alone(self): + # the destination cannot create the marker the source just created + source = pool(NVRAM_DIR, ["vm_VARS.fd"]) + destination = connect([pool(NVRAM_DIR, create_error=error(libvirt.VIR_ERR_STORAGE_VOL_EXIST))]) + self.assertFalse(connect([source]).remove_nvram(PATH, destination)) + self.assertFalse(source.vols["vm_VARS.fd"].delete.called) + source.created[0].delete.assert_called_once_with(0) # the marker is gone + + def test_both_markers_are_removed(self): + source = pool(NVRAM_DIR, ["vm_VARS.fd"]) + other = pool(NVRAM_DIR) + self.assertTrue(connect([source]).remove_nvram(PATH, connect([other]))) + self.assertEqual(source.createXML.call_args, other.createXML.call_args) # one name + source.created[0].delete.assert_called_once_with(0) + other.created[0].delete.assert_called_once_with(0) + + def test_when_the_destination_cannot_be_checked_nothing_is_deleted(self): + source = pool(NVRAM_DIR, ["vm_VARS.fd"]) + destination = connect([], None) + destination.wvm.storagePoolCreateXML.side_effect = error(libvirt.VIR_ERR_INTERNAL_ERROR) + 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) + + def test_the_transient_pool_is_stopped_when_the_delete_fails(self): + transient = pool(NVRAM_DIR, ["vm_VARS.fd"]) + transient.vols["vm_VARS.fd"].delete.side_effect = error(libvirt.VIR_ERR_INTERNAL_ERROR) + conn = connect([], transient) + with self.assertRaises(libvirt.libvirtError): + conn.remove_nvram(PATH, local_destination()) + transient.destroy.assert_called_once_with() + + def test_a_missing_file_is_nothing_to_remove(self): + transient = pool(NVRAM_DIR) + conn = connect([], transient) + self.assertFalse(conn.remove_nvram(PATH, local_destination())) + transient.destroy.assert_called_once_with()