Merge ~blake-rouse/maas:bind9-restart into maas:master

[Blake Rouse]

Fixes LP: #1710278 - Notice when bind9 is hung, force kill the service, ensure it starts again, then perform a reload.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bind9-restart lp:~blake-rouse/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e9e8b973d4fb446e9b067daf812ba8b4f9b216b7

review: Approve

[Lee Trager (ltrager)]

LGTM!

review: Approve

diff --git a/debian/extras/99-maas-common-sudoers b/debian/extras/99-maas-common-sudoers
index 47387d2..4c2bfd3 100644
--- a/debian/extras/99-maas-common-sudoers
+++ b/debian/extras/99-maas-common-sudoers
@@ -18,10 +18,11 @@ maas ALL= NOPASSWD: /bin/systemctl restart chrony
 maas ALL= NOPASSWD: /bin/systemctl start chrony
 maas ALL= NOPASSWD: /bin/systemctl status chrony
 
-# Control of the DNS server: MAAS needs to track status and start it
-# again if stopped manually.
+# Control of the DNS server: MAAS needs to track status, start it
+# again if stopped manually, or if MAAS must brutally kill it!
 maas ALL= NOPASSWD: /bin/systemctl status bind9
 maas ALL= NOPASSWD: /bin/systemctl start bind9
+maas ALL= NOPASSWD: /bin/systemctl kill -s SIGKILL bind9
 
 # Control of the Proxy server: MAAS needs to reconfigure it after editing
 # its configuration file, start it and stop it.
diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index 702b194..36aaacf 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -54,7 +54,7 @@ def dns_force_reload():
     DNSPublication(source="Force reload").save()
 
 
