On Tue, Aug 14, 2018 at 11:27:33AM +0200, Christian Ehrhardt wrote:
> It was found that in cases with host devices virProcessKillPainfully
> might be able to send signal zero to the target PID for quite a while
> with the process already being gone from /proc/<PID>.
>
> That is due to cleanup and reset of devices which might include a
> secondary bus reset that on top of the actions taken has a 1s delay
> to let the bus settle. Due to that guests with plenty of Host devices
> could easily exceed the default timeouts.
>
> To solve that, this adds an extra delay of 2s per hostdev that is associated
> to a VM.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_process.c | 5 +++--
> src/util/virprocess.c | 18 +++++++++++++++---
> src/util/virprocess.h | 1 +
> 4 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ca4a192a4a..47ea35f864 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2605,6 +2605,7 @@ virProcessGetPids;
> virProcessGetStartTime;
> virProcessKill;
> virProcessKillPainfully;
> +virProcessKillPainfullyDelay;
> virProcessNamespaceAvailable;
> virProcessRunInMountNamespace;
> virProcessSchedPolicyTypeFromString;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 02fdc55156..b7bf8813da 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags)
> return 0;
> }
>
> - ret = virProcessKillPainfully(vm->pid,
> - !!(flags & VIR_QEMU_PROCESS_KILL_FORCE));
> + ret = virProcessKillPainfullyDelay(vm->pid,
> + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE),
> + vm->def->nhostdevs * 2);
This API contract is a bit wierd. You've got an arbitray x2 multiplier
here...
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index ecea27a2d4..46360cc051 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)
> * Returns 0 if it was killed gracefully, 1 if it
> * was killed forcibly, -1 if it is still alive,
> * or another error occurred.
> + *
> + * Callers can proide an extra delay to wait longer
> + * than the default.
Mention that it is in "seconds"
> */
> int
> -virProcessKillPainfully(pid_t pid, bool force)
> +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay)
> {
> size_t i;
> int ret = -1;
> + unsigned int delay = 75 + (extradelay*5);
...and it gets another x5 multiplier here.
Feels nicer to just have the caller provide a x10 multiplier and
honour that directly
> const char *signame = "TERM";
>
> - VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);
> + VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid);
s/pid/extradelay/
>
> /* This loop sends SIGTERM, then waits a few iterations (10 seconds)
> * to see if it dies. If the process still hasn't exited, and
> @@ -357,9 +361,12 @@ virProcessKillPainfully(pid_t pid, bool force)
> * wait up to 5 seconds more for the process to exit before
> * returning.
> *
> + * An extra delay can be specified for cases that are expected to clean
> + * up slower than usual.
> + *
> * Note that setting @force could result in dataloss for the process.
> */
> - for (i = 0; i < 75; i++) {
> + for (i = 0; i < delay; i++) {
> int signum;
> if (i == 0) {
> signum = SIGTERM; /* kindly suggest it should exit */
> @@ -402,6 +409,11 @@ virProcessKillPainfully(pid_t pid, bool force)
> }
>
>
> +int virProcessKillPainfully(pid_t pid, bool force)
> +{
> + return virProcessKillPainfullyDelay(pid, force, 0);
> +}
> +
> #if HAVE_SCHED_GETAFFINITY
>
> int virProcessSetAffinity(pid_t pid, virBitmapPtr map)
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index 3c5a882772..b72603ca8e 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -55,6 +55,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
> int virProcessKill(pid_t pid, int sig);
>
> int virProcessKillPainfully(pid_t pid, bool force);
> +int virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int);
Missing parameter name here
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
