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

[Maxiwell S. Garcia]

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 at gmail.com>
> > Tested-by: Daniel Henrique Barboza <danielhb413 at 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.