[libvirt] [PATCHv2 0/2] notify about reverting to snapshot
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
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
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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.
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);
participants (3)
-
Dmitry Andreev -
John Ferlan -
Peter Krempa