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

[Dmitry Andreev]

On 16.12.2015 16:26, Peter Krempa wrote:
> 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.
I'll try to be more verbose in commit messages next time.
>
>>
>> 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".
>
Thank you for the complete explanation, it helps me to understand that
in most cases we have <STATE>_FROM_SNAPSHOT event and don't need
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.
>
And yes, this is the case that has no events.
> 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.
>
I'll describe what problem I tried to solve. We use libvirt's API and
store vm config on the client side. Each time we get DEFINED event we
rewrite stored config. Now I know that we should watch for
<STATE>_FROM_SNAPSHOT events also.

But the case with two offline states has no events and we have a
problem. bz 1081148 confirmed that we are not alone and virt-manager
has the same problem. Any event about a snapshot will be enough to
solve my problem.

My mistake was that I thought there should be DEFINED event before any
<STATE>_FROM_SNAPSHOT. Now I think that there could be
STOPPED_FROM_SNAPSHOT or DEFINED event in case of offline target states.