Re: [libvirt] [PATCH] qemu: don't error out when cgroups don't exist

On Wed, Jul 09, 2014 at 01:07:47PM +0200, Ján Tomko wrote: > On 07/09/2014 10:15 AM, Martin Kletzander wrote: >> When creating cgroups for vcpu and emulator threads whilst starting a >> domain, we explicitly skip creating those cgroups in case priv->cgroup >> is NULL (cgroups not supported) because SetAffinity() serves the same >> purpose. If the host supports only some cgroups (the ones we need are >> either unmounted or disabled in qemu.conf), we error out with weird >> message even though we could continue starting the domain. > > Yet this patch does not change the error message. > Yes, because there should be no error. >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1097028 >> >> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >> --- >> src/qemu/qemu_cgroup.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c >> index 3394c68..0af6ac5 100644 >> --- a/src/qemu/qemu_cgroup.c >> +++ b/src/qemu/qemu_cgroup.c >> @@ -949,7 +949,11 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) >> virCgroupFree(&cgroup_vcpu); >> } >> >> - return -1; >> + if (period || quota) >> + return -1; >> + >> + virResetLastError(); >> + return 0; >> } >> > > This also resets OOM errors and errors that happen when these controllers are > mounted. > > Can't we just check upfront if the needed controllers are available? > There might be more problems than the controllers not being mounted, but OK, the others are corner cases anyway. Would the following diff be more suitable?

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..d4c969b 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -893,6 +893,15 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm) return -1; } + /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + /* We are trying to setup cgroups for CPU pinning, which can also be done * with virProcessSetAffinity, thus the lack of cgroups is not fatal here. */ @@ -972,6 +981,15 @@ qemuSetupCgroupForEmulator(virQEMUDriverPtr driver, return -1; } + /* + * If CPU cgroup controller is not initialized here, than we need + * neither period nor quota settings. And if CPUSET controller is + * not initialized either, than there's nothing to do anyway. + */ + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) && + !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) + return 0; + if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */