diff --git a/instances/jobs.py b/instances/jobs.py index 8734cceb..40c0d0d6 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -18,7 +18,7 @@ from libvirt import (VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, VIR_DOMAIN_JOB_NONE, VIR_ERR_NO_STORAGE_VOL, libvirtError) from lxml import etree from vrtManager import util -from vrtManager.connection import wvmConnect +from vrtManager.connection import NVRAM_DIR, wvmConnect from vrtManager.create import cpu_choices, wvmCreate from vrtManager.instance import HOST_DEVICES, check_snapshot_name, wvmInstance, wvmInstances from vrtManager.nodedevs import check_hostdevs @@ -248,6 +248,26 @@ def _recovered_clone(op): return _finish_clone(op, op.intent["clone"], []) +def _remove_clone_nvram(op): + """The copy of the VM's UEFI variables a shut-off clone made just before + it defines the clone (define_clone), left by a worker that stopped + between the two: the clone was not defined, and no VM has it.""" + if not (path := op.intent.get("nvram_copy")): + return + c = Compute.objects.get(pk=op.intent["compute"]) + host = wvmInstances(c.hostname, c.login, c.password, c.type) + try: + if os.path.dirname(path) == NVRAM_DIR: # a volume only of a pool there, which may be none + host.delete_nvram(os.path.basename(path)) + else: + host.wvm.storageVolLookupByPath(path).delete(0) + except libvirtError as err: + if err.get_error_code() != VIR_ERR_NO_STORAGE_VOL: + raise # unreachable: recovery waits and retries + finally: + host.close() + + def _clone_made(op, result): """Records the clone; the job's result, with the clone's page, which the job panel opens: the page the job ran from is the VM cloned.""" @@ -607,7 +627,7 @@ class CloneInstance(Kind): if status == 5: ctx.record_intent(live=False) ctx.set_phase(gettext_noop("Copying disks")) - proxy.clone_instance(clone, uuid) + proxy.clone_instance(clone, uuid, on_nvram=lambda path: ctx.record_intent(nvram_copy=path)) return _clone_made(op, _finish_clone(op, clone, "")) if status not in (1, 3): raise util.OperationError(CLONE_STATE_REFUSED) @@ -699,6 +719,8 @@ class CloneInstance(Kind): outcome = vm_outcome(op, owner=True, volumes=disks) if outcome.state == Operation.SUCCEEDED: outcome.result = {**outcome.result, **_recovered_clone(op), "open": _clone_page(op)} + else: + _remove_clone_nvram(op) return outcome proxy = _recovery_proxy(op) if proxy is None: diff --git a/instances/tests/test_clone_job.py b/instances/tests/test_clone_job.py index ed69ec46..aea10251 100644 --- a/instances/tests/test_clone_job.py +++ b/instances/tests/test_clone_job.py @@ -2,7 +2,7 @@ import contextlib from types import SimpleNamespace -from unittest.mock import MagicMock, patch +from unittest.mock import ANY, MagicMock, patch from uuid import uuid4 import libvirt @@ -71,7 +71,7 @@ class CloneJobTestCase(TransactionTestCase): Operation.objects.filter(pk=op.pk).update(state=Operation.RUNNING, worker="wvc-test:1:a") op.refresh_from_db() CloneInstance().run(op, Context(op, SimpleNamespace(identity="wvc-test:1:a"))) - self.proxy.clone_instance.assert_called_once_with(op.intent["clone"], op.intent["uuid"]) + self.proxy.clone_instance.assert_called_once_with(op.intent["clone"], op.intent["uuid"], on_nvram=ANY) clone = Instance.objects.get(uuid=op.intent["uuid"]) self.assertTrue(UserInstance.objects.filter(instance=clone, user=self.admin).exists()) @@ -178,7 +178,8 @@ class CloneJobTestCase(TransactionTestCase): @patch("instances.jobs.wvmConnect") def test_recovery(self, conn): - op = Operation(user=self.admin, intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", + op = Operation(user=self.admin, params={"instance": self.source.pk}, + intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", "name": "copy", "clone": {"name": "copy", "disk-vda": "copy.qcow2"}}) recovered = CloneInstance().reconcile(op) self.assertEqual(recovered.state, Operation.SUCCEEDED) # the VM is defined @@ -195,7 +196,8 @@ class CloneJobTestCase(TransactionTestCase): @patch("instances.jobs.wvmConnect") def test_recovery_gives_a_defined_shut_off_clone_the_size_asked(self, conn): conn.return_value.wvm.lookupByUUIDString.return_value.state.return_value = [libvirt.VIR_DOMAIN_SHUTOFF, 0] - op = Operation(user=self.admin, intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", + op = Operation(user=self.admin, params={"instance": self.source.pk}, + intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", "name": "copy", "clone": {"name": "copy", "disk-vda": "copy.qcow2", "vcpu": 4, "memory": None, "start": True}}) recovered = CloneInstance().reconcile(op) @@ -206,7 +208,8 @@ class CloneJobTestCase(TransactionTestCase): @patch("instances.jobs.wvmConnect") def test_a_clone_that_left_nothing_failed(self, conn): - op = Operation(user=self.admin, intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", + op = Operation(user=self.admin, params={"instance": self.source.pk}, + intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", "name": "copy", "clone": {"name": "copy", "disk-vda": "copy.qcow2"}}) missing = libvirt.libvirtError("no domain") missing.err = (libvirt.VIR_ERR_NO_DOMAIN, libvirt.VIR_FROM_QEMU, "no domain") @@ -214,6 +217,27 @@ class CloneJobTestCase(TransactionTestCase): conn.return_value.wvm.listAllStoragePools.return_value = [pool(["other.qcow2"])] self.assertEqual(CloneInstance().reconcile(op).state, Operation.FAILED) + @patch("instances.jobs.wvmInstances") + @patch("instances.jobs.wvmConnect") + def test_a_clone_not_defined_leaves_no_copy_of_the_uefi_variables(self, conn, hosts): + """define_clone copies them just before it defines the clone; the job records the copy it made.""" + op = Operation(user=self.admin, params={"instance": self.source.pk}, + intent={"compute": self.compute.pk, "uuid": "abababab-0000-1111-2222-333333333333", + "name": "copy", "clone": {"name": "copy", "disk-vda": "copy.qcow2"}}) + missing = libvirt.libvirtError("no domain") + missing.err = (libvirt.VIR_ERR_NO_DOMAIN, libvirt.VIR_FROM_QEMU, "no domain") + conn.return_value.wvm.lookupByUUIDString.side_effect = missing + conn.return_value.wvm.listAllStoragePools.return_value = [] + CloneInstance().reconcile(op) # no copy recorded: nothing of the job to remove + hosts.return_value.delete_nvram.assert_not_called() + op.intent["nvram_copy"] = "/var/lib/libvirt/qemu/nvram/copy_VARS.fd" + self.assertEqual(CloneInstance().reconcile(op).state, Operation.FAILED) + hosts.return_value.delete_nvram.assert_called_once_with("copy_VARS.fd") + op.intent["nvram_copy"] = "/pool/copy_VARS.fd" # in a storage pool + CloneInstance().reconcile(op) + hosts.return_value.wvm.storageVolLookupByPath.assert_called_once_with("/pool/copy_VARS.fd") + hosts.return_value.wvm.storageVolLookupByPath.return_value.delete.assert_called_once_with(0) + def libvirt_error(code): err = libvirt.libvirtError("libvirt error") @@ -297,7 +321,7 @@ class LiveCloneJobTestCase(TransactionTestCase): op = self.op() CloneInstance().precheck(op, None) self.run_job(op) - self.proxy.clone_instance.assert_called_once_with(op.intent["clone"], op.intent["uuid"]) + self.proxy.clone_instance.assert_called_once_with(op.intent["clone"], op.intent["uuid"], on_nvram=ANY) self.proxy.live_clone_targets.assert_not_called() self.assertFalse(Operation.objects.get(pk=op.pk).intent["live"]) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 7c232c39..26b62d11 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -2449,11 +2449,11 @@ class wvmInstance(wvmConnect): """ - def define_clone(self, clone_data, uuid, paths): + def define_clone(self, clone_data, uuid, paths, on_nvram=None): """Defines the clone with each copied disk's source set from paths - ({dev: new path}) and its own copy of the VM's UEFI variables (NVRAM). - The NVRAM copy is removed if the definition fails. Returns the - clone's UUID.""" + ({dev: new path}) and its own copy of the VM's UEFI variables (NVRAM), + whose path on_nvram is given once made. The NVRAM copy is removed if + the definition fails. Returns the clone's UUID.""" tree, disks = self._clone_definition(clone_data, uuid) for source, dev in disks: source.set("file" if source.get("file") else "dev" if source.get("dev") else "name", paths[dev]) @@ -2485,6 +2485,8 @@ class wvmInstance(wvmConnect): if target.find(tag) is not None: copy_target.append(target.find(tag)) copy = pool.createXMLFrom(etree.tostring(volume).decode(), original, 0) + if on_nvram is not None: + on_nvram(copy.path()) if nvram_source is not None: nvram_source.set("file", copy.path()) else: @@ -2500,10 +2502,11 @@ class wvmInstance(wvmConnect): ) from err raise - def clone_instance(self, clone_data, uuid): - """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.""" + def clone_instance(self, clone_data, uuid, on_nvram=None): + """Copy a shut-off VM, with its disks and UEFI variables (NVRAM; see + define_clone for on_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(gettext_noop("Shut the VM down before cloning it")) @@ -2551,7 +2554,7 @@ class wvmInstance(wvmConnect): created.append(new_path) paths[dev] = new_path - return self.define_clone(clone_data, uuid, paths) + return self.define_clone(clone_data, uuid, paths, on_nvram) except Exception as err: leftovers = [] for path in reversed(created): diff --git a/vrtManager/tests/test_live_clone.py b/vrtManager/tests/test_live_clone.py index c8983441..cadc78b3 100644 --- a/vrtManager/tests/test_live_clone.py +++ b/vrtManager/tests/test_live_clone.py @@ -321,7 +321,7 @@ class DefineCloneTestCase(unittest.TestCase): p.define_clone = MagicMock(return_value=UUID) self.assertEqual(p.clone_instance(FORM, UUID), UUID) p.pool.createXMLFrom.assert_called_once() - p.define_clone.assert_called_once_with(FORM, UUID, {"vda": "/pool/copy-vda.qcow2"}) + p.define_clone.assert_called_once_with(FORM, UUID, {"vda": "/pool/copy-vda.qcow2"}, None) def test_clone_instance_checks_the_macs_before_copying(self): p = proxy() @@ -414,12 +414,15 @@ class LiveCloneTestCase(SimpleTestCase): vm = wvmInstance(*self.args, None, uuid=dom.UUIDString()) source = vm.get_nvram() + made = [] 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}, "abababab-0000-1111-2222-333333333333" + {"name": P + "clone-uefi-copy", "disk-vda": P + "clone-uefi-copy-a.qcow2", "disk_owner_uid": 0, "disk_owner_gid": 0}, "abababab-0000-1111-2222-333333333333", + on_nvram=made.append, ) clone = self.conn.lookupByUUIDString(uuid) path = ElementTree.fromstring(clone.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)).findtext("os/nvram") + self.assertEqual(made, [path]) # the job records it: recovery removes it if the clone is not defined 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))