[libvirt] [PATCH v3 09/12] qemu: Start PR daemon on domain startup

[Peter Krempa]

On Wed, Mar 14, 2018 at 17:05:38 +0100, Michal Privoznik wrote:
> Before we exec() qemu we have to spawn pr-helper processes for
> all managed reservations (well, technically there can only one).
> The only caveat there is that we should place the process into
> the same namespace and cgroup as qemu (so that it shares the same
> view of the system). But we can do that only after we've forked.
> That means calling the setup function between fork() and exec().
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_process.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 169 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 57c06c7c1..b876d293a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2551,6 +2551,169 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
> +static int
> +qemuProcessSetupOnePRDaemonHook(void *opaque)

The naming does not make much sense here. There's only one managed
daemon.

> +{
> +    virDomainObjPtr vm = opaque;
> +    size_t i, nfds = 0;
> +    int *fds = NULL;
> +    int ret = -1;
> +
> +    if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0)
> +        return ret;
> +
> +    if (nfds > 0 &&
> +        virProcessSetNamespaces(nfds, fds) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    for (i = 0; i < nfds; i++)
> +        VIR_FORCE_CLOSE(fds[i]);
> +    VIR_FREE(fds);
> +    return ret;
> +}
> +
> +
> +static int
> +qemuProcessSetupOnePRDaemon(virDomainObjPtr vm,
> +                            virDomainDiskDefPtr disk)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
> +    virQEMUDriverConfigPtr cfg;
> +    qemuDomainStorageSourcePrivatePtr srcPriv;
> +    qemuDomainDiskPRDPtr prd;
> +    char *pidfile = NULL;
> +    pid_t cpid = -1;
> +    virCommandPtr cmd = NULL;
> +    virTimeBackOffVar timebackoff;
> +    const unsigned long long timeout = 500000; /* ms */
> +    int ret = -1;
> +
> +    if (priv->prPid != (pid_t) -1 ||
> +        !virStoragePRDefIsManaged(disk->src->pr))
> +        return 0;

This function should ever be called only for the managed PR daemon and
only once per domain. This looks wrong and makes the caller look that
multiple daemons are setup.

Please rename it and move this logic to the caller, which determines
whether the managed daemon is necessary and calls this exactly once if
it is.

> +
> +    cfg = virQEMUDriverGetConfig(driver);
> +    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +    prd = srcPriv->prd;
> +
> +    if (!virFileIsExecutable(cfg->prHelperName)) {
> +        virReportSystemError(errno, _("'%s' is not a suitable pr helper"),
> +                             cfg->prHelperName);
> +        goto cleanup;
> +    }
> +
> +    if (!(pidfile = virPidFileBuildPath(cfg->stateDir, prd->alias)))
> +        goto cleanup;
> +
> +    if (unlink(pidfile) < 0 &&
> +        errno != ENOENT) {
> +        virReportSystemError(errno,
> +                             _("Cannot remove stale PID file %s"),
> +                             pidfile);
> +        goto cleanup;
> +    }
> +
> +    if (!(cmd = virCommandNewArgList(cfg->prHelperName,
> +                                     "-k", prd->path,
> +                                     NULL)))
> +        goto cleanup;
> +
> +    virCommandDaemonize(cmd);
> +    virCommandSetPidFile(cmd, pidfile);
> +    /* The only caveat there is that we should place the process
> +     * into the same namespace and cgroup as qemu (so that it
> +     * shares the same view of the system). */
> +    virCommandSetPreExecHook(cmd, qemuProcessSetupOnePRDaemonHook, vm);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    if (virPidFileReadPath(pidfile, &cpid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("pr helper %s didn't show up"), prd->alias);
> +        goto cleanup;
> +    }
> +
> +    if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0)
> +        goto cleanup;
> +    while (virTimeBackOffWait(&timebackoff)) {
> +        if (virFileExists(prd->path))
> +            break;
> +
> +        if (virProcessKill(cpid, 0) == 0)
> +            continue;
> +
> +        virReportSystemError(errno,
> +                             _("pr helper %s died unexpectedly"), prd->alias);
> +        goto cleanup;
> +    }

This code does not do anything on timeout.

> +
> +    if (priv->cgroup &&
> +        virCgroupAddMachineTask(priv->cgroup, cpid) < 0)
> +        goto cleanup;
> +
> +    if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> +                                       vm->def, prd->path, true) < 0)
> +        goto cleanup;

So this function will probably fail with a weird error message.

> +
> +    priv->prPid = cpid;
> +    ret = 0;
> + cleanup:
> +    if (ret < 0) {
> +        virCommandAbort(cmd);
> +        virProcessKillPainfully(cpid, true);
> +    }
> +    virCommandFree(cmd);
> +    if (pidfile) {
> +        if (unlink(pidfile) < 0 &&
> +            errno != ENOENT &&
> +            !virGetLastError())
> +            virReportSystemError(errno,
> +                                 _("Cannot remove stale PID file %s"),
> +                                 pidfile);
> +        VIR_FREE(pidfile);

By removing the pidfile, you can't make sure later that the PR helper
program is still the same process, thus when killing the VM by the pid
number only you might actually kill a different process.

I unfortunately already deleted the top of the message so you'll need to
fix the function qemuProcessKillPRDaemon to see whether it's the same
process, similarly to what we do when reconnecting to qemu.

> +    }
> +    virObjectUnref(cfg);
> +    return ret;
> +}
> +
> +
> +static int
> +qemuProcessSetupPRDaemon(virDomainObjPtr vm)
> +{
> +    size_t i;
> +    int ret = -1;
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        if (qemuProcessSetupOnePRDaemon(vm, vm->def->disks[i]) < 0)
> +            goto cleanup;

This looks wrong, since we only do one PR daemon. This function should
determine whether one is needed and call that function only once.

> +    }
> +
> +    ret = 0;
> + cleanup:
> +    if (ret < 0)
> +        qemuProcessKillPRDaemon(vm);
>      return ret;
>  }
>  
-------------- 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/20180315/052b3685/attachment-0001.sig>