[libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert
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 <maxiwell@linux.ibm.com> --- 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)) { -- 2.20.1
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 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: https://bugzilla.redhat.com/show_bug.cgi?id=1689216 Since the bug you're fixing here also fixes this RH bugzilla. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 4/30/19 2:54 PM, Maxiwell S. Garcia wrote:
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 <maxiwell@linux.ibm.com> --- 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)) {
On 5/13/19 8:48 PM, Daniel Henrique Barboza wrote:
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 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: https://bugzilla.redhat.com/show_bug.cgi?id=1689216
Since the bug you're fixing here also fixes this RH bugzilla.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Actually, I think that this is not the proper fix. The proper fix would be to store both active AND inactive XMLs when creating a snapshot and then restore them both on snapshot revert. While this fix may fix one use case, it's not dealing with the issue properly IMO. But I'm not snapshot expert really. Michal
On 5/14/19 8:37 AM, Michal Privoznik wrote:
On 5/13/19 8:48 PM, Daniel Henrique Barboza wrote:
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 when trying to create the snapshot using upstream code:
Actually, I think that this is not the proper fix. The proper fix would be to store both active AND inactive XMLs when creating a snapshot and then restore them both on snapshot revert. While this fix may fix one use case, it's not dealing with the issue properly IMO. But I'm not snapshot expert really.
Sadly, if we were to modify snapshot XML to store both the active and inactive XML with a snapshot, all existing snapshots of a running guest would be incomplete (right now, the snapshot XML only allows the storage of a single <domain> sub-element). I think the best we can do is store an active XML with a snapshot of an active domain, and an inactive XML with a snapshot of an offline domain. Reverting to an offline snapshot only has to worry about inactive XML; reverting to an online snapshot needs the active XML to properly restore the domain, but we then have to decide whether to also update the inactive XML to match the fact that the active XML has changed. So changing both inactive and active XML to describe the same state during a revert to a snapshot of an active domain seems like the best approach to me, short of rewriting snapshot XML to store two different domain definitions at once. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, May 14, 2019 at 09:38:12AM -0500, Eric Blake wrote:
On 5/14/19 8:37 AM, Michal Privoznik wrote:
On 5/13/19 8:48 PM, Daniel Henrique Barboza wrote:
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 when trying to create the snapshot using upstream code:
Actually, I think that this is not the proper fix. The proper fix would be to store both active AND inactive XMLs when creating a snapshot and then restore them both on snapshot revert. While this fix may fix one use case, it's not dealing with the issue properly IMO. But I'm not snapshot expert really.
Sadly, if we were to modify snapshot XML to store both the active and inactive XML with a snapshot, all existing snapshots of a running guest would be incomplete (right now, the snapshot XML only allows the storage of a single <domain> sub-element). I think the best we can do is store an active XML with a snapshot of an active domain, and an inactive XML with a snapshot of an offline domain. Reverting to an offline snapshot only has to worry about inactive XML; reverting to an online snapshot needs the active XML to properly restore the domain, but we then have to decide whether to also update the inactive XML to match the fact that the active XML has changed. So changing both inactive and active XML to describe the same state during a revert to a snapshot of an active domain seems like the best approach to me, short of rewriting snapshot XML to store two different domain definitions at once.
Hi Eric, The actual behavior of ‘snapshot revert’ changes both inactive and active XML to describe the same state saved in the ‘snapshot create’. The steps below were commented in a bugzilla: 1. Create a guest say VM1 2. Attach a virtual NIC to the guest : virsh attach-device VM1 network.xml --live interface will be seen in the guest. 3. Take a snapshot of the guest : virsh snapshot-create VM1 Domain snapshot 1505299135 created 4. Destroy the guest 5. Now, revert back the snapshot: virsh snapshot-revert VM1 1505299135 Interface is seen in the guest. 6. Now destroy the guest and start it again Interface is still seen in the guest. It is making the attach persistent. So, after a revert, the guest always loses the inactive configuration. Why do you think this behavior is the best approach?
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 5/15/19 10:22 AM, Maxiwell S. Garcia wrote:
On Tue, May 14, 2019 at 09:38:12AM -0500, Eric Blake wrote:
On 5/14/19 8:37 AM, Michal Privoznik wrote:
On 5/13/19 8:48 PM, Daniel Henrique Barboza wrote:
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 when trying to create the snapshot using upstream code:
Actually, I think that this is not the proper fix. The proper fix would be to store both active AND inactive XMLs when creating a snapshot and then restore them both on snapshot revert. While this fix may fix one use case, it's not dealing with the issue properly IMO. But I'm not snapshot expert really.
Sadly, if we were to modify snapshot XML to store both the active and inactive XML with a snapshot, all existing snapshots of a running guest would be incomplete (right now, the snapshot XML only allows the storage of a single <domain> sub-element). I think the best we can do is store an active XML with a snapshot of an active domain, and an inactive XML with a snapshot of an offline domain. Reverting to an offline snapshot only has to worry about inactive XML; reverting to an online snapshot needs the active XML to properly restore the domain, but we then have to decide whether to also update the inactive XML to match the fact that the active XML has changed. So changing both inactive and active XML to describe the same state during a revert to a snapshot of an active domain seems like the best approach to me, short of rewriting snapshot XML to store two different domain definitions at once.
Hi Eric,
The actual behavior of ‘snapshot revert’ changes both inactive and active XML to describe the same state saved in the ‘snapshot create’. The steps below were commented in a bugzilla:
1. Create a guest say VM1 2. Attach a virtual NIC to the guest : virsh attach-device VM1 network.xml --live interface will be seen in the guest. 3. Take a snapshot of the guest : virsh snapshot-create VM1 Domain snapshot 1505299135 created 4. Destroy the guest 5. Now, revert back the snapshot: virsh snapshot-revert VM1 1505299135 Interface is seen in the guest. 6. Now destroy the guest and start it again Interface is still seen in the guest. It is making the attach persistent.
So, after a revert, the guest always loses the inactive configuration. Why do you think this behavior is the best approach?
In your sequence, at point 2, the live XML differs from the inactive XML (the live reflects the hot-plug, the inactive will revert to the state at the time of the creation). You are correct that at step 5, we are updating the inactive XML in addition to the live XML. But consider this counter-sequence: 1. Create guest 2. Take snapshot 3. Hotplug a device 4. Revert to the snapshot There, we MUST make sure that the revert does NOT have the hotplugged device (since it was added only after the snapshot was created). Then couple it with the fact that you can have: 1. Create guest 2. Take snapshot 3. Define new inactive XML that includes new device If you reboot the guest, the inactive XML will become active and the guest will have a new device. But if the guest continues running without rebooting, the inactive XML will be different from the live XML. Whether or not the guest rebooted, a revert must NOT expose the new device to the live XML installed during the revert. But there is still a question as to whether the revert should also undo the inactive XML change that was made after the snapshot was created, or leave the inactive XML alone (that is, after we revert, will a fresh boot once again pick up the new device, or will the fresh boot be stuck with the configuration as though step 3 had never happened). I can buy the argument that when reverting to a live state, libvirt could be changed to only revert the live XML and leave the inactive XML unchanged. But will that break any existing clients that have come to rely on several years of behavior where reverting sets both XML at once? Or is it really that many years that we've had a bug and no one has noticed it until now? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 5/15/19 12:49 PM, Eric Blake wrote:
If you reboot the guest, the inactive XML will become active and the guest will have a new device. But if the guest continues running without rebooting, the inactive XML will be different from the live XML. Whether or not the guest rebooted, a revert must NOT expose the new device to the live XML installed during the revert. But there is still a question as to whether the revert should also undo the inactive XML change that was made after the snapshot was created, or leave the inactive XML alone (that is, after we revert, will a fresh boot once again pick up the new device, or will the fresh boot be stuck with the configuration as though step 3 had never happened).
Maybe we should consider that, in Libvirt, the snapshot state consists of both inactive and live XMLs. Like Michal suggested in his first reply. It appears to be the most consistent way of dealing with the revert of a VM state - the change is that the VM state is now both inactive and active XMLs. This would cover all possible scenarios (at least the ones I can think of) - we will not overwrite the inactive XML with stuff that weren't supposed to be there ("but I used setvcpus --live, why the extra vcpus are showing in the offline XML after revert") and the other way around ("I hotplugged with --config, but after the revert my vcpus are gone"). This would mean that yeah, a user made change in the inactive XML will be overwritten in a revert operation with the inactive XML of the snapshot. The documentation already states that this would be case though: --- snapshot-revert domain {snapshot | --current} [{--running | --paused}] [--force] Revert the given domain to the snapshot specified by snapshot, or to the current snapshot with --current. Be aware that this is a destructive action; any changes in the domain since the last snapshot was taken will be lost. Also note that the state of the domain after snapshot-revert is complete will be the state of the domain at the time the original snapshot was taken. ---- "any changes in the domain since the last snapshot was taken will be lost". This includes the inactive XML, which is a domain change.
I can buy the argument that when reverting to a live state, libvirt could be changed to only revert the live XML and leave the inactive XML unchanged. But will that break any existing clients that have come to rely on several years of behavior where reverting sets both XML at once? Or is it really that many years that we've had a bug and no one has noticed it until now?
I can't comment on how most users expect snapshot-revert to work, but I would say that the initial bug that Maxiwell is addressing here (for reference: https://bugzilla.redhat.com/show_bug.cgi?id=1662588 ) is something that users will complain about if they face it. It is annoying, at the very least. Hopefully, at least in my estimation, taking both XMLs into consideration when snapshot reverting fixes this bug without creating other ones. Maxiwell, have you had the chance to assess how hard would it be to make such change? Thanks, DHB
On 5/15/19 1:03 PM, Daniel Henrique Barboza wrote:
On 5/15/19 12:49 PM, Eric Blake wrote:
If you reboot the guest, the inactive XML will become active and the guest will have a new device. But if the guest continues running without rebooting, the inactive XML will be different from the live XML. Whether or not the guest rebooted, a revert must NOT expose the new device to the live XML installed during the revert. But there is still a question as to whether the revert should also undo the inactive XML change that was made after the snapshot was created, or leave the inactive XML alone (that is, after we revert, will a fresh boot once again pick up the new device, or will the fresh boot be stuck with the configuration as though step 3 had never happened).
Maybe we should consider that, in Libvirt, the snapshot state consists of both inactive and live XMLs.
Right now, that is untrue. The snapshot state consists of a single XML (either the live XML for a live or disk-only snapshot, or the inactive XML for an offline snapshot). We could make it true, but it is an invasive change and we'd still have to cope with existing live snapshots that didn't have both stored.
Like Michal suggested in his first reply. It appears to be the most consistent way of dealing with the revert of a VM state - the change is that the VM state is now both inactive and active XMLs.
Reverting to an offline or disk-only snapshot is easy - there's only one XML to worry about (once you've reverted, the domain is offline, so there is no live XML - unless the revert command also included the flag to start the domain in which case the live XML will match the just-reverted offline XML). Reverting to an online snapshot is trickier (right now, we MUST overwrite the live XML to perform the revert correctly, and we HAPPEN to overwrite the inactive XML as well) - but there you can argue that we've merely been buggy for a few years, and that we should leave the inactive XML untouched in that case. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 5/15/19 3:17 PM, Eric Blake wrote:
On 5/15/19 1:03 PM, Daniel Henrique Barboza wrote:
On 5/15/19 12:49 PM, Eric Blake wrote:
If you reboot the guest, the inactive XML will become active and the guest will have a new device. But if the guest continues running without rebooting, the inactive XML will be different from the live XML. Whether or not the guest rebooted, a revert must NOT expose the new device to the live XML installed during the revert. But there is still a question as to whether the revert should also undo the inactive XML change that was made after the snapshot was created, or leave the inactive XML alone (that is, after we revert, will a fresh boot once again pick up the new device, or will the fresh boot be stuck with the configuration as though step 3 had never happened). Maybe we should consider that, in Libvirt, the snapshot state consists of both inactive and live XMLs.
Right now, that is untrue. The snapshot state consists of a single XML (either the live XML for a live or disk-only snapshot, or the inactive XML for an offline snapshot). We could make it true, but it is an invasive change and we'd still have to cope with existing live snapshots that didn't have both stored.
Good point. In case we go on with this change, the current behavior should be maintained for the existing live snaps that doesn't have the inactiveXML information.
Like Michal suggested in his first reply. It appears to be the most consistent way of dealing with the revert of a VM state - the change is that the VM state is now both inactive and active XMLs. Reverting to an offline or disk-only snapshot is easy - there's only one XML to worry about (once you've reverted, the domain is offline, so there is no live XML - unless the revert command also included the flag to start the domain in which case the live XML will match the just-reverted offline XML). Reverting to an online snapshot is trickier (right now, we MUST overwrite the live XML to perform the revert correctly, and we HAPPEN to overwrite the inactive XML as well) - but there you can argue that we've merely been buggy for a few years, and that we should leave the inactive XML untouched in that case.
The solution will need to consider all this cases, including an offline domain being reverted to a live domain and so on. I didn't see the code thus I don't know how hard it is to implement it consistently. Worst case scenario, if a new solution (snap with both XMLs for example) is proven to be too hard or too error prone, I'd say that keeping what we already do, but with a documentation amend saying that snapshot-revert will overwrite the inactiveXML with the live XML that is being loaded, suffices. At least the user can be aware of what is happening and that the behavior is expected. Thanks, DHB
On Tue, May 14, 2019 at 03:37:26PM +0200, Michal Privoznik wrote:
On 5/13/19 8:48 PM, Daniel Henrique Barboza wrote:
Tried to reproduce the error using my x86 laptop but got hit by https://bugzilla.redhat.com/show_bug.cgi?id=1689216 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: https://bugzilla.redhat.com/show_bug.cgi?id=1689216
Since the bug you're fixing here also fixes this RH bugzilla.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Actually, I think that this is not the proper fix. The proper fix would be to store both active AND inactive XMLs when creating a snapshot and then restore them both on snapshot revert. While this fix may fix one use case, it's not dealing with the issue properly IMO. But I'm not snapshot expert really.
Michal
Hi Michal, My patch only revert the ‘snapshot XML’ in the active domain and do not touch in the inactive XML. A limitation is that many snapshots will have the same inactive XML (the last one). Maybe save both inactive and active XML with ‘snapshot create’ is an answer.
participants (4)
-
Daniel Henrique Barboza -
Eric Blake -
Maxiwell S. Garcia -
Michal Privoznik