<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Aug 21, 2018 at 1:15 PM Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Aug 14, 2018 at 11:27:33AM +0200, Christian Ehrhardt wrote:<br>
> It was found that in cases with host devices virProcessKillPainfully<br>
> might be able to send signal zero to the target PID for quite a while<br>
> with the process already being gone from /proc/<PID>.<br>
> <br>
> That is due to cleanup and reset of devices which might include a<br>
> secondary bus reset that on top of the actions taken has a 1s delay<br>
> to let the bus settle. Due to that guests with plenty of Host devices<br>
> could easily exceed the default timeouts.<br>
> <br>
> To solve that, this adds an extra delay of 2s per hostdev that is associated<br>
> to a VM.<br>
> <br>
> Signed-off-by: Christian Ehrhardt <<a href="mailto:christian.ehrhardt@canonical.com" target="_blank">christian.ehrhardt@canonical.com</a>><br>
> ---<br>
> src/libvirt_private.syms | 1 +<br>
> src/qemu/qemu_process.c | 5 +++--<br>
> src/util/virprocess.c | 18 +++++++++++++++---<br>
> src/util/virprocess.h | 1 +<br>
> 4 files changed, 20 insertions(+), 5 deletions(-)<br>
> <br>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms<br>
> index ca4a192a4a..47ea35f864 100644<br>
> --- a/src/libvirt_private.syms<br>
> +++ b/src/libvirt_private.syms<br>
> @@ -2605,6 +2605,7 @@ virProcessGetPids;<br>
> virProcessGetStartTime;<br>
> virProcessKill;<br>
> virProcessKillPainfully;<br>
> +virProcessKillPainfullyDelay;<br>
> virProcessNamespaceAvailable;<br>
> virProcessRunInMountNamespace;<br>
> virProcessSchedPolicyTypeFromString;<br>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c<br>
> index 02fdc55156..b7bf8813da 100644<br>
> --- a/src/qemu/qemu_process.c<br>
> +++ b/src/qemu/qemu_process.c<br>
> @@ -6814,8 +6814,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags)<br>
> return 0;<br>
> }<br>
> <br>
> - ret = virProcessKillPainfully(vm->pid,<br>
> - !!(flags & VIR_QEMU_PROCESS_KILL_FORCE));<br>
> + ret = virProcessKillPainfullyDelay(vm->pid,<br>
> + !!(flags & VIR_QEMU_PROCESS_KILL_FORCE),<br>
> + vm->def->nhostdevs * 2);<br>
<br>
This API contract is a bit wierd. You've got an arbitray x2 multiplier<br>
here...<br></blockquote><div> </div><div>Out of the discussion with Alex I derived that due to the 1 sec settling of the bus plus some extra work 2 seconds per device would be a good rule of thumb.</div><div>The delay is meant to be in seconds and here it requests 2*nhostdevs to get the amount needed.</div><div><br></div><div>And I'd not want to make the "unit" of the call "double-seconds" :-)</div><div><br></div><div>Let me add a comment at this call to explain the reasoning on the multiplier here.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c<br>
> index ecea27a2d4..46360cc051 100644<br>
> --- a/src/util/virprocess.c<br>
> +++ b/src/util/virprocess.c<br>
> @@ -341,15 +341,19 @@ int virProcessKill(pid_t pid, int sig)<br>
> * Returns 0 if it was killed gracefully, 1 if it<br>
> * was killed forcibly, -1 if it is still alive,<br>
> * or another error occurred.<br>
> + *<br>
> + * Callers can proide an extra delay to wait longer<br>
> + * than the default.<br>
<br>
Mention that it is in "seconds"<br></blockquote><div> </div><div>Ack</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> */<br>
> int<br>
> -virProcessKillPainfully(pid_t pid, bool force)<br>
> +virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int extradelay)<br>
> {<br>
> size_t i;<br>
> int ret = -1;<br>
> + unsigned int delay = 75 + (extradelay*5);<br>
<br>
...and it gets another x5 multiplier here.<br></blockquote><div><br></div><div>That *5 is due to the internal unit it polls being 0.2 seconds.</div><div>I think externally anything other than seconds (double-seconds, half-deca-seconds, ...) makes no sense.</div><div><br></div><div>I'll call it "polldelay" instead to make it more clear that.</div><div>That uncommon *5 ratio was there before, just not as redable (it just defined 75 in the loop header).</div><div>I'll add some text about it as well to be sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Feels nicer to just have the caller provide a x10 multiplier and<br>
honour that directly<br></blockquote><div><br></div><div>See above, the unit being seconds seems the most readable IMHO.</div><div>With the changes made I think it won't be as confusing.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> const char *signame = "TERM";<br>
> <br>
> - VIR_DEBUG("vpid=%lld force=%d", (long long)pid, force);<br>
> + VIR_DEBUG("vpid=%lld force=%d delay=%u", (long long)pid, force, pid);<br>
<br>
s/pid/extradelay/<br></blockquote><div> </div><div>Yes, that was mentioned by Bjoern and queued that way.</div><div>Also in the text the delay= should be extradelay= which is ready that way.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <br>
> /* This loop sends SIGTERM, then waits a few iterations (10 seconds)<br>
> * to see if it dies. If the process still hasn't exited, and<br>
> @@ -357,9 +361,12 @@ virProcessKillPainfully(pid_t pid, bool force)<br>
> * wait up to 5 seconds more for the process to exit before<br>
> * returning.<br>
> *<br>
> + * An extra delay can be specified for cases that are expected to clean<br>
> + * up slower than usual.<br>
> + *<br>
> * Note that setting @force could result in dataloss for the process.<br>
> */<br>
> - for (i = 0; i < 75; i++) {<br>
> + for (i = 0; i < delay; i++) {<br>
> int signum;<br>
> if (i == 0) {<br>
> signum = SIGTERM; /* kindly suggest it should exit */<br>
> @@ -402,6 +409,11 @@ virProcessKillPainfully(pid_t pid, bool force)<br>
> }<br>
> <br>
> <br>
> +int virProcessKillPainfully(pid_t pid, bool force)<br>
> +{<br>
> + return virProcessKillPainfullyDelay(pid, force, 0);<br>
> +}<br>
> +<br>
> #if HAVE_SCHED_GETAFFINITY<br>
> <br>
> int virProcessSetAffinity(pid_t pid, virBitmapPtr map)<br>
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h<br>
> index 3c5a882772..b72603ca8e 100644<br>
> --- a/src/util/virprocess.h<br>
> +++ b/src/util/virprocess.h<br>
> @@ -55,6 +55,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)<br>
> int virProcessKill(pid_t pid, int sig);<br>
> <br>
> int virProcessKillPainfully(pid_t pid, bool force);<br>
> +int virProcessKillPainfullyDelay(pid_t pid, bool force, unsigned int);<br>
<br>
Missing parameter name here<br></blockquote><div> </div><div>Ack, and then breaking it into new lines for 80 chars</div><div><br></div><div>Thanks all these will be part of a v4 ready after some syntax check and test builds ...</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Regards,<br>
Daniel<br>
-- <br>
|: <a href="https://berrange.com" rel="noreferrer" target="_blank">https://berrange.com</a> -o- <a href="https://www.flickr.com/photos/dberrange" rel="noreferrer" target="_blank">https://www.flickr.com/photos/dberrange</a> :|<br>
|: <a href="https://libvirt.org" rel="noreferrer" target="_blank">https://libvirt.org</a> -o- <a href="https://fstop138.berrange.com" rel="noreferrer" target="_blank">https://fstop138.berrange.com</a> :|<br>
|: <a href="https://entangle-photo.org" rel="noreferrer" target="_blank">https://entangle-photo.org</a> -o- <a href="https://www.instagram.com/dberrange" rel="noreferrer" target="_blank">https://www.instagram.com/dberrange</a> :|<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_1876539459936316048gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><span style="color:rgb(136,136,136);font-size:12.8px">Christian Ehrhardt</span><div style="color:rgb(136,136,136);font-size:12.8px">Software Engineer, Ubuntu Server</div><div style="color:rgb(136,136,136);font-size:12.8px">Canonical Ltd</div></div></div></div></div></div>