3:09 a.m.
Reverting to snapshot may change domain configuration but
there will be no events about that.
Lack of the event become a problem for virt-manager
https://bugzilla.redhat.com/show_bug.cgi?id=1081148
This patch-set introduces new event and emits it in
qemuDomainRevertToSnapshot.
v2:
[2/2] Reworked. John noted that in some error cases I
send 'defined' event without changes in configuration.
Dmitry Andreev (2):
Introduce new VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT event
qemu: emit 'defined' event after reverted to snapshot
examples/object-events/event-test.c | 2 ++
include/libvirt/libvirt-domain.h | 1 +
src/qemu/qemu_driver.c | 13 +++++++++++--
tools/virsh-domain.c | 3 ++-
4 files changed, 16 insertions(+), 3 deletions(-)
--
1.8.3.1
3:09 a.m.
New subject: [libvirt] [PATCHv2 1/2] Introduce new VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT event
This should be emitted whenever a domain is reverted to snapshot.
---
examples/object-events/event-test.c | 2 ++
include/libvirt/libvirt-domain.h | 1 +
tools/virsh-domain.c | 3 ++-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c
index dcae981..afac100 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -110,6 +110,8 @@ static const char *eventDetailToString(int event, int detail) {
ret = "Updated";
else if (detail == VIR_DOMAIN_EVENT_DEFINED_RENAMED)
ret = "Renamed";
+ else if (detail == VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT)
+ ret = "Snapshot";
break;
case VIR_DOMAIN_EVENT_UNDEFINED:
if (detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a1ea6a5..f3d360b 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2330,6 +2330,7 @@ typedef enum {
VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */
VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1, /* Changed config file */
VIR_DOMAIN_EVENT_DEFINED_RENAMED = 2, /* Domain was renamed */
+ VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT = 3, /* Config file was restored from a
snapshot */
# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_DEFINED_LAST
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 7650535..69f4792 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11910,7 +11910,8 @@ VIR_ENUM_IMPL(virshDomainEventDefined,
VIR_DOMAIN_EVENT_DEFINED_LAST,
N_("Added"),
N_("Updated"),
- N_("Renamed"))
+ N_("Renamed"),
+ N_("Snapshot"))
VIR_ENUM_DECL(virshDomainEventUndefined)
VIR_ENUM_IMPL(virshDomainEventUndefined,
--
1.8.3.1
3:09 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
If config file was changed VIR_DOMAIN_EVENT_DEFINED should be emitted
---
src/qemu/qemu_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..1474eaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_DOMAIN_EVENT_STARTED,
detail);
if (rc < 0)
- goto endjob;
+ goto defined;
}
/* Touch up domain state. */
@@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
- goto endjob;
+ goto defined;
}
rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
@@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
ret = 0;
+ defined:
+ if (config) {
+ virObjectEventPtr define_event;
+ define_event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
+ qemuDomainEventQueue(driver, define_event);
+ }
+
endjob:
qemuProcessEndJob(driver, vm);
--
1.8.3.1
6:01 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
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.
---
src/qemu/qemu_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..1474eaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_DOMAIN_EVENT_STARTED,
detail);
if (rc < 0)
- goto endjob;
+ goto defined;
}
/* Touch up domain state. */
@@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
- goto endjob;
+ goto defined;
}
rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
@@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
ret = 0;
+ defined:
+ if (config) {
+ virObjectEventPtr define_event;
+ define_event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
+ qemuDomainEventQueue(driver, define_event);
+ }
This will emit the event even if the configuration itself didn't change.
We do a similar thing when you switch off the VM. The next-start config
replaces the current config. This is implied by the events of starting
and stopping of the VM.
Since the internal snapshot code doesn't restart qemu in some cases,
that's when the configuration can't change (which actually might be
implemented wrong in a few places). For snapshot state transitions that
change domain state, the appropriate events are already used.
As of the reasons above, I'd like you to provide some justification why
this is a good thing to do.
Peter
6:28 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
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:
Lack of the event become a problem for virt-manager
https://bugzilla.redhat.com/show_bug.cgi?id=1081148
Without the event, it seems virt-manager doesn't "see" the change and
presents the data prior to snapshot
John
> ---
> src/qemu/qemu_driver.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 783a7cd..1474eaa 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> VIR_DOMAIN_EVENT_STARTED,
> detail);
> if (rc < 0)
> - goto endjob;
> + goto defined;
> }
>
> /* Touch up domain state. */
> @@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> if (!virDomainObjIsActive(vm)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("guest unexpectedly quit"));
> - goto endjob;
> + goto defined;
> }
> rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
> VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
> @@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>
> ret = 0;
>
> + defined:
> + if (config) {
> + virObjectEventPtr define_event;
> + define_event = virDomainEventLifecycleNewFromObj(vm,
> + VIR_DOMAIN_EVENT_DEFINED,
> + VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
> + qemuDomainEventQueue(driver, define_event);
> + }
This will emit the event even if the configuration itself didn't change.
We do a similar thing when you switch off the VM. The next-start config
replaces the current config. This is implied by the events of starting
and stopping of the VM.
Since the internal snapshot code doesn't restart qemu in some cases,
that's when the configuration can't change (which actually might be
implemented wrong in a few places). For snapshot state transitions that
change domain state, the appropriate events are already used.
As of the reasons above, I'd like you to provide some justification why
this is a good thing to do.
Peter
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:26 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
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
9:15 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
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.
6:21 a.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: emit 'defined' event after reverted to snapshot
On 12/16/2015 04:09 AM, Dmitry Andreev wrote:
If config file was changed VIR_DOMAIN_EVENT_DEFINED should be
emitted
---
src/qemu/qemu_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
I liked v1 better with a tweak or two
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 783a7cd..1474eaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15512,7 +15512,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
VIR_DOMAIN_EVENT_STARTED,
detail);
if (rc < 0)
- goto endjob;
+ goto defined;
}
/* Touch up domain state. */
@@ -15534,7 +15534,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
- goto endjob;
+ goto defined;
There's done more goto endjob after here which would seemingly skip the
defined event.
There's also another "if (config)" in the SHUTDOWN, SHUTOFF, CRASHED
case that would be missed due to a goto cleanup
I can fiddle with v1 slightly and have it do the right thing.
John
}
rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn,
VIR_DOMAIN_RUNNING_FROM_SNAPSHOT,
@@ -15636,6 +15636,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
ret = 0;
+ defined:
+ if (config) {
+ virObjectEventPtr define_event;
+ define_event = virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_DEFINED,
+ VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT);
+ qemuDomainEventQueue(driver, define_event);
+ }
+
endjob:
qemuProcessEndJob(driver, vm);
3517
days inactive
3517
days old
7 comments
3 participants
participants (3)
-
Dmitry Andreev -
John Ferlan -
Peter Krempa