[libvirt] [PATCH] qemu: use systemd's TerminateMachine to kill all processes

[Cole Robinson]

On 09/30/2014 10:25 AM, Guido Günther wrote:
> On Tue, Sep 30, 2014 at 03:22:41PM +0100, Daniel P. Berrange wrote:
>> On Tue, Sep 30, 2014 at 10:10:51AM -0400, Cole Robinson wrote:
>>> 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
>>
>> I'm surprised we see the problem with QEMU, but this matches what we
>> do for LXC and is recommended by systemd maintainers so fine to for
>> QEMU too.
> 
> Thanks to the two of you for reviewing. Should this go into 1.2.9 ?

I don't know why this patch would cause problems... but then again it's
cgroup/systemd stuff which makes me a little nervous. If it was my patch I'd
wait until after the release to be safe.

- Cole