-def dns_update_all_zones(reload_retry=False):
+def dns_update_all_zones(reload_retry=False, reload_timeout=2):
     """Update all zone files for all domains.
 
     Serving these zone files means updating BIND's configuration to include
@@ -96,12 +96,12 @@ def dns_update_all_zones(reload_retry=False):
     # actually needed but it seems safer to maintain this behaviour until we
     # have a better understanding.
     if reload_retry:
-        bind_reload_with_retries()
+        reloaded = bind_reload_with_retries(timeout=reload_timeout)
     else:
-        bind_reload()
+        reloaded = bind_reload(timeout=reload_timeout)
 
     # Return the current serial and list of domain names.
-    return serial, [
+    return serial, reloaded, [
         domain.name
         for domain in domains
     ]
diff --git a/src/maasserver/dns/tests/test_config.py b/src/maasserver/dns/tests/test_config.py
index 6bb5bfb..6d0bdfb 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -62,6 +62,7 @@ from testtools.matchers import (
     Equals,
     FileContains,
     HasLength,
+    Is,
     MatchesSetwise,
     MatchesStructure,
 )
@@ -307,7 +308,8 @@ class TestDNSConfigModifications(TestDNSServer):
         bind_reload_with_retries = self.patch_autospec(
             dns_config_module, "bind_reload_with_retries")
         dns_update_all_zones(reload_retry=True)
-        self.assertThat(bind_reload_with_retries, MockCalledOnceWith())
+        self.assertThat(
+            bind_reload_with_retries, MockCalledOnceWith(timeout=2))
 
     def test_dns_update_all_zones_passes_upstream_dns_parameter(self):
         self.patch(settings, 'DNS_CONNECT', True)
@@ -377,8 +379,9 @@ class TestDNSConfigModifications(TestDNSServer):
         self.patch(
             dns_config_module,
             "current_zone_serial").return_value = fake_serial
-        serial, domains = dns_update_all_zones()
+        serial, reloaded, domains = dns_update_all_zones()
         self.assertThat(serial, Equals(fake_serial))
+        self.assertThat(reloaded, Is(True))
         self.assertThat(domains, MatchesSetwise(*[
             Equals(domain.name)
             for domain in Domain.objects.filter(authoritative=True)
diff --git a/src/maasserver/region_controller.py b/src/maasserver/region_controller.py
index 9a41c72..2989b6e 100644
--- a/src/maasserver/region_controller.py
+++ b/src/maasserver/region_controller.py
@@ -48,6 +48,7 @@ from maasserver.rbac import (
     Resource,
     SyncConflictError,
 )
+from maasserver.service_monitor import service_monitor
 from maasserver.utils import synchronised
 from maasserver.utils.orm import (
     transactional,
@@ -191,7 +192,11 @@ class RegionControllerService(Service):
             d = deferToDatabase(transactional(dns_update_all_zones))
             d.addCallback(self._checkSerial)
             d.addCallback(self._logDNSReload)
+            # Order here matters, first needsDNSUpdate is set then pass the
+            # failure onto `_onDNSReloadFailure` to do the correct thing
+            # with the DNS server.
             d.addErrback(_onFailureRetry, 'needsDNSUpdate')
+            d.addErrback(self._onDNSReloadFailure)
             d.addErrback(
                 log.err,
                 "Failed configuring DNS.")
@@ -229,7 +234,10 @@ class RegionControllerService(Service):
         """Check that the serial of the domain is updated."""
         if result is None:
             return None
-        serial, domain_names = result
+        serial, reloaded, domain_names = result
+        if not reloaded:
+            raise DNSReloadError(
+                "Failed to reload DNS; timeout or rdnc command failed.")
         not_matching_domains = set(domain_names)
         loop = 0
         while len(not_matching_domains) > 0 and loop != 30:
@@ -252,13 +260,13 @@ class RegionControllerService(Service):
             raise DNSReloadError(
                 "Failed to reload DNS; serial mismatch "
                 "on domains %s" % ', '.join(not_matching_domains))
-        return serial, domain_names
+        return result
 
     def _logDNSReload(self, result):
         """Log the reason DNS was reloaded."""
         if result is None:
             return None
-        serial, domain_names = result
+        serial, _, domain_names = result
         if self.previousSerial is None:
             # This was the first load for starting the service.
             self.previousSerial = serial
@@ -269,7 +277,10 @@ class RegionControllerService(Service):
             # reason for the reload.
 
             def _logReason(reasons):
-                if len(reasons) == 1:
+                if len(reasons) == 0:
+                    msg = (
+                        "Reloaded DNS configuration; previous failure (retry)")
+                elif len(reasons) == 1:
                     msg = "Reloaded DNS configuration; %s" % reasons[0]
                 else:
                     msg = 'Reloaded DNS configuration: \n' + '\n'.join(
@@ -286,6 +297,16 @@ class RegionControllerService(Service):
             self.previousSerial = serial
             return d
 
+    def _onDNSReloadFailure(self, failure):
+        """Force kill and restart bind9."""
+        failure.trap(DNSReloadError)
+        if not self.retryOnFailure:
+            return failure
+        log.err(failure, "Failed configuring DNS; killing and restarting")
+        d = service_monitor.killService('bind9')
+        d.addErrback(log.err, "Failed to kill and restart DNS.")
+        return d
+
     @transactional
     def _getReloadReasons(self, previousSerial, currentSerial):
         return [
diff --git a/src/maasserver/service_monitor.py b/src/maasserver/service_monitor.py
index 92baa43..c902f86 100644
--- a/src/maasserver/service_monitor.py
+++ b/src/maasserver/service_monitor.py
@@ -30,6 +30,9 @@ class BIND9Service(AlwaysOnService):
     service_name = "bind9"
     snap_service_name = "bind9"
 
+    # Pass SIGKILL directly to parent.
+    kill_extra_opts = ('-s', 'SIGKILL')
+
 
 class NTPServiceOnRegion(AlwaysOnService):
     """Monitored NTP service on a region controller host."""
diff --git a/src/maasserver/tests/test_region_controller.py b/src/maasserver/tests/test_region_controller.py
index 2c109db..b00fb2d 100644
--- a/src/maasserver/tests/test_region_controller.py
+++ b/src/maasserver/tests/test_region_controller.py
@@ -212,7 +212,7 @@ class TestRegionControllerService(MAASServerTestCase):
         service = self.make_service(sentinel.listener)
         service.needsDNSUpdate = True
         dns_result = (
-            random.randint(1, 1000), [
+            random.randint(1, 1000), True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
@@ -234,6 +234,46 @@ class TestRegionControllerService(MAASServerTestCase):
 
     @wait_for_reactor
     @inlineCallbacks
+    def test_process_zones_kills_bind_on_failed_reload(self):
+        service = self.make_service(sentinel.listener)
+        service.needsDNSUpdate = True
+        service.retryOnFailure = True
+        dns_result_0 = (
+            random.randint(1, 1000), False, [
+                factory.make_name('domain')
+                for _ in range(3)
+            ])
+        dns_result_1 = (dns_result_0[0], True, dns_result_0[2])
+        mock_dns_update_all_zones = self.patch(
+            region_controller, "dns_update_all_zones")
+        mock_dns_update_all_zones.side_effect = [dns_result_0, dns_result_1]
+
+        service._checkSerialCalled = False
+        orig_checkSerial = service._checkSerial
+
+        def _checkSerial(result):
+            if service._checkSerialCalled:
+                return dns_result_1
+            service._checkSerialCalled = True
+            return orig_checkSerial(result)
+
+        mock_check_serial = self.patch(service, "_checkSerial")
+        mock_check_serial.side_effect = _checkSerial
+        mock_killService = self.patch(
+            region_controller.service_monitor, 'killService')
+        mock_killService.return_value = succeed(None)
+        service.startProcessing()
+        yield service.processingDefer
+        self.assertThat(
+            mock_dns_update_all_zones, MockCallsMatch(call(), call()))
+        self.assertThat(
+            mock_check_serial,
+            MockCallsMatch(call(dns_result_0), call(dns_result_1)))
+        self.assertThat(
+            mock_killService, MockCalledOnceWith("bind9"))
+
+    @wait_for_reactor
+    @inlineCallbacks
     def test_process_updates_proxy(self):
         service = self.make_service(sentinel.listener)
         service.needsProxyUpdate = True
@@ -348,7 +388,7 @@ class TestRegionControllerService(MAASServerTestCase):
         service.needsProxyUpdate = True
         service.needsRBACUpdate = True
         dns_result = (
-            random.randint(1, 1000), [
+            random.randint(1, 1000), True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
@@ -407,7 +447,7 @@ class TestRegionControllerService(MAASServerTestCase):
             succeed(([self.make_soa_result(result_serial)], [], [])),
         ]
         # Error should not be raised.
-        return service._checkSerial((formatted_serial, dns_names))
+        return service._checkSerial((formatted_serial, True, dns_names))
 
     @wait_for_reactor
     @inlineCallbacks
@@ -425,7 +465,7 @@ class TestRegionControllerService(MAASServerTestCase):
         mock_lookup.side_effect = lambda *args: succeed(([], [], []))
         # Error should not be raised.
         with ExpectedException(DNSReloadError):
-            yield service._checkSerial((formatted_serial, dns_names))
+            yield service._checkSerial((formatted_serial, True, dns_names))
 
     @wait_for_reactor
     @inlineCallbacks
@@ -443,7 +483,7 @@ class TestRegionControllerService(MAASServerTestCase):
         mock_lookup.side_effect = ValueError()
         # Error should not be raised.
         with ExpectedException(DNSReloadError):
-            yield service._checkSerial((formatted_serial, dns_names))
+            yield service._checkSerial((formatted_serial, True, dns_names))
 
     @wait_for_reactor
     @inlineCallbacks
@@ -461,7 +501,7 @@ class TestRegionControllerService(MAASServerTestCase):
         mock_lookup.side_effect = TimeoutError()
         # Error should not be raised.
         with ExpectedException(DNSReloadError):
-            yield service._checkSerial((formatted_serial, dns_names))
+            yield service._checkSerial((formatted_serial, True, dns_names))
 
     def test__getRBACClient_returns_None_when_no_url(self):
         service = self.make_service(sentinel.listener)
@@ -565,7 +605,7 @@ class TestRegionControllerServiceTransactional(MAASTransactionServerTestCase):
         service.needsDNSUpdate = True
         service.previousSerial = publications[0].serial
         dns_result = (
-            publications[-1].serial, [
+            publications[-1].serial, True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
@@ -602,7 +642,7 @@ class TestRegionControllerServiceTransactional(MAASTransactionServerTestCase):
         service.needsDNSUpdate = True
         service.previousSerial = publications[0].serial
         dns_result = (
-            publications[-1].serial, [
+            publications[-1].serial, True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
diff --git a/src/provisioningserver/dns/actions.py b/src/provisioningserver/dns/actions.py
index a955023..d8439f8 100644
--- a/src/provisioningserver/dns/actions.py
+++ b/src/provisioningserver/dns/actions.py
@@ -13,7 +13,10 @@ __all__ = [
 ]
 
 import collections
-from subprocess import CalledProcessError
+from subprocess import (
+    CalledProcessError,
+    TimeoutExpired,
+)
 from time import sleep
 
 from provisioningserver.dns.config import (
@@ -48,7 +51,7 @@ def bind_reconfigure():
         raise
 
 
-def bind_reload():
+def bind_reload(timeout=2):
     """Ask BIND to reload its configuration and all zone files.  This operation
     is 'best effort' (with logging) as the server may not be running, and there
     is often no context for reporting.
