1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-08 19:26:48 +00:00

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.
This commit is contained in:
catborise 2026-10-04 00:27:55 +03:00
parent e5fda79f5a
commit 57cf49262a
8 changed files with 282 additions and 139 deletions

View file

@ -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())

View file

@ -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:

View file

@ -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()

View file

@ -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):
"""

View file

@ -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"""<volume>
<name>{target_file}</name>
<capacity>0</capacity>
<allocation>0</allocation>
<target>
<format type='{vol_format}'/>
<permissions>
<owner>{clone_data['disk_owner_uid']}</owner>
<group>{clone_data['disk_owner_gid']}</group>
<mode>0644</mode>
<label>virt_image_t</label>
</permissions>
<compat>1.1</compat>
<features><lazy_refcounts/></features>
</target>
</volume>""",
vol,
meta_prealloc and vol_format == "qcow2",
)
if vol_format == "qcow2" and meta_prealloc:
meta_prealloc = True
vol_clone_xml = f"""
<volume>
<name>{target_file}</name>
<capacity>0</capacity>
<allocation>0</allocation>
<target>
<format type='{vol_format}'/>
<permissions>
<owner>{clone_data['disk_owner_uid']}</owner>
<group>{clone_data['disk_owner_gid']}</group>
<mode>0644</mode>
<label>virt_image_t</label>
</permissions>
<compat>1.1</compat>
<features>
<lazy_refcounts/>
</features>
</target>
</volume>"""
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"""<volume type='network'>
<name>{target_file}</name>
<capacity>0</capacity>
<allocation>0</allocation>
<target><format type='{vol_format}'/></target>
</volume>""",
vol,
meta_prealloc,
)
vol_clone_xml = f"""
<volume type='network'>
<name>{target_file}</name>
<capacity>0</capacity>
<allocation>0</allocation>
<target>
<format type='{vol_format}'/>
</target>
</volume>"""
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"))
)

View file

@ -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()

View file

@ -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=(
"<domain><name>src</name><devices>"
"<disk type='file' device='disk'><source file='/pool/src.qcow2'/><target dev='vda'/></disk>"
"</devices></domain>"
))
source_vol = MagicMock()
source_vol.XMLDesc.return_value = "<volume><target><format type='qcow2'/></target></volume>"
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()

View file

@ -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