On 09/25/2014 08:30 AM, Guido Günther wrote:
> If we don't properly clean up all processes in the
> machine-<vmname>.scope systemd won't remove the cgroup and subsequent vm
> starts fail with
>
> 'CreateMachine: File exists'
>
> Additional processes can e.g. be added via
>
> echo $PID > /sys/fs/cgroup/systemd/machine.slice/machine-${VMNAME}.scope/tasks
>
> but there are other cases like
>
> http://bugs.debian.org/761521
>
> Invoke TerminateMachine to be on the safe side since systemd tracks the
> cgroup anyway. This is a noop if all processes have terminated already.
Thanks for the patch, I've definitely seen this a handful of times on Fedora
as well.
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_cgroup.c | 11 ++++++++++-
> src/qemu/qemu_cgroup.h | 2 +-
> src/qemu/qemu_process.c | 4 ++--
> src/util/vircgroup.c | 11 +++++++++++
> src/util/vircgroup.h | 5 +++++
> 6 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 51a692b..99ef1db 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1115,6 +1115,7 @@ virCgroupSetMemorySoftLimit;
> virCgroupSetMemSwapHardLimit;
> virCgroupSetOwner;
> virCgroupSupportsCpuBW;
> +virCgroupTerminateMachine;
>
>
> # util/virclosecallbacks.h
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 7c6b2c1..0ab7227 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -1188,13 +1188,22 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
> }
>
> int
> -qemuRemoveCgroup(virDomainObjPtr vm)
> +qemuRemoveCgroup(virQEMUDriverPtr driver,
> + virDomainObjPtr vm)
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
> if (priv->cgroup == NULL)
> return 0; /* Not supported, so claim success */
>
> + if (virCgroupTerminateMachine(vm->def->name,
> + "qemu",
> + cfg->privileged) < 0) {
> + if (!virCgroupNewIgnoreError())
> + VIR_DEBUG("Failed to terminate cgroup for %s", vm->def->name);
> + }
> +
> return virCgroupRemove(priv->cgroup);
> }
>
> diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
> index 8a2c723..4a4f22c 100644
> --- a/src/qemu/qemu_cgroup.h
> +++ b/src/qemu/qemu_cgroup.h
> @@ -66,7 +66,7 @@ int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
> int qemuSetupCgroupForEmulator(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virBitmapPtr nodemask);
> -int qemuRemoveCgroup(virDomainObjPtr vm);
> +int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
> int qemuAddToCgroup(virDomainObjPtr vm);
>
> #endif /* __QEMU_CGROUP_H__ */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 13614e9..e7cce1a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4131,7 +4131,7 @@ int qemuProcessStart(virConnectPtr conn,
> /* Ensure no historical cgroup for this VM is lying around bogus
> * settings */
> VIR_DEBUG("Ensuring no historical cgroup is lying around");
> - qemuRemoveCgroup(vm);
> + qemuRemoveCgroup(driver, vm);
>
> for (i = 0; i < vm->def->ngraphics; ++i) {
> virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
> @@ -4909,7 +4909,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> }
>
> retry:
> - if ((ret = qemuRemoveCgroup(vm)) < 0) {
> + if ((ret = qemuRemoveCgroup(driver, vm)) < 0) {
> if (ret == -EBUSY && (retries++ < 5)) {
> usleep(200*1000);
> goto retry;
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index 1dbe6f9..d69f71b 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1680,6 +1680,17 @@ virCgroupNewMachineSystemd(const char *name,
> }
>
>
> +/*
> + * Returns 0 on success, -1 on fatal error
> + */
> +int virCgroupTerminateMachine(const char *name,
> + const char *drivername,
> + bool privileged)
> +{
> + return virSystemdTerminateMachine(name, drivername, privileged);
> +}
> +
> +
> static int
> virCgroupNewMachineManual(const char *name,
> const char *drivername,
> diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
> index 19e82d1..7718a07 100644
> --- a/src/util/vircgroup.h
> +++ b/src/util/vircgroup.h
> @@ -106,6 +106,11 @@ int virCgroupNewMachine(const char *name,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> ATTRIBUTE_NONNULL(4);
>
> +int virCgroupTerminateMachine(const char *name,
> + const char *drivername,
> + bool privileged)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> bool virCgroupNewIgnoreError(void);
>
> void virCgroupFree(virCgroupPtr *group);
>
All the above seems reasonable to me. ACK
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list