@@ -56,21 +59,24 @@ def bind_reload():
     :return: True if success, False otherwise.
     """
     try:
-        execute_rndc_command(("reload",))
+        execute_rndc_command(("reload",), timeout=timeout)
         return True
     except CalledProcessError as exc:
         maaslog.error("Reloading BIND failed (is it running?): %s", exc)
         return False
+    except TimeoutExpired as exc:
+        maaslog.error("Reloading BIND timed out (is it locked?): %s", exc)
+        return False
 
 
-def bind_reload_with_retries(attempts=10, interval=2):
+def bind_reload_with_retries(attempts=10, interval=2, timeout=2):
     """Ask BIND to reload its configuration and all zone files.
 
     :param attempts: The number of attempts.
     :param interval: The time in seconds to sleep between each attempt.
     """
     for countdown in range(attempts - 1, -1, -1):
-        if bind_reload():
+        if bind_reload(timeout=timeout):
             break
         if countdown == 0:
             break
diff --git a/src/provisioningserver/dns/config.py b/src/provisioningserver/dns/config.py
index 715b36e..f196902 100644
--- a/src/provisioningserver/dns/config.py
+++ b/src/provisioningserver/dns/config.py
@@ -177,12 +177,12 @@ def set_up_rndc():
         f.write(named_content)
 
 
-def execute_rndc_command(arguments):
+def execute_rndc_command(arguments, timeout=None):
     """Execute a rndc command."""
     rndc_conf = get_rndc_conf_path()
     rndc_cmd = ['rndc', '-c', rndc_conf]
     rndc_cmd.extend(arguments)
-    call_and_check(rndc_cmd)
+    call_and_check(rndc_cmd, timeout=timeout)
 
 
 def set_up_options_conf(overwrite=True, **kwargs):
diff --git a/src/provisioningserver/dns/tests/test_actions.py b/src/provisioningserver/dns/tests/test_actions.py
index 472bb56..58d216e 100644
--- a/src/provisioningserver/dns/tests/test_actions.py
+++ b/src/provisioningserver/dns/tests/test_actions.py
@@ -78,7 +78,7 @@ class TestReload(MAASTestCase):
         actions.bind_reload()
         self.assertThat(
             actions.execute_rndc_command,
-            MockCalledOnceWith(("reload",)))
+            MockCalledOnceWith(("reload",), timeout=2))
 
     def test__logs_subprocess_error(self):
         erc = self.patch_autospec(actions, "execute_rndc_command")
@@ -105,7 +105,7 @@ class TestReloadWithRetries(MAASTestCase):
         bind_reload.return_value = False
         attempts = randint(3, 13)
         actions.bind_reload_with_retries(attempts=attempts)
-        expected_calls = [call()] * attempts
+        expected_calls = [call(timeout=2)] * attempts
         self.assertThat(
             actions.bind_reload,
             MockCallsMatch(*expected_calls))
@@ -114,11 +114,11 @@ class TestReloadWithRetries(MAASTestCase):
         self.patch_autospec(actions, "sleep")  # Disable.
         bind_reload = self.patch(actions, "bind_reload")
         bind_reload_return_values = [False, False, True, ]
-        bind_reload.side_effect = lambda: (
+        bind_reload.side_effect = lambda *args, **kwargs: (
             bind_reload_return_values.pop(0))
 
         actions.bind_reload_with_retries(attempts=5)
-        expected_calls = [call(), call(), call()]
+        expected_calls = [call(timeout=2), call(timeout=2), call(timeout=2)]
         self.assertThat(
             actions.bind_reload,
             MockCallsMatch(*expected_calls))
diff --git a/src/provisioningserver/dns/tests/test_config.py b/src/provisioningserver/dns/tests/test_config.py
index f569394..902ab12 100644
--- a/src/provisioningserver/dns/tests/test_config.py
+++ b/src/provisioningserver/dns/tests/test_config.py
@@ -9,7 +9,10 @@ import errno
 import os.path
 import random
 from textwrap import dedent
-from unittest.mock import Mock
+from unittest.mock import (
+    Mock,
+    sentinel,
+)
 
 from fixtures import EnvironmentVariable
 from maastesting.factory import factory
@@ -278,10 +281,11 @@ class TestRNDCUtilities(MAASTestCase):
         fake_dir = patch_dns_config_path(self)
         self.patch(config, 'call_and_check', recorder)
         command = factory.make_string()
-        execute_rndc_command([command])
+        execute_rndc_command([command], timeout=sentinel.timeout)
         rndc_conf_path = os.path.join(fake_dir, MAAS_RNDC_CONF_NAME)
         expected_command = ['rndc', '-c', rndc_conf_path, command]
         self.assertEqual((expected_command,), recorder.calls[0][0])
+        self.assertEqual({'timeout': sentinel.timeout}, recorder.calls[0][1])
 
     def test_extract_suggested_named_conf_extracts_section(self):
         named_part = factory.make_string()
diff --git a/src/provisioningserver/service_monitor.py b/src/provisioningserver/service_monitor.py
index 75caab2..701fe85 100644
--- a/src/provisioningserver/service_monitor.py
+++ b/src/provisioningserver/service_monitor.py
@@ -50,6 +50,9 @@ class DNSServiceOnRack(ToggleableService):
     service_name = "bind9"
     snap_service_name = "bind9"
 
+    # Pass SIGKILL directly to parent.
+    kill_extra_opts = ('-s', 'SIGKILL')
+
 
 class ProxyServiceOnRack(ToggleableService):
     """Monitored proxy service on a rack controller host."""
diff --git a/src/provisioningserver/utils/service_monitor.py b/src/provisioningserver/utils/service_monitor.py
index a224632..2959679 100644
--- a/src/provisioningserver/utils/service_monitor.py
+++ b/src/provisioningserver/utils/service_monitor.py
@@ -413,6 +413,26 @@ class ServiceMonitor:
             raise ServiceActionError(error_msg)
         yield self._performServiceAction(service, "reload")
 
+    @asynchronous
+    @inlineCallbacks
+    def killService(self, name):
+        """Kill service.
+
+        Service will be killed then its state will be ensured to be in its
+        expected state. This is a way of getting a broken related service to be
+        responsive.
+        """
+        service = self.getServiceByName(name)
+        try:
+            yield self._performServiceAction(service, "kill")
+        except ServiceActionError:
+            # Kill action is allowed to fail, as its possible the service is
+            # already dead or not responding to correct signals to check
+            # the current status.
+            pass
+        state = yield self.ensureService(name)
+        return state
+
     @inlineCallbacks
     def _execCmd(self, cmd, env, timeout=8, retries=3):
         """Execute the `cmd` with the `env`."""
@@ -468,14 +488,46 @@ class ServiceMonitor:
         return self._execCmd(cmd, env)
 
     @asynchronous
-    def _execSupervisorServiceAction(self, service_name, action):
+    def _execSupervisorServiceAction(
+            self, service_name, action, extra_opts=None):
         """Perform the action with the run-supervisorctl command.
 
         :return: tuple (exit code, std-output, std-error)
         """
         env = get_env_with_bytes_locale()
         cmd = os.path.join(snappy.get_snap_path(), "bin", "run-supervisorctl")
-        cmd = (cmd, action, service_name)
+
+        # supervisord doesn't support native kill like systemd. Emulate this
+        # behaviour by getting the PID of the process and then killing the PID.
+        if action == 'kill':
+
+            def _kill_pid(result):
+                exit_code, stdout, _ = result
+                if exit_code != 0:
+                    return result
+                try:
+                    pid = int(stdout.strip())
+                except ValueError:
+                    pid = 0
+                if pid == 0:
+                    # supervisorctl returns 0 when the process is already dead
+                    # or we where not able to get the actual pid. Nothing to
+                    # do, as its already dead.
+                    return 0, "", ""
+                cmd = ('kill',)
+                if extra_opts:
+                    cmd += extra_opts
+                cmd += ('%s' % pid,)
+                return self._execCmd(cmd, env)
+
+            d = self._execCmd((cmd, 'pid', service_name), env)
+            d.addCallback(_kill_pid)
+            return d
+
+        cmd = (cmd, action)
+        if extra_opts is not None:
+            cmd += extra_opts
+        cmd += (service_name,)
         return self._execCmd(cmd, env)
 
     @inlineCallbacks
@@ -488,8 +540,9 @@ class ServiceMonitor:
         else:
             exec_action = self._execSystemDServiceAction
             service_name = service.service_name
+        extra_opts = getattr(service, '%s_extra_opts' % action, None)
         exit_code, output, error = yield lock.run(
-            exec_action, service_name, action)
+            exec_action, service_name, action, extra_opts=extra_opts)
         if exit_code != 0:
             error_msg = (
                 "Service '%s' failed to %s: %s" % (
diff --git a/src/provisioningserver/utils/shell.py b/src/provisioningserver/utils/shell.py
index 0ec7c39..488afb6 100644
--- a/src/provisioningserver/utils/shell.py
+++ b/src/provisioningserver/utils/shell.py
@@ -105,8 +105,9 @@ def call_and_check(command, *args, **kwargs):
     :return: The command's output from standard output.
     :raise ExternalProcessError: If the command returns nonzero.
     """
