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

[Gavin Panella]

Backport r5866 from trunk: Remove ProcessGroupLeaderMixin: it is subject to a race with reactor.spawnProcess.

Instead, subprocesses must set themselves as process group leaders.

[Gavin Panella (allenap)]

Backport, hence selfie.

review: Approve

=== 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-29 13:03:13 +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-29 13:03:13 +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-29 13:03:13 +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-29 13:03:13 +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-29 13:03:13 +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-29 13:03:13 +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-29 13:03:13 +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.