Merge lp:~allenap/maas/process-group-leader-race into lp:~maas-committers/maas/trunk

[Gavin Panella]

Remove ProcessGroupLeaderMixin: it is subject to a race with reactor.spawnProcess.

Instead, subprocesses must set themselves as process group leaders.

[Mike Pontillo (mpontillo)]

Looks good; thanks for the fix.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/process-group-leader-race into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [102 kB]
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Fetched 306 kB in 0s (617 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql psmisc pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
psmisc is already the newest version (22.21-2.1build1).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
python-n...

=== modified file 'src/provisioningserver/utils/arp.py'
--- src/provisioningserver/utils/arp.py	2016-09-27 11:56:15 +0000
+++ src/provisioningserver/utils/arp.py	2017-03-24 15:31:21 +0000
@@ -370,6 +370,11 @@
 def run(args, output=sys.stdout, stdin=sys.stdin,
         stdin_buffer=sys.stdin.buffer):
     """Observe an Ethernet interface and print ARP bindings."""
+
+    # First, become a progress group leader, so that signals can be directed
+    # to this process and its children; see p.u.twisted.terminateProcess.
+    os.setpgrp()
+
     network_monitor = None
     if args.input_file is None:
         if args.interface is None:

=== modified file 'src/provisioningserver/utils/avahi.py'
--- src/provisioningserver/utils/avahi.py	2016-09-21 15:25:11 +0000
+++ src/provisioningserver/utils/avahi.py	2017-03-24 15:31:21 +0000
@@ -38,6 +38,7 @@
 from contextlib import contextmanager
 import io
 import json
+import os
 import re
 import subprocess
 import sys
@@ -203,6 +204,11 @@
 
 def run(args, output=sys.stdout, stdin=sys.stdin):
     """Observe an Ethernet interface and print ARP bindings."""
+
+    # First, become a progress group leader, so that signals can be directed
+    # to this process and its children; see p.u.twisted.terminateProcess.
+    os.setpgrp()
+
     if args.input_file is None:
         reader = _reader_from_avahi()
     elif args.input_file == "-":

=== modified file 'src/provisioningserver/utils/services.py'
--- src/provisioningserver/utils/services.py	2016-10-28 15:58:32 +0000
+++ src/provisioningserver/utils/services.py	2017-03-24 15:31:21 +0000
@@ -32,7 +32,6 @@
 from provisioningserver.utils.twisted import (
     callOut,
     deferred,
-    ProcessGroupLeaderMixin,
     terminateProcess,
 )
 from twisted.application.internet import TimerService
@@ -124,8 +123,7 @@
             self.done.errback(reason)
 
 
-class ProtocolForObserveARP(
-        ProcessGroupLeaderMixin, JSONPerLineProtocol):
+class ProtocolForObserveARP(JSONPerLineProtocol):
     """Protocol used when spawning `maas-rack observe-arp`.
 
     The difference between `JSONPerLineProtocol` and `ProtocolForObserveARP`
@@ -149,8 +147,7 @@
         log.msg("observe-arp[%s]:" % self.interface, line)
 
 
-class ProtocolForObserveMDNS(
-        ProcessGroupLeaderMixin, JSONPerLineProtocol):
+class ProtocolForObserveMDNS(JSONPerLineProtocol):
     """Protocol used when spawning `maas-rack observe-mdns`.
 
     This ensures that the spawned process is configured as a process group

=== modified file 'src/provisioningserver/utils/tests/test_arp.py'
--- src/provisioningserver/utils/tests/test_arp.py	2016-08-25 16:10:40 +0000
+++ src/provisioningserver/utils/tests/test_arp.py	2017-03-24 15:31:21 +0000
@@ -14,6 +14,8 @@
 import time
 from unittest.mock import Mock
 
+from maastesting.factory import factory
+from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from netaddr import (
     EUI,
@@ -509,3 +511,10 @@
         with ExpectedException(
                 SystemExit, '.*42.*'):
             run(args, output=output)
+
+    def test__sets_self_as_process_group_leader(self):
+        exception_type = factory.make_exception_type()
+        os = self.patch(arp_module, "os")
+        os.setpgrp.side_effect = exception_type
+        self.assertRaises(exception_type, run, [])
+        self.assertThat(os.setpgrp, MockCalledOnceWith())

=== modified file 'src/provisioningserver/utils/tests/test_avahi.py'
--- src/provisioningserver/utils/tests/test_avahi.py	2016-09-13 17:15:27 +0000
+++ src/provisioningserver/utils/tests/test_avahi.py	2017-03-24 15:31:21 +0000
@@ -12,6 +12,8 @@
 from tempfile import NamedTemporaryFile
 import time
 
+from maastesting.factory import factory
+from maastesting.matchers import MockCalledOnceWith
 from maastesting.testcase import MAASTestCase
 from provisioningserver.utils import avahi as avahi_module
 from provisioningserver.utils.avahi import (
@@ -279,3 +281,10 @@
         output = io.StringIO()
         with ExpectedException(SystemExit, '.*42.*'):
             run(args, output=output)
+
+    def test__sets_self_as_process_group_leader(self):
+        exception_type = factory.make_exception_type()
+        os = self.patch(avahi_module, "os")
+        os.setpgrp.side_effect = exception_type
+        self.assertRaises(exception_type, run, [])
+        self.assertThat(os.setpgrp, MockCalledOnceWith())

=== modified file 'src/provisioningserver/utils/tests/test_twisted.py'
--- src/provisioningserver/utils/tests/test_twisted.py	2016-10-24 14:36:30 +0000
+++ src/provisioningserver/utils/tests/test_twisted.py	2017-03-24 15:31:21 +0000
@@ -14,7 +14,6 @@
 from random import (
     randint,
     random,
-    randrange,
 )
 import re
 import signal
@@ -64,7 +63,6 @@
     LONGTIME,
     makeDeferredWithProcessProtocol,
     pause,
-    ProcessGroupLeaderMixin,
     retries,
     RPCFetcher,
     synchronous,
@@ -1723,6 +1721,30 @@
 
 # A script that prints the signals it receives, as long as they're SIGTERM or
 # SIGQUIT. It prints "Ready." on stderr once it's registered signal handlers.
+
+# This sets itself as process group leader.
+signal_printer_pgl = """\
+from os import setpgrp
+from signal import Signals, signal
+from sys import stderr
+from time import sleep
+from traceback import print_exc
+
+def print_signal(signum, frame):
+    print(Signals(signum).name, flush=True)
+
+signal(Signals.SIGTERM, print_signal)
+signal(Signals.SIGQUIT, print_signal)
+
+setpgrp()
+
+print("Ready.", file=stderr, flush=True)
+
+sleep(5.0)
+"""
+
+# This variant should be identical, except that it does
+# not set itself as process group leader.
 signal_printer = """\
 from signal import Signals, signal
 from sys import stderr
@@ -1763,14 +1785,6 @@
         self.done.callback(reason)
 
 
-class SignalPrinterProtocolAsGroupLeader(
-        ProcessGroupLeaderMixin, SignalPrinterProtocol):
-    """Process protocol for use with `signal_printer` that ...
-
-    ...configures itself as a process group leader.
-    """
-
-
 class TestTerminateProcess(MAASTestCase):
     """Tests for `terminateProcess`."""
 
@@ -1778,7 +1792,6 @@
 
     def setUp(self):
         super(TestTerminateProcess, self).setUp()
-        self.sigprint = self.make_file("sigprint.py", signal_printer)
         # Allow spying on calls to os.kill and os.killpg by terminateProcess.
         self.assertThat(twisted_module._os_kill, Is(os.kill))
         self.patch(
@@ -1789,13 +1802,16 @@
             twisted_module, "_os_killpg",
             Mock(wraps=twisted_module._os_killpg))
 
-    def startSignalPrinter(self, protocol):
+    def startSignalPrinter(self, protocol, *, pgrp=False):
+        script_filename = self.make_file(
+            "sigprint.py", signal_printer_pgl if pgrp else signal_printer)
+
         self.assertThat(protocol, IsInstance(SignalPrinterProtocol))
         self.addDetail("out", content_from_stream(protocol.out, seek_offset=0))
         self.addDetail("err", content_from_stream(protocol.err, seek_offset=0))
         python = sys.executable.encode("utf-8")
         process = reactor.spawnProcess(
-            protocol, python, (python, self.sigprint.encode("utf-8")))
+            protocol, python, (python, script_filename.encode("utf-8")))
 
         # Wait for the spawned subprocess to tell us that it's ready.
         def cbReady(message):
@@ -1829,8 +1845,8 @@
 
     @inlineCallbacks
     def test__terminates_with_kill_and_killpg(self):
-        protocol = SignalPrinterProtocolAsGroupLeader()
-        process = yield self.startSignalPrinter(protocol)
+        protocol = SignalPrinterProtocol()
+        process = yield self.startSignalPrinter(protocol, pgrp=True)
         # Capture the pid now; it gets cleared when the process exits.
         pid = process.pid
         # Terminate and wait for it to exit.
@@ -1851,7 +1867,7 @@
     @inlineCallbacks
     def test__terminates_with_kill_if_not_in_separate_process_group(self):
         protocol = SignalPrinterProtocol()
-        process = yield self.startSignalPrinter(protocol)
+        process = yield self.startSignalPrinter(protocol, pgrp=False)
         # Capture the pid now; it gets cleared when the process exits.
         pid = process.pid
         # Terminate and wait for it to exit.
@@ -1867,15 +1883,3 @@
             ))
         self.assertThat(
             twisted_module._os_killpg, MockNotCalled())
-
-
-class TestProcessGroupLeaderMixin(MAASTestCase):
-    """Tests for `ProcessGroupLeaderMixin`."""
-
-    def test__calls_setpgid_on_child_process(self):
-        self.assertThat(twisted_module._os_setpgid, Is(os.setpgid))
-        setpgid = self.patch(twisted_module, "_os_setpgid")
-        protocol = ProcessGroupLeaderMixin()
-        transport = Mock(pid=randrange(99999, 9999999))
-        protocol.makeConnection(transport)
-        self.assertThat(setpgid, MockCalledOnceWith(transport.pid, 0))

=== modified file 'src/provisioningserver/utils/twisted.py'
--- src/provisioningserver/utils/twisted.py	2016-10-28 15:58:32 +0000
+++ src/provisioningserver/utils/twisted.py	2017-03-24 15:31:21 +0000
@@ -20,7 +20,6 @@
     'LONGTIME',
     'makeDeferredWithProcessProtocol',
     'pause',
-    'ProcessGroupLeaderMixin',
     'retries',
     'synchronous',
     'ThreadPool',
@@ -46,7 +45,6 @@
 from os import (
     kill as _os_kill,
     killpg as _os_killpg,
-    setpgid as _os_setpgid,
 )
 import signal
 import threading
@@ -1003,9 +1001,8 @@
     Steps #3 and #5 have a safeguard: if the process identified by `pid` has
     the same process group as the invoking process the signal is sent only to
     the process and not to the process group. This prevents the caller from
-    inadvertently killing itself. You may want to use this in conjunction with
-    `ProcessGroupLeaderMixin` to ensure that the child process becomes a
-    process group leader soon after spawning.
+    inadvertently killing itself. For best effect, ensure that new processes
+    become process group leaders soon after spawning.
 
     :param pid: The PID to terminate.
     :param done: A `Deferred` that fires when the process exits.
@@ -1046,15 +1043,3 @@
                 killer.cancel()
 
     done.addBoth(callOut, ended)
-
-
-class ProcessGroupLeaderMixin(ProcessProtocol):
-    """Mix-in to ensure that the spawned process is a process group leader."""
-
-    def connectionMade(self):
-        super().connectionMade()
-        if self.transport.pid is not None:
-            try:
-                _os_setpgid(self.transport.pid, 0)
-            except ProcessLookupError:
-                pass  # Already exited.