+    timeout = kwargs.pop('timeout', None)
     process = Popen(command, *args, stdout=PIPE, stderr=PIPE, **kwargs)
-    stdout, stderr = process.communicate()
+    stdout, stderr = process.communicate(timeout=timeout)
     stderr = stderr.strip()
     if process.returncode != 0:
         raise ExternalProcessError(process.returncode, command, output=stderr)
diff --git a/src/provisioningserver/utils/tests/test_service_monitor.py b/src/provisioningserver/utils/tests/test_service_monitor.py
index eb984ed..11e71da 100644
--- a/src/provisioningserver/utils/tests/test_service_monitor.py
+++ b/src/provisioningserver/utils/tests/test_service_monitor.py
@@ -10,6 +10,7 @@ import os
 import random
 from textwrap import dedent
 from unittest.mock import (
+    call,
     Mock,
     sentinel,
 )
@@ -18,6 +19,7 @@ from fixtures import FakeLogger
 from maastesting.factory import factory
 from maastesting.matchers import (
     MockCalledOnceWith,
+    MockCallsMatch,
     MockNotCalled,
 )
 from maastesting.runtest import MAASTwistedRunTest
@@ -50,6 +52,7 @@ from twisted.internet import reactor
 from twisted.internet.defer import (
     CancelledError,
     DeferredLock,
+    fail,
     inlineCallbacks,
     succeed,
 )
