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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list