12:54 p.m.
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(a)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
1:48 p.m.
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(a)gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413(a)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(a)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)) {
8:37 a.m.
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(a)gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413(a)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
9:38 a.m.
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
10:22 a.m.
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
10:49 a.m.
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
1:03 p.m.
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
1:17 p.m.
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
1:43 p.m.
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
10:20 a.m.
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(a)gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413(a)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.
2018
days inactive
2033
days old
9 comments
4 participants
participants (4)
-
Daniel Henrique Barboza -
Eric Blake -
Maxiwell S. Garcia -
Michal Privoznik