@@ -452,6 +455,42 @@ class TestServiceMonitor(MAASTestCase):
             yield service_monitor.reloadService(fake_service.name, if_on=True)
 
     @inlineCallbacks
+    def test__killService_performs_kill_then_ensureService(self):
+        fake_service = make_fake_service(SERVICE_STATE.ON)
+        service_monitor = self.make_service_monitor([fake_service])
+        mock_performServiceAction = self.patch(
+            service_monitor, "_performServiceAction")
+        mock_performServiceAction.return_value = succeed(None)
+        mock_ensureService = self.patch(service_monitor, "ensureService")
+        mock_ensureService.return_value = succeed(
+            ServiceState(SERVICE_STATE.ON, "running"))
+        yield service_monitor.killService(fake_service.name)
+        self.assertThat(
+            mock_performServiceAction,
+            MockCalledOnceWith(fake_service, "kill"))
+        self.assertThat(
+            mock_ensureService,
+            MockCalledOnceWith(fake_service.name))
+
+    @inlineCallbacks
+    def test__killService_doesnt_fail_on_ServiceActionError(self):
+        fake_service = make_fake_service(SERVICE_STATE.ON)
+        service_monitor = self.make_service_monitor([fake_service])
+        mock_performServiceAction = self.patch(
+            service_monitor, "_performServiceAction")
+        mock_performServiceAction.return_value = fail(ServiceActionError())
+        mock_ensureService = self.patch(service_monitor, "ensureService")
+        mock_ensureService.return_value = succeed(
+            ServiceState(SERVICE_STATE.ON, "running"))
+        yield service_monitor.killService(fake_service.name)
+        self.assertThat(
+            mock_performServiceAction,
+            MockCalledOnceWith(fake_service, "kill"))
+        self.assertThat(
+            mock_ensureService,
+            MockCalledOnceWith(fake_service.name))
+
+    @inlineCallbacks
     def test___execCmd_times_out(self):
         monitor = ServiceMonitor(make_fake_service())
         with ExpectedException(ServiceActionError):
