mirror of
https://github.com/retspen/webvirtcloud
synced 2026-11-08 11:16:47 +00:00
fix(instances): recovery removes the UEFI variables copy of a clone that was not defined
A shut-off clone copies the VM's NVRAM file just before it defines the clone; a worker that stopped between the two left the copy, unnamed by the job, which recovery then reported as leaving nothing. The job records the copy's path once libvirt made it, and recovery deletes that file when the clone was not defined, in whichever pool it is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kk4h2TcUiaWmU5MUszYSea
This commit is contained in:
catborise
parent
b4e5567e4a
commit
ce98bc85d0
4 changed files with 71 additions and 19 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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"])
|
||||
|
||||
|
|
|
|||
|
|
@ -2449,11 +2449,11 @@ class wvmInstance(wvmConnect):
|
|||
</target>
|
||||
</volume>"""
|
||||
|
||||
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):
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue