Merge ~ubuntu-release/autopkgtest/+git/development:kill-harder into ~ubuntu-release/autopkgtest/+git/development:master

[Brian Murray]

kill processes with SIGKILL if SIGTERM fails

When trying to kill long running test processes on armhf they don't always get killed so if the process is still running kill it again with SIGKILL.

Tests on armhf were continuing to run after a timeout was reached. I added some debugging to the autopkgtest code on the lxd worker and noticed that that SIGTERM call wasn't actually killing the process. This change uses SIGKILL if SIGTERM doesn't work out.

I cowboy'ed this in production and we can now see that the apport tests on armhf are now getting killed.

https://autopkgtest.ubuntu.com/results/autopkgtest-impish/impish/armhf/a/apport/20220524_020151_24815@/log.gz

dbdb476... by Brian Murray on 2022-05-25

remove redundant return

[Julian Andres Klode (juliank)]

There should be some delay between those checks, as it might take a couple of seconds to complete a SIGTERM.

Alternatively, if we deem it safe to kill without waiting, we don't need to send SIGTERM in the first place and can directly send SIGKILL.

review: Needs Fixing

a35c60c... by Brian Murray on 2022-05-25

Give time for SIGTERM to work.

[Julian Andres Klode (juliank)]

Should we check /proc/PID/cmdline before term and before kill to make sure the PID did not get reassigned?

Certainly you do not want to sleep after the check, as that increases the chance of a race.

Races might be possible if the testbed fork bombs.

[Iain Lane (laney)]

How come Popen.kill() can't be used?

Anyway, I'd suggest creating a pidfd using os.pidfd_open(), storing that, and then using signal.pidfd_send_signal (python 3.9+) to avoid races like the ones described. That avoids all PID recycling errors.

Also, do forward upstream! :-)

[Brian Murray (brian-murray)]

> How come Popen.kill() can't be used?
>
> Anyway, I'd suggest creating a pidfd using os.pidfd_open(), storing that, and
> then using signal.pidfd_send_signal (python 3.9+) to avoid races like the ones
> described. That avoids all PID recycling errors.

The autopkgtest environment is still running on Focal which has python3.8 in main and python3.9 in universe and python3.9 is not installed. So while your idea sounds best I wonder what issues we'd run into with python3.9.

> Also, do forward upstream! :-)

da06549... by Brian Murray on 2022-05-26

Implement changes based on reviewer feedback.

[Paride Legovini (paride)]

A couple of inline comments, but this basically LGTM.

review: Needs Fixing

ed50ab5... by Brian Murray on 2022-06-01

Check more frequently for SIGTERM to work and use adtlog.info()

0789950... by Brian Murray on 2022-06-01

fix math

[Paride Legovini (paride)]

LGTM (one nit).

review: Approve

396dc57... by Brian Murray on 2022-06-02

Fix a nit regarding the variable name

diff --git a/lib/adt_testbed.py b/lib/adt_testbed.py
index c0cbaad..d2991eb 100644
--- a/lib/adt_testbed.py
+++ b/lib/adt_testbed.py
@@ -1551,7 +1551,25 @@ def killtree(pid):
 
     for child in child_ps(pid):
         killtree(child)
+    with open('/proc/%s/cmdline' % pid, 'rb') as fd:
+        cmd = fd.read()
     try:
         os.kill(pid, signal.SIGTERM)
     except OSError:
         pass
+    for _ in range(21):
+        if not os.path.exists('/proc/%s' % pid):
+            return
+        else:
+            time.sleep(0.2)
+    try:
+        with open('/proc/%s/cmdline' % pid, 'rb') as fd:
+            if cmd != fd.read():
+                return
+    except FileNotFoundError:
+        return
+    try:
+        adtlog.info('kill with SIGTERM did not work sending SIGKILL')
+        os.kill(pid, signal.SIGKILL)
+    except OSError:
+        pass