@@ -535,16 +574,44 @@ class TestServiceMonitor(MAASTestCase):
         mock_getProcessOutputAndValue = self.patch(
             service_monitor_module, "getProcessOutputAndValue")
         mock_getProcessOutputAndValue.return_value = succeed((b"", b"", 0))
+        extra_opts = ('--extra', factory.make_name('extra'))
         yield service_monitor._execSupervisorServiceAction(
-            service_name, action)
+            service_name, action, extra_opts=extra_opts)
         cmd = os.path.join(snap_path, 'bin', 'run-supervisorctl')
-        cmd = cmd, action, service_name
+        cmd = (cmd, action) + extra_opts + (service_name,)
         self.assertThat(
             mock_getProcessOutputAndValue, MockCalledOnceWith(
                 # The environment contains LC_ALL and LANG too.
                 cmd[0], cmd[1:], env=get_env_with_bytes_locale()))
 
     @inlineCallbacks
+    def test___execSupervisorServiceAction_emulates_kill(self):
+        snap_path = factory.make_name("path")
+        self.patch(snappy, "get_snap_path").return_value = snap_path
+        service_monitor = self.make_service_monitor()
+        service_name = factory.make_name("service")
+        fake_pid = random.randint(1, 100)
+        mock_getProcessOutputAndValue = self.patch(
+            service_monitor_module, "getProcessOutputAndValue")
+        mock_getProcessOutputAndValue.side_effect = [
+            succeed((("%s" % fake_pid).encode('utf-8'), b"", 0)),
+            succeed((b"", b"", 0)),
+        ]
+        extra_opts = ('-s', factory.make_name('SIGKILL'))
+        yield service_monitor._execSupervisorServiceAction(
+            service_name, 'kill', extra_opts=extra_opts)
+        cmd = os.path.join(snap_path, 'bin', 'run-supervisorctl')
+        self.assertThat(
+            mock_getProcessOutputAndValue, MockCallsMatch(
+                call(
+                    cmd, ("pid", service_name),
+                    env=get_env_with_bytes_locale()),
+                call(
+                    "kill", extra_opts + ("%s" % fake_pid,),
+                    env=get_env_with_bytes_locale()),
+            ))
+
+    @inlineCallbacks
     def test___execSupervisorServiceAction_decodes_stdout_and_stderr(self):
         # From https://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-demo.txt.
         example_text = (
@@ -579,13 +646,16 @@ class TestServiceMonitor(MAASTestCase):
             service_monitor, "_execSystemDServiceAction")
         mock_execSystemDServiceAction.return_value = (0, "", "")
         action = factory.make_name("action")
