Re: [libvirt] [PATCH 5/7] qemu: Wire up PR_MANAGER_STATUS_CHANGED event
On 07/04/2018 01:42 PM, Peter Krempa wrote:
On Wed, Jul 04, 2018 at 12:46:53 +0200, Michal Privoznik wrote:
> This event is emitted on the monitor if one of pr-managers lost
> connection to its pr-helper process. What libvirt needs to do is
> restart the pr-helper process iff it corresponds to managed
> pr-manager.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_monitor.c | 15 +++++++++++++
> src/qemu/qemu_monitor.h | 11 +++++++++
> src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++
> src/qemu/qemu_process.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 102 insertions(+)
[...]
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f200729cb1..94b7de76d7 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1615,6 +1615,58 @@ qemuProcessHandleDumpCompleted(qemuMonitorPtr mon
ATTRIBUTE_UNUSED,
> }
>
>
> +static int
> +qemuProcessHandlePRManagerStatusChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm,
> + const char *prManager,
> + bool connected,
> + void *opaque ATTRIBUTE_UNUSED)
> +{
> + qemuDomainObjPrivatePtr priv;
> + size_t i;
> + int ret = -1;
> +
> + virObjectLock(vm);
> +
> + VIR_DEBUG("pr-manager %s status changed for domain %p %s
connected=%d",
> + prManager, vm, vm->def->name, connected);
> +
> + if (connected) {
> + /* Connect events are boring. */
> + ret = 0;
> + goto cleanup;
> + }
> + /* Disconnect events are more interesting. */
> +
> + for (i = 0; i < vm->def->ndisks; i++) {
> + const char *mgralias;
> +
> + mgralias =
virStorageSourceChainGetManagedPRAlias(vm->def->disks[i]->src);
> +
> + if (STREQ_NULLABLE(prManager, mgralias))
> + break;
I'm not a fan of this. We always know which is the managed alias
Do we? I thought the whole point of storing it in source private data
was to make it independent of qemuDomainGetManagedPRAlias().
Imagine I started a domain with pr-manager.alias = pr-helper0. Then
restarted libvirtd to upgrade it. This new version has, however,
pr-helper1 as default alias (because reasons). Then all of a sudden I
receive this event with alias pr-helper0 (because that is how I started
the domain). I can't do plain:
STREQ(prManager, qemuDomainGetManagedPRAlias())
can I?
and we
also know if it is supposed to be running.
The hotplug code already does not inspect disks to do this since there
is only one instance.
Sure. But hotplug code does not have to care about old instances.
> + }
> +
> + if (i == vm->def->ndisks) {
> + VIR_DEBUG("pr-manager %s not managed, ignoring event",
> + prManager);
> + ret = 0;
> + goto cleanup;
> + }
> +
> + priv = vm->privateData;
> + priv->prDaemonRunning = false;
> +
> + if (qemuProcessStartManagedPRDaemon(vm) < 0)
This has a timeout built in. Thus executing this from the event loop
will make the whole libvirtd get stuck until it starts. This should not
be in the event loop.
Ah good point. I'll rewrite this to run it in the qemu event process
thread (or what do we call it).
Also does every disconnect equal to the daemon crashing/stopping?
It should. If qemu sends us bogus events I'm not sure there's much we
can do about it.
Michal