[libvirt] [PATCH v2 16/17] vircgroupv2: use dummy process to workaround kernel bug with systemd

[Michal Privoznik]

On 1/14/19 4:47 PM, Pavel Hrdina wrote:
> If some program is attached to cgroup and that cgroup is removed before
> detaching the program it might not be freed and will remain in the
> system until it's rebooted.
> 
> This would not be that big deal to workaround if we wouldn't use
> machined to track our guests.  Systemd tries to be the nice guy and if
> there is no process attached to TransientUnit it will destroy the unit
> and remove the cgroup from system which will hit the kernel bug.
> 
> In order to prevent this behavior of systemd we can move another process
> into the guest cgroup and the cgroup will remain active even if we kill
> qemu process while destroying guest.  We can then detach our BPF program
> from that cgroup and call machined terminate API to cleanup the cgroup.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/libvirt_private.syms      |  1 +
>  src/lxc/lxc_cgroup.c          |  1 +
>  src/qemu/qemu_cgroup.c        |  6 ++-
>  src/util/vircgroup.c          | 16 ++++++++
>  src/util/vircgroup.h          |  1 +
>  src/util/vircgrouppriv.h      |  2 +
>  src/util/vircgroupv2.c        |  2 +
>  src/util/vircgroupv2devices.c | 70 ++++++++++++++++++++++++++++++++++-
>  src/util/virsystemd.c         |  2 +-
>  src/util/virsystemd.h         |  2 +
>  10 files changed, 99 insertions(+), 4 deletions(-)


> +static pid_t
> +virCgroupV2DevicesStartDummyProc(virCgroupPtr group)
> +{
> +    pid_t ret = -1;
> +    pid_t pid = -1;
> +    virCommandPtr cmd = NULL;
> +    VIR_AUTOFREE(char *) pidfile = NULL;
> +    VIR_AUTOFREE(char *) fileName = NULL;
> +
> +    if (virAsprintf(&fileName, "cgroup-%s", group->vmName) < 0)
> +        return -1;
> +
> +    pidfile = virPidFileBuildPath(group->stateDir, fileName);
> +    if (!pidfile)
> +        return -1;
> +
> +    cmd = virCommandNewArgList("/usr/bin/tail", "-f", "/dev/null",
> +                               "-s", "3600", NULL);
> +    if (!cmd)
> +        return -1;
> +
> +    virCommandDaemonize(cmd);
> +    virCommandSetPidFile(cmd, pidfile);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    if (virPidFileReadPath(pidfile, &pid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to start dummy cgroup helper"));
> +        goto cleanup;
> +    }
> +
> +    if (virCgroupAddMachineProcess(group, pid) < 0)
> +        goto cleanup;
> +
> +    ret = pid;
> + cleanup:
> +    if (ret < 0) {
> +        virCommandAbort(cmd);
> +        if (pid > 0)
> +            virProcessKillPainfully(pid, true);
> +    }
> +    if (pidfile)
> +        unlink(pidfile);
> +    virCommandFree(cmd);
> +    return ret;
> +}

I don't think we should bother with this. This is clearly a kernel bug
and once fixed (which we can't detect) we would still be placing this
'tail' into the cgroup for nothing. I know we have lots of workarounds
for bugs in other libraries (and even probably kernels too) in our code,
but we should start getting rid of those instead of introducing new ones.

Michal