[libvirt] [PATCH 5/7] qemu: Wire up PR_MANAGER_STATUS_CHANGED event

[Peter Krempa]

On Wed, Jul 04, 2018 at 14:42:07 +0200, Michal Privoznik wrote:
> 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 at 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?

Well you can. On upgrade path you will need to determine what the old
default was and store it somewhere. The problem here is that you should
not determine the alias of the default managed pr helper by inspecting
the disks but you should rather store it somewhere else if you want
to. Specifically in the same place as the PID is stored. The object is
just one and it's a property of the VM not of the disk.

By storing it in the disk you don't help in the case when something gets
changed anyways. If you add something that adds a second managed
pr-helper you will need to modify the code which is doing the alias
detection anyways, so that argument does not make much sense either.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180704/51a8fbcc/attachment-0001.sig>