[libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot

[Peter Krempa]

On Wed, Dec 16, 2015 at 07:28:43 -0500, John Ferlan wrote:
> 
> 
> On 12/16/2015 07:01 AM, Peter Krempa wrote:
> > On Wed, Dec 16, 2015 at 12:09:42 +0300, Dmitry Andreev wrote:
> >> If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
> > 
> > drop "file" here. The important part is the config itself being changed.
> > The file is just a implication of that.
> > 
> > Additionally, commit messages here or in the previous patch don't
> > provide any justification for this change. I'd like to have a statement
> > why is this important to change.
> > 
> 
> FWIW: This is what I got from the cover letter:

The cover letter is not commited to the repo so it shouldn't be the only
place to put such information. I usualy don't even bother to read the
cover letter.

> 
> Lack of the event become a problem for virt-manager
> https://bugzilla.redhat.com/show_bug.cgi?id=1081148

Okay, that's starting to be reasonable let's say, but ...
> 
> 
> Without the event, it seems virt-manager doesn't "see" the change and
> presents the data prior to snapshot

With internal snapshots there's quite a few state changes that can
happen according to the source and target state that need to be taken
into account.

If you follow the snapshot code you'll see that in some cases qemu has
to be restarted and in some cases not. In case when qemu is not
restarted during reversion we should not emit a lifecycle event.

Stop/start of qemu is based on the fact that the "ABI stability"
check of the current and target config will not match. This leaves a
loophole where e.g. change of a disk source would not be properly
tracked. This problem will most likely happen when combining external
and internal snapshots
( https://bugzilla.redhat.com/show_bug.cgi?id=880565 ), but also other
legitimate changes of config (mostly change of disk source) will not be
tracked properly. This is a bug in the snapshot handling code though and
qemu should be restarted at that point since it needs to open different
files and libvirt needs to label them prior to that. As said, this is a
bug in the snapshot code and patching it by doing an event is not right.

Additionally if you are going to revert a transient VM snapshot from
running state to running state the DEFINED event is totaly bogus since
it doesn't have a persistent definition, which is where we refer to it
as "DEFINED".

In case of offline target states this approach may be valid since the
config changes without any notification which is actually what the bug
is complaining about. Using the defined event should be used only in
such case.

One thing to consider then is whether it's worth fixing what bz 1081148
is tracking with this (but the other cases described above should not
emit the event then), or whether a different approach is desired. That's
why I asked for justification.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151216/ada3e9b2/attachment-0001.sig>