+        extra_opts = ('--option', factory.make_name("option"))
+        setattr(service, '%s_extra_opts' % action, extra_opts)
         yield service_monitor._performServiceAction(service, action)
         self.assertThat(service_lock.run, MockCalledOnceWith(
             service_monitor._execSystemDServiceAction,
-            service.service_name, action))
+            service.service_name, action, extra_opts=extra_opts))
         self.assertThat(
             mock_execSystemDServiceAction,
-            MockCalledOnceWith(service.service_name, action))
+            MockCalledOnceWith(
+                service.service_name, action, extra_opts=extra_opts))
 
     @inlineCallbacks
     def test___performServiceAction_holds_lock_perform_supervisor_action(self):
@@ -599,13 +669,16 @@ class TestServiceMonitor(MAASTestCase):
             service_monitor, "_execSupervisorServiceAction")
         mock_execSupervisorServiceAction.return_value = (0, "", "")
         action = factory.make_name("action")
+        extra_opts = ('--option', factory.make_name("option"))
+        setattr(service, '%s_extra_opts' % action, extra_opts)
         yield service_monitor._performServiceAction(service, action)
         self.assertThat(service_lock.run, MockCalledOnceWith(
             service_monitor._execSupervisorServiceAction,
-            service.service_name, action))
+            service.service_name, action, extra_opts=extra_opts))
         self.assertThat(
             mock_execSupervisorServiceAction,
-            MockCalledOnceWith(service.service_name, action))
+            MockCalledOnceWith(
+                service.service_name, action, extra_opts=extra_opts))
 
     @inlineCallbacks
     def test___performServiceAction_raises_ServiceActionError_if_fails(self):
diff --git a/src/provisioningserver/utils/tests/test_shell.py b/src/provisioningserver/utils/tests/test_shell.py
index 4492b3a..03f67fd 100644
--- a/src/provisioningserver/utils/tests/test_shell.py
+++ b/src/provisioningserver/utils/tests/test_shell.py
@@ -6,6 +6,7 @@
 __all__ = []
 
 import os
+import random
 from subprocess import CalledProcessError
 
 from fixtures import EnvironmentVariable
@@ -54,6 +55,14 @@ class TestCallAndCheck(MAASTestCase):
         self.assertEqual(command, error.cmd)
         self.assertEqual(message, error.output)
 
+    def test__passes_timeout_to_communicate(self):
+        command = factory.make_name('command')
+        process = self.patch_popen()
+        timeout = random.randint(1, 10)
+        call_and_check(command, timeout=timeout)
+        self.assertThat(
+            process.communicate, MockCalledOnceWith(timeout=timeout))
+
     def test__reports_stderr_on_failure(self):
         nonfile = os.path.join(self.make_dir(), factory.make_name('nonesuch'))
         error = self.assertRaises(