From 57cf49262ac1664f179f6abe7d612663133181bb Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 00:27:55 +0300 Subject: [PATCH] fix(instances): clones roll back their copies; own error messages stay Clone: - only shut-off VMs are cloned, as the UI already required: a running guest keeps writing while its disks are copied; the definition comes from the persistent XML; - MAC addresses and disk destinations are checked before anything is allocated; - every volume the clone creates (NVRAM and disks) is tracked; if a later copy or the definition fails, only those are deleted, and any that cannot be deleted are named in the error. Before, a failed clone left its earlier copies behind. libvirtError(msg) reports the thread's last libvirt error instead of msg when there is one, so WebVirtCloud's own errors could show an earlier, unrelated failure. util.OperationError, a libvirtError subclass that keeps its message, replaces those raises in vrtManager, instances and nwfilters; 'except libvirtError' still catches it. --- instances/test_live_clone.py | 92 ++++++++++++++ instances/views.py | 14 +-- nwfilters/views.py | 10 +- vrtManager/connection.py | 2 +- vrtManager/instance.py | 231 ++++++++++++++++------------------ vrtManager/test_util.py | 16 +++ vrtManager/test_xml_safety.py | 43 +++++++ vrtManager/util.py | 13 ++ 8 files changed, 282 insertions(+), 139 deletions(-) create mode 100644 instances/test_live_clone.py diff --git a/instances/test_live_clone.py b/instances/test_live_clone.py new file mode 100644 index 00000000..174f4f5f --- /dev/null +++ b/instances/test_live_clone.py @@ -0,0 +1,92 @@ +"""Live checks for cloning (R-08). Runs only with TEST_LIBVIRT_HOST set.""" + +import os +import unittest + +import libvirt +from django.test import SimpleTestCase +from vrtManager.connection import connection_manager +from vrtManager.instance import wvmInstance + +from . import livetest + +P = livetest.PREFIX + + +class LiveCloneTestCase(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) + livetest.ensure_pool(cls.conn) + cls.inventory = livetest.inventory(cls.conn) + super().setUpClass() + + @classmethod + def tearDownClass(cls): + try: + livetest.remove_pool(cls.conn) + finally: + super().tearDownClass() + if livetest.inventory(cls.conn) != cls.inventory: + raise AssertionError("Live tests changed objects outside the test namespace") + + def tearDown(self): + livetest.cleanup(self.conn) + + def pool_volumes(self): + pool = self.conn.storagePoolLookupByName(livetest.POOL) + pool.refresh(0) + return set(pool.listVolumes()) + + def source(self, name): + disks = [livetest.create_volume(self.conn, P + f"{name}-{d}") for d in ("a", "b")] + dom = livetest.define_vm(self.conn, P + name, disks) + return dom, wvmInstance(*self.args, None, uuid=dom.UUIDString()) + + def clone_data(self, name): + return { + "name": P + name, + "disk-vda": P + name + "-a.qcow2", + "disk-vdb": P + name + "-b.qcow2", + "disk_owner_uid": 0, + "disk_owner_gid": 0, + } + + def test_failed_clone_removes_only_its_own_copies(self): + _, vm = self.source("clone-src") + taken = livetest.create_volume(self.conn, P + "clone-dst-b") # destination of vdb exists + before = self.pool_volumes() + + with self.assertRaises(Exception): + vm.clone_instance(self.clone_data("clone-dst")) + + self.assertEqual(self.pool_volumes(), before, "copies left behind or a volume deleted") + self.assertTrue(livetest.volume_exists(self.conn, taken)) + with self.assertRaises(libvirt.libvirtError): + self.conn.lookupByName(P + "clone-dst") + + def test_running_vm_is_not_cloned(self): + dom, vm = self.source("clone-run") + dom.create() + before = self.pool_volumes() + + with self.assertRaises(Exception): + vm.clone_instance(self.clone_data("clone-run-copy")) + + self.assertEqual(self.pool_volumes(), before) + + def test_clone_of_a_stopped_vm(self): + _, vm = self.source("clone-ok") + uuid = vm.clone_instance(self.clone_data("clone-ok-copy")) + clone = self.conn.lookupByUUIDString(uuid) + self.assertEqual(clone.name(), P + "clone-ok-copy") + self.assertTrue({P + "clone-ok-copy-a.qcow2", P + "clone-ok-copy-b.qcow2"} <= self.pool_volumes()) diff --git a/instances/views.py b/instances/views.py index bed07f02..449aac71 100755 --- a/instances/views.py +++ b/instances/views.py @@ -2100,9 +2100,9 @@ def create_instance(request, compute_id, arch, machine): if conn: if not storages: - raise libvirtError(_("You haven't defined any storage pools")) + raise util.OperationError(_("You haven't defined any storage pools")) if not networks: - raise libvirtError(_("You haven't defined any network pools")) + raise util.OperationError(_("You haven't defined any network pools")) if request.method == "POST": if "create" in request.POST: @@ -2117,11 +2117,11 @@ def create_instance(request, compute_id, arch, machine): meta_prealloc = True if instances: if data["name"] in instances: - raise libvirtError( + raise util.OperationError( _("A virtual machine with this name already exists") ) if Instance.objects.filter(name__exact=data["name"]): - raise libvirtError( + raise util.OperationError( _( "There is an instance with same name. Remove it and try again!" ) @@ -2129,7 +2129,7 @@ def create_instance(request, compute_id, arch, machine): if data["hdd_size"]: if not data["mac"]: - raise libvirtError( + raise util.OperationError( _("No Virtual Machine MAC has been entered") ) else: @@ -2163,7 +2163,7 @@ def create_instance(request, compute_id, arch, machine): data["name"] + ".img", data["storage"] ) if dest_vol: - raise libvirtError( + raise util.OperationError( _( "Image has already exist. Please check volumes or change instance name" ) @@ -2193,7 +2193,7 @@ def create_instance(request, compute_id, arch, machine): is_disk_created = True else: if not data["images"]: - raise libvirtError( + raise util.OperationError( _("First you need to create or select an image") ) else: diff --git a/nwfilters/views.py b/nwfilters/views.py index 1959cf1f..d82d30f6 100644 --- a/nwfilters/views.py +++ b/nwfilters/views.py @@ -185,7 +185,7 @@ def nwfilter(request, compute_id, nwfltr): conn.create_nwfilter(new_xml) except libvirtError as lib_err: conn.create_nwfilter(xml) - raise libvirtError(lib_err) + raise util.OperationError(lib_err) if "del_nwfilter_rule" in request.POST: action = request.POST.get("action", "") @@ -198,7 +198,7 @@ def nwfilter(request, compute_id, nwfltr): conn.create_nwfilter(new_xml) except libvirtError as lib_err: conn.create_nwfilter(xml) - raise libvirtError(lib_err) + raise util.OperationError(lib_err) if "del_nwfilter_ref" in request.POST: ref_name = request.POST.get("ref") @@ -208,7 +208,7 @@ def nwfilter(request, compute_id, nwfltr): conn.create_nwfilter(new_xml) except libvirtError as lib_err: conn.create_nwfilter(xml) - raise libvirtError(lib_err) + raise util.OperationError(lib_err) if "add_nwfilter_rule" in request.POST: rule_xml = request.POST.get("nwfilterrule_xml", "") @@ -220,7 +220,7 @@ def nwfilter(request, compute_id, nwfltr): conn.create_nwfilter(new_xml) except libvirtError as lib_err: conn.create_nwfilter(xml) - raise libvirtError(lib_err) + raise util.OperationError(lib_err) if "add_nwfilter_ref" in request.POST: ref_name = request.POST.get("nwfilters_select", "") @@ -232,7 +232,7 @@ def nwfilter(request, compute_id, nwfltr): conn.create_nwfilter(new_xml) except libvirtError as lib_err: conn.create_nwfilter(xml) - raise libvirtError(lib_err) + raise util.OperationError(lib_err) return HttpResponseRedirect(request.get_full_path()) conn.close() diff --git a/vrtManager/connection.py b/vrtManager/connection.py index 093b5ef0..34ff87b9 100644 --- a/vrtManager/connection.py +++ b/vrtManager/connection.py @@ -301,7 +301,7 @@ class wvmConnectionManager(object): return connection.connection else: # raise libvirt error - raise libvirtError(connection.last_error) + raise util.OperationError(connection.last_error) def host_is_up(self, conn_type, hostname): """ diff --git a/vrtManager/instance.py b/vrtManager/instance.py index a0054f62..490f3c09 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -140,7 +140,7 @@ class wvmInstances(wvmConnect): dom_emulator = conn.get_dom_emulator() if dom_emulator != self.get_emulator(dom_arch): - raise libvirtError( + raise util.OperationError( "Destination host emulator is different. Cannot be migrated" ) @@ -721,7 +721,7 @@ class wvmInstance(wvmConnect): tree = etree.fromstring(self._XMLDesc(xml_flags)) found = tree.xpath("./devices/disk[@device='cdrom'][target/@dev=$dev]", dev=dev) if not found: - raise libvirtError(f"CD-ROM {dev} is not in the {label} definition") + raise util.OperationError(f"CD-ROM {dev} is not in the {label} definition") devices.append(found[0]) for (label, _, affect), disk in zip(definitions, devices): for source in disk.findall("source"): @@ -732,7 +732,7 @@ class wvmInstance(wvmConnect): self.instance.updateDeviceFlags(etree.tostring(disk).decode(), affect) except libvirtError as err: if label == "persistent" and len(definitions) == 2: - raise libvirtError(f"Media of {dev} changed in the running VM only: {err}") from err + raise util.OperationError(f"Media of {dev} changed in the running VM only: {err}") from err raise def mount_iso(self, dev, image): @@ -742,9 +742,9 @@ class wvmInstance(wvmConnect): if image in stg.listVolumes(): paths.append(stg.storageVolLookupByName(image).path()) if not paths: - raise libvirtError(f"ISO image {image} was not found") + raise util.OperationError(f"ISO image {image} was not found") if len(paths) > 1: - raise libvirtError(f"ISO image {image} is in more than one storage pool") + raise util.OperationError(f"ISO image {image} is in more than one storage pool") self._set_cdrom_media(dev, paths[0]) def umount_iso(self, dev, image=None): @@ -986,7 +986,7 @@ class wvmInstance(wvmConnect): parent.remove(vcpu) self._defineXML(etree.tostring(tree).decode()) else: - raise libvirtError( + raise util.OperationError( "Please shutdown the instance then try to enable vCPU hotplug" ) @@ -1505,26 +1505,29 @@ class wvmInstance(wvmConnect): return ":".join(mac_tuples) def clone_instance(self, clone_data): - clone_dev_path = [] - + """Copy a shut-off VM. Inputs are checked before anything is allocated, + and on failure only the volumes this call created are removed (R-08).""" + 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") if clone_data["name"] in self.get_instances(): raise ValueError("An instance with the clone name already exists") - xml = self._XMLDesc(VIR_DOMAIN_XML_SECURE) - tree = etree.fromstring(xml) - name = tree.find("name") - name.text = clone_data["name"] + tree = etree.fromstring(self._XMLDesc(PERSISTENT_XML)) + tree.find("name").text = clone_data["name"] uuid = tree.find("uuid") if uuid is None: uuid = etree.SubElement(tree, "uuid") uuid.text = util.randomUUID() + self._set_options( + tree, + { + "title": clone_data.get("clone-title", ""), + "description": clone_data.get("clone-description", ""), + }, + ) - options = { - "title": clone_data.get("clone-title", ""), - "description": clone_data.get("clone-description", ""), - } - self._set_options(tree, options) - + disks = [] for disk in list(tree.findall("devices/disk")): if disk.get("device") != "disk": continue @@ -1539,120 +1542,96 @@ class wvmInstance(wvmConnect): dev = target.get("dev") if target is not None else None if not supported or not dev or not clone_data.get("disk-" + dev): raise ValueError("Cannot clone a disk without a supported source and destination") - - src_nvram_path = self.get_nvram() - if src_nvram_path: - # Change XML for nvram - 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: - stg_conn = self.get_wvmStorages() - stg_conn.create_storage("dir", nvram_pool_name, None, nvram_dir) - - new_nvram_name = f"{clone_data['name']}_VARS" - nvram_stg = self.get_wvmStorage(nvram_pool_name) - nvram_stg.clone_volume(src_nvram_name, new_nvram_name, file_suffix="fd") + disks.append((source, dev)) for num, net in enumerate(tree.findall("devices/interface")): - elm = net.find("mac") - mac_address = self.fix_mac(clone_data["clone-net-mac-" + str(num)]) - elm.set("address", mac_address) + mac = clone_data.get(f"clone-net-mac-{num}") + if not mac: + raise ValueError(f"No MAC address for network interface {num}") + net.find("mac").set("address", self.fix_mac(mac)) - for disk in tree.findall("devices/disk"): - if disk.get("device") == "disk": - elm = disk.find("target") - device_name = elm.get("dev") - if device_name: - target_file = clone_data["disk-" + device_name] - meta_prealloc = False - with contextlib.suppress(Exception): - meta_prealloc = clone_data["meta-" + device_name] + 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) - elm.set("dev", device_name) + # 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)) - elm = disk.find("source") - source_file = elm.get("file") - if source_file: - clone_dev_path.append(source_file) - clone_path = os.path.join(os.path.dirname(source_file), target_file) - elm.set("file", clone_path) - - vol = self.get_volume_by_path(source_file) - vol_format = util.get_xml_path( - vol.XMLDesc(0), "/volume/target/format/@type" + for source, dev in disks: + target_file = clone_data["disk-" + dev] + meta_prealloc = bool(clone_data.get("meta-" + dev)) + if source.get("file"): + vol = self.get_volume_by_path(source.get("file")) + vol_format = util.get_xml_path(vol.XMLDesc(0), "/volume/target/format/@type") + new_vol = vol.storagePoolLookupByVolume().createXMLFrom( + f""" + {target_file} + 0 + 0 + + + + {clone_data['disk_owner_uid']} + {clone_data['disk_owner_gid']} + 0644 + + + 1.1 + + + """, + vol, + meta_prealloc and vol_format == "qcow2", ) - - if vol_format == "qcow2" and meta_prealloc: - meta_prealloc = True - - vol_clone_xml = f""" - - {target_file} - 0 - 0 - - - - {clone_data['disk_owner_uid']} - {clone_data['disk_owner_gid']} - 0644 - - - 1.1 - - - - - """ - - stg = vol.storagePoolLookupByVolume() - stg.createXMLFrom(vol_clone_xml, vol, meta_prealloc) - - source_protocol = elm.get("protocol") - if source_protocol == "rbd": - source_name = elm.get("name") - clone_name = "%s/%s" % (os.path.dirname(source_name), target_file) - elm.set("name", clone_name) - - vol = self.get_volume_by_path(source_name) - vol_format = util.get_xml_path( - vol.XMLDesc(0), "/volume/target/format/@type" + created.append(new_vol.path()) + source.set("file", new_vol.path()) + elif source.get("protocol") == "rbd": + vol = self.get_volume_by_path(source.get("name")) + vol_format = util.get_xml_path(vol.XMLDesc(0), "/volume/target/format/@type") + new_vol = vol.storagePoolLookupByVolume().createXMLFrom( + f""" + {target_file} + 0 + 0 + + """, + vol, + meta_prealloc, ) - - vol_clone_xml = f""" - - {target_file} - 0 - 0 - - - - """ - stg = vol.storagePoolLookupByVolume() - stg.createXMLFrom(vol_clone_xml, vol, meta_prealloc) - - source_dev = elm.get("dev") - if source_dev: - clone_path = os.path.join(os.path.dirname(source_dev), target_file) - elm.set("dev", clone_path) - - vol = self.get_volume_by_path(source_dev) - stg = vol.storagePoolLookupByVolume() - - vol_name = util.get_xml_path(vol.XMLDesc(0), "/volume/name") - pool_name = util.get_xml_path(stg.XMLDesc(0), "/pool/name") - + created.append(new_vol.path()) + source.set("name", f"{os.path.dirname(source.get('name'))}/{target_file}") + else: + vol = self.get_volume_by_path(source.get("dev")) + pool_name = util.get_xml_path(vol.storagePoolLookupByVolume().XMLDesc(0), "/pool/name") storage = self.get_wvmStorage(pool_name) - storage.clone_volume(vol_name, target_file) + name = storage.clone_volume(vol.name(), target_file) + new_path = storage.get_volume(name).path() + created.append(new_path) + source.set("dev", new_path) - self._defineXML(ElementTree.tostring(tree).decode()) + self._defineXML(etree.tostring(tree).decode()) + except Exception as err: + leftovers = [] + for path in reversed(created): + try: + self.get_volume_by_path(path).delete(0) + except libvirtError: + leftovers.append(path) + if leftovers: + 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() @@ -1720,7 +1699,7 @@ class wvmInstance(wvmConnect): """Replace the NIC with old_mac; put it back if the new one cannot be added.""" old_xml = self.delete_network(old_mac) if old_xml is None: - raise libvirtError(f"No network interface with MAC {old_mac}") + raise util.OperationError(f"No network interface with MAC {old_mac}") try: self.add_network(mac, source, source_type, model, nwfilter) except libvirtError: @@ -1757,11 +1736,11 @@ class wvmInstance(wvmConnect): elif net_source_type == "iface": source.set("dev", net_source) else: - raise libvirtError( + raise util.OperationError( "Unknown network type: {}".format(net_source_type) ) else: - raise libvirtError( + raise util.OperationError( "Unknown network type: {}".format(interface.get("type")) ) diff --git a/vrtManager/test_util.py b/vrtManager/test_util.py index 13abd6a6..fb174231 100644 --- a/vrtManager/test_util.py +++ b/vrtManager/test_util.py @@ -128,5 +128,21 @@ class TestVrtManagerUtil(unittest.TestCase): self.assertIsNone(vol_dev_type("unknown_bus")) + +class OperationErrorTestCase(unittest.TestCase): + def test_keeps_its_own_message_after_a_libvirt_failure(self): + import libvirt + from vrtManager.util import OperationError + + conn = libvirt.open("test:///default") + self.addCleanup(conn.close) + with self.assertRaises(libvirt.libvirtError): + conn.lookupByName("no-such-domain") # leaves a thread-local last error + self.assertNotEqual(str(libvirt.libvirtError("own message")), "own message") + error = OperationError("own message") + self.assertEqual(str(error), "own message") + self.assertIsInstance(error, libvirt.libvirtError) + + if __name__ == "__main__": unittest.main() diff --git a/vrtManager/test_xml_safety.py b/vrtManager/test_xml_safety.py index d7d61bc3..84679803 100644 --- a/vrtManager/test_xml_safety.py +++ b/vrtManager/test_xml_safety.py @@ -185,6 +185,47 @@ class TestXmlSafety(unittest.TestCase): self.assertIn("type='pflash'", inst._snapshotCreateXML.call_args[0][0]) + +class TestCloneRollback(unittest.TestCase): + """R-08: a failed clone removes only the volumes it created.""" + + def test_copies_are_removed_when_the_definition_fails(self): + import libvirt + + inst = wvmInstance.__new__(wvmInstance) + inst.wvm = MagicMock() + inst.get_status = MagicMock(return_value=5) + inst.get_instances = MagicMock(return_value=[]) + inst.get_nvram = MagicMock(return_value=None) + inst._XMLDesc = MagicMock(return_value=( + "src" + "" + "" + )) + source_vol = MagicMock() + source_vol.XMLDesc.return_value = "" + copy = MagicMock() + copy.path.return_value = "/pool/copy.qcow2" + source_vol.storagePoolLookupByVolume.return_value.createXMLFrom.return_value = copy + inst.get_volume_by_path = MagicMock(side_effect=lambda path: copy if path == "/pool/copy.qcow2" else source_vol) + inst._defineXML = MagicMock(side_effect=libvirt.libvirtError("bad definition")) + + with self.assertRaises(libvirt.libvirtError): + inst.clone_instance({"name": "copy", "disk-vda": "copy.qcow2", "disk_owner_uid": 0, "disk_owner_gid": 0}) + copy.delete.assert_called_once_with(0) + source_vol.delete.assert_not_called() + + def test_running_vm_is_refused_before_anything_is_read(self): + import libvirt + + inst = wvmInstance.__new__(wvmInstance) + inst.get_status = MagicMock(return_value=1) + inst._XMLDesc = MagicMock() + with self.assertRaisesRegex(libvirt.libvirtError, "Shut the VM down"): + inst.clone_instance({"name": "copy"}) + inst._XMLDesc.assert_not_called() + + if __name__ == "__main__": unittest.main() @@ -379,6 +420,7 @@ class TestEmptyDiskClone(unittest.TestCase): # Exercise defensive handling of incomplete disk metadata, while the # clone is still defined and looked up through the real test driver. inst._XMLDesc = MagicMock(return_value=etree.tostring(config).decode()) + inst.get_status = MagicMock(return_value=5) # only shut-off VMs are cloned clone_uuid = inst.clone_instance({"name": "empty-disk-clone"}) clone = conn.lookupByUUIDString(clone_uuid) self.addCleanup(clone.undefine) @@ -396,6 +438,7 @@ class TestEmptyDiskClone(unittest.TestCase): inst.wvm = conn inst.instance = conn.lookupByName("test") inst._XMLDesc = MagicMock() + inst.get_status = MagicMock(return_value=5) with self.assertRaisesRegex(ValueError, "already exists"): inst.clone_instance({"name": "test"}) inst._XMLDesc.assert_not_called() diff --git a/vrtManager/util.py b/vrtManager/util.py index 7ec27f80..26ca01f9 100644 --- a/vrtManager/util.py +++ b/vrtManager/util.py @@ -9,6 +9,19 @@ import lxml.etree as etree from django.conf import settings + +class OperationError(libvirt.libvirtError): + """An error with WebVirtCloud's own message. + + libvirtError(msg) reports the thread's last libvirt error instead of msg + when there is one, so an earlier, unrelated failure would be shown. + """ + + def __init__(self, msg): + Exception.__init__(self, msg) + self.err = None + + def is_kvm_available(xml): kvm_domains = get_xml_path(xml, "//domain/@type='kvm'") return kvm_domains > 0