[libvirt PATCH v3] cgroup/LXC: Do not condition availability of v2 by controllers

[Michal Prívozník]

On 10/23/22 14:08, Eric van Blokland wrote:
> systemd in hybrid mode uses v1 hierarchies for controllers and v2 for
> process tracking.
> 
> The LXC code uses virCgroupAddMachineProcess() to move processes into
> appropriate cgroup by manipulating cgroupfs directly. (Note, despite
> libvirt also supports talking to systemd directly via
> org.freedesktop.machine1 API.)
> 
> If this path is taken, libvirt/lxc must convince systemd that processes
> really belong to new cgroup, i.e. also the tracking v2 hierarchy must
> undergo migration too.
> 
> The current check would evaluate v2 backend as unavailable with hybrid
> mode (because there are no available controllers). Simplify the
> condition and consider the mounted cgroup2 as sufficient to touch v2
> hierarchy.
> 
> This consequently creates an issue with binding the V2 mount. In hybrid
> mode the V2 filesystem may be mounted upon the V1 filesystem. By reversing
> the order in which backends are mounted in virCgroupBindMount this problem
> is circumvented.
> 
> Fixes: #182

We prefer full URL because it enables to just click the link when
viewing git log.

Also, v3? Somehow I missed v2 and v1 :-D

> Signed-off-by: Eric van Blokland <mail at ericvanblokland.nl>
> ---
>  src/util/vircgroup.c   |  8 +++++---
>  src/util/vircgroupv2.c | 12 ------------
>  2 files changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index a6a409af3d..48fbcf625a 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -2924,9 +2924,11 @@ virCgroupBindMount(virCgroup *group, const char *oldroot,
>      size_t i;
>      virCgroup *parent = virCgroupGetNested(group);
>  
> -    for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> -        if (parent->backends[i] &&
> -            parent->backends[i]->bindMount(parent, oldroot, mountopts) < 0) {
> +    /* In hybrid environments, V2 may be mounted over V1.
> +     * Mount the backends in reverse order. */
> +    for (i = 1; i <= VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> +        if (parent->backends[VIR_CGROUP_BACKEND_TYPE_LAST - i] &&
> +            parent->backends[VIR_CGROUP_BACKEND_TYPE_LAST - i]->bindMount(parent, oldroot, mountopts) < 0) {

This works, but I'd rather have the variable count down. For that we'd
need to change the type of it from size_t to ssize_t but that's okay.

>              return -1;
>          }
>      }
> diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
> index 4c110940cf..0e0c61d466 100644
> --- a/src/util/vircgroupv2.c
> +++ b/src/util/vircgroupv2.c
> @@ -75,22 +75,10 @@ virCgroupV2Available(void)
>          if (STRNEQ(entry.mnt_type, "cgroup2"))
>              continue;
>  
> -        /* Systemd uses cgroup v2 for process tracking but no controller is
> -         * available. We should consider this configuration as cgroup v2 is
> -         * not available. */
> -        contFile = g_strdup_printf("%s/cgroup.controllers", entry.mnt_dir);
> -
> -        if (virFileReadAll(contFile, 1024 * 1024, &contStr) < 0)
> -            goto cleanup;
> -
> -        if (STREQ(contStr, ""))
> -            continue;
> -
>          ret = true;
>          break;
>      }
>  
> - cleanup:
>      VIR_FORCE_FCLOSE(mounts);
>      return ret;
>  }


I'm fixing the small nit in the first hunk, cleaning up the commit
message and merging. Congratulations on your first libvirt contribution!

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal