[libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Maxiwell S. Garcia posted 1 patch 11 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190430175401.19485-1-maxiwell@linux.ibm.com
src/qemu/qemu_driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

[libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Maxiwell S. Garcia 11 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Daniel Henrique Barboza 10 weeks ago
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)) {

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Michal Privoznik 10 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Maxiwell S. Garcia 9 weeks ago
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.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Eric Blake 10 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Maxiwell S. Garcia 9 weeks ago
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
> 



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Eric Blake 9 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Daniel Henrique Barboza 9 weeks ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Eric Blake 9 weeks ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

Posted by Daniel Henrique Barboza 9 weeks ago

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



>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list