[libvirt] [PATCH 4/4] Remount cgroups controllers after setting up new /sys in LXC

[Stefan Berger]

On 05/11/2012 12:48 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> Normal practice is for cgroups controllers to be mounted at
> /sys/fs/cgroup. When setting up a container, /sys is mounted
> with a new sysfs instance, thus we must re-mount all the
> cgroups controllers. The complexity is that we must mount
> them in the same layout as the host OS. ie if 'cpu' and 'cpuacct'
> were mounted at the same location in the host we must preserve
> this in the container. Also if any controllers are co-located
> we must setup symlinks from the individual controller name to
> the co-located mount-point

>
> Signed-off-by: Daniel P. Berrange<berrange at redhat.com>
> ---
>   src/lxc/lxc_container.c |  243 +++++++++++++++++++++++++++++++++++++++++++++--
>   src/util/cgroup.h       |    2 +
>   2 files changed, 236 insertions(+), 9 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index a3ca76c..79ecae8 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -35,6 +35,7 @@
>   #include<sys/stat.h>
>   #include<unistd.h>
>   #include<mntent.h>
> +#include<dirent.h>
>
>   /* Yes, we want linux private one, for _syscall2() macro */
>   #include<linux/unistd.h>
>
> +static int lxcContainerIdentifyCGroups(struct lxcContainerCGroup **mountsret,
> +                                       size_t *nmountsret)
> +{
[...]
> +    while ((dent = readdir(dh)) != NULL) {
> +        ssize_t rv;
> +        /* The cgroups links are just relative to the local
> +         * dir so we don't need a large buf */
> +        char linkbuf[100];

Nit: empty line here.

> +        if (dent->d_name[0] == '.')
> +            continue;
> +
[...]
> +cleanup:
> +    closedir(dh);
> +    endmntent(procmnt);
> +    VIR_FREE(path);
> +
> +    if (ret<  0) {
> +        for (i = 0 ; i<  nmounts ; i++) {
> +            VIR_FREE(mounts[i].dir);
> +            VIR_FREE(mounts[i].linkDest);
> +        }
> +        VIR_FREE(mounts);

You could put this into a function lxcContainerCGroupFree().

>+
>+cleanup:
>+    for (i = 0 ; i<  nmounts ; i++) {
>+        VIR_FREE(mounts[i].dir);
>+        VIR_FREE(mounts[i].linkDest);
>+    }
>+    VIR_FREE(mounts);


... and call it again from here and save a few lines ...


> @@ -1190,19 +1394,40 @@ static int lxcContainerSetupExtraMounts(virDomainDefPtr vmDef,
>       if (lxcContainerMountAllFS(vmDef, "", false)<  0)
>           return -1;
>
> +    /* Before replacing /sys we need to identify any
> +     * cgroups controllers that are mounted */
> +    if (lxcContainerIdentifyCGroups(&mounts,&nmounts)<  0)
> +        goto cleanup;
> +
>       /* Gets rid of any existing stuff under /proc, since we need new
>        * namespace aware versions of those. We must do /proc second
>        * otherwise we won't find /proc/mounts :-) */
>       if (lxcContainerUnmountSubtree("/sys", false)<  0 ||
>           lxcContainerUnmountSubtree("/proc", false)<  0)
> -        return -1;
> +        goto cleanup;
>
>       /* Mounts the core /proc, /sys, etc filesystems */
>       if (lxcContainerMountBasicFS(vmDef, false, securityDriver)<  0)
> -        return -1;
> +        goto cleanup;
> +
> +    /* Now we can re-mount the cgroups controllers in the
> +     * same configuration as before */
> +    if (lxcContainerMountCGroups(mounts, nmounts)<  0)
> +        goto cleanup;
>
>       VIR_DEBUG("Mounting completed");
>       return 0;
> +
> +    ret = 0;
> +
> +cleanup:
> +    for (i = 0 ; i<  nmounts ; i++) {
> +        VIR_FREE(mounts[i].dir);
> +        VIR_FREE(mounts[i].linkDest);
> +    }
> +    VIR_FREE(mounts);

... and save a few more lines here as well.

  With this nit, I'd ACK the series.

    Stefan