[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Tue, Sep 30, 2014 at 10:43:47AM -0400, Cole Robinson wrote:
> 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.

Yeah, I think I'd rather we waited, and put it in a stable release once
we've had some usage in master.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]