[libvirt] [PATCH] virCgroupValidateMachineGroup: Reflect change in CGroup struct naming
Fron c3bd0019c0e on instead of creating the following path for cgroups: /sys/fs/cgroupX/$name.libvirt-$driver we generate rather more verbose one: /sys/fs/cgroupX/$driver-$id-$name.libvirt-$driver where $name is optional and included iff contains allowed chars. See original commit for more reasoning. Now, problem with the original commit is that we are unable to start any LXC domain after it. Because when starting LXC container, the CGroup layout is created by our lxc_controller process and then detected and validated by libvirtd. The validation is done by trying to match detected layout against all the possible patterns for cgroup paths that we've ever had. And the commit in question forgot to update this part of the code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- I am really surprised how long this bug went unnoticed. Is anybody using LXC? When pushed, it should be backported to 1.3.2+. src/util/vircgroup.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index da5ccff..add6c5f 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -261,12 +261,17 @@ virCgroupValidateMachineGroup(virCgroupPtr group, char *scopename_new = NULL; char *machinename = virSystemdMakeMachineName(drivername, id, name, privileged); + char *partmachinename = NULL; if (virAsprintf(&partname, "%s.libvirt-%s", - name, drivername) < 0) + name, drivername) < 0 || + (machinename && + virAsprintf(&partmachinename, "%s.libvirt-%s", + machinename, drivername) < 0)) goto cleanup; - if (virCgroupPartitionEscape(&partname) < 0) + if (virCgroupPartitionEscape(&partname) < 0 || + (machinename && virCgroupPartitionEscape(&partmachinename) < 0)) goto cleanup; if (!(scopename_old = virSystemdMakeScopeName(name, drivername, true))) @@ -315,6 +320,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, if (STRNEQ(tmp, name) && STRNEQ_NULLABLE(tmp, machinename) && STRNEQ(tmp, partname) && + STRNEQ_NULLABLE(tmp, partmachinename) && STRNEQ(tmp, scopename_old) && STRNEQ_NULLABLE(tmp, scopename_new)) { VIR_DEBUG("Name '%s' for controller '%s' does not match " @@ -329,6 +335,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, valid = true; cleanup: + VIR_FREE(partmachinename); VIR_FREE(partname); VIR_FREE(scopename_old); VIR_FREE(scopename_new); -- 2.8.1
On Thu, May 05, 2016 at 05:56:55PM +0200, Michal Privoznik wrote:
Fron c3bd0019c0e on instead of creating the following path for cgroups:
/sys/fs/cgroupX/$name.libvirt-$driver
we generate rather more verbose one:
/sys/fs/cgroupX/$driver-$id-$name.libvirt-$driver
where $name is optional and included iff contains allowed chars. See original commit for more reasoning. Now, problem with the original commit is that we are unable to start any LXC domain after it. Because when starting LXC container, the CGroup layout is created by our lxc_controller process and then detected and validated by libvirtd. The validation is done by trying to match detected layout against all the possible patterns for cgroup paths that we've ever had. And the commit in question forgot to update this part of the code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
I am really surprised how long this bug went unnoticed. Is anybody using LXC? When pushed, it should be backported to 1.3.2+.
src/util/vircgroup.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index da5ccff..add6c5f 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -261,12 +261,17 @@ virCgroupValidateMachineGroup(virCgroupPtr group, char *scopename_new = NULL; char *machinename = virSystemdMakeMachineName(drivername, id, name, privileged); + char *partmachinename = NULL;
if (virAsprintf(&partname, "%s.libvirt-%s", - name, drivername) < 0) + name, drivername) < 0 || + (machinename && + virAsprintf(&partmachinename, "%s.libvirt-%s", + machinename, drivername) < 0)) goto cleanup;
- if (virCgroupPartitionEscape(&partname) < 0) + if (virCgroupPartitionEscape(&partname) < 0 || + (machinename && virCgroupPartitionEscape(&partmachinename) < 0)) goto cleanup;
Instead of those composite conditions I would rather make something like this: if (machinename) { create partmachinename; run virCgroupPartitionEscape(); } It's the same but for me more readable and cleaner code. ACK with or without the change. Pavel
On 05/05/2016 11:56 AM, Michal Privoznik wrote:
Fron c3bd0019c0e on instead of creating the following path for cgroups:
/sys/fs/cgroupX/$name.libvirt-$driver
we generate rather more verbose one:
/sys/fs/cgroupX/$driver-$id-$name.libvirt-$driver
where $name is optional and included iff contains allowed chars. See original commit for more reasoning. Now, problem with the original commit is that we are unable to start any LXC domain after it. Because when starting LXC container, the CGroup layout is created by our lxc_controller process and then detected and validated by libvirtd. The validation is done by trying to match detected layout against all the possible patterns for cgroup paths that we've ever had. And the commit in question forgot to update this part of the code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
I am really surprised how long this bug went unnoticed. Is anybody using LXC? When pushed, it should be backported to 1.3.2+.
FWIW libvirt.git without this patch (and with f24 libvirt 1.3.2 package) my containers start fine, and as part of the upstream bug cleanup I did a handful of lxc testing. So I don't think the failure is quite as unconditional as you suggest - Cole
participants (3)
-
Cole Robinson -
Michal Privoznik -
Pavel Hrdina