<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> Tried to reproduce the error using my x86 laptop but got hit by <a class="moz-txt-link-freetext" href="https://bugzilla.redhat.com/show_bug.cgi?id=1689216">https://bugzilla.redhat.com/show_bug.cgi?id=1689216</a> when trying to create the snapshot using upstream code: $ sudo ./run tools/virsh snapshot-create-as ub1810-cpu-hotplug snap error: operation failed: Failed to take snapshot: Error: Nested VMX virtualization does not support live migration yet Since this issue is arch independent I tried it out with a Power server. Here's the behavior with current upstream: - relevant piece of VM XML: <domain type='kvm'> <name>dhb</name> <uuid>d3fe0169-2be6-431c-85a5-cc178601bb69</uuid> <memory unit='KiB'>67108864</memory> <currentMemory unit='KiB'>67108864</currentMemory> <vcpu placement='static' current='4'>16</vcpu> $ sudo ./run tools/virsh setvcpus dhb 8 --live $ $ sudo ./run tools/virsh snapshot-create-as dhb snap1 Domain snapshot snap1 created $ $ sudo ./run tools/virsh snapshot-revert dhb snap1 $ $ sudo ./run tools/virsh dumpxml --inactive dhb <domain type='kvm'> <name>dhb</name> <uuid>d3fe0169-2be6-431c-85a5-cc178601bb69</uuid> <memory unit='KiB'>67108864</memory> <currentMemory unit='KiB'>67108864</currentMemory> <vcpu placement='static' current='8'>16</vcpu> <vcpus> The inactive XML got overwritten by the vcpu hotplug, which is not intended. After the patch, the problem isn't reproduced: the inactive VM XML was preserved after the snapshot-revert. Maxiwell, you (or the commiter) can add the following in the commit-msg: Fixes: <a class="moz-txt-link-freetext" href="https://bugzilla.redhat.com/show_bug.cgi?id=1689216">https://bugzilla.redhat.com/show_bug.cgi?id=1689216</a> Since the bug you're fixing here also fixes this RH bugzilla. Reviewed-by: Daniel Henrique Barboza <a class="moz-txt-link-rfc2396E" href="mailto:danielhb413@gmail.com"><danielhb413@gmail.com></a> Tested-by: Daniel Henrique Barboza <a class="moz-txt-link-rfc2396E" href="mailto:danielhb413@gmail.com"><danielhb413@gmail.com></a> <div class="moz-cite-prefix">On 4/30/19 2:54 PM, Maxiwell S. Garcia wrote: </div> <blockquote type="cite" cite="mid:20190430175401.19485-1-maxiwell@linux.ibm.com"> <pre class="moz-quote-pre" wrap="">Snapshot create operation saves the live XML and uses it to replace the domain definition in case of revert. But the VM config XML is not saved and the revert operation does not address this issue. This commit prevents the config XML from being overridden by snapshot definition. An active domain stores both current and new definitions. The current definition (vm->def) stores the live XML and the new definition (vm->newDef) stores the config XML. In an inactive domain, only the config XML is persistent, and it's saved in vm->def. The revert operation uses the virDomainObjAssignDef() to set the snapshot definition in vm->newDef, if domain is active, or in vm->def otherwise. But before that, it saves the old value to return to caller. This return is used here to restore the config XML after all snapshot startup process finish. Signed-off-by: Maxiwell S. Garcia <a class="moz-txt-link-rfc2396E" href="mailto:maxiwell@linux.ibm.com"><maxiwell@linux.ibm.com></a> --- src/qemu/qemu_driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b2ac737d1f..a73122454a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16251,6 +16251,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr inactiveConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_stopped = false; @@ -16465,7 +16466,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } if (config) { - virDomainObjAssignDef(vm, config, false, NULL); + virDomainObjAssignDef(vm, config, false, &inactiveConfig); virCPUDefFree(priv->origCPU); VIR_STEAL_PTR(priv->origCPU, origCPU); } @@ -16474,7 +16475,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, load: was_stopped = true; if (config) - virDomainObjAssignDef(vm, config, false, NULL); + virDomainObjAssignDef(vm, config, false, &inactiveConfig); /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. @@ -16533,6 +16534,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, detail); } } + if (inactiveConfig) + VIR_STEAL_PTR(vm->newDef, inactiveConfig); + break; case VIR_DOMAIN_SNAPSHOT_SHUTDOWN: @@ -16560,8 +16564,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuProcessEndJob(driver, vm); goto cleanup; } - if (config) - virDomainObjAssignDef(vm, config, false, NULL); + if (config) { + virDomainObjAssignDef(vm, config, false, &inactiveConfig); + if (inactiveConfig) + VIR_STEAL_PTR(vm->newDef, inactiveConfig); + } if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { </pre> </blockquote> </body> </html>