Merge ~blake-rouse/maas:fix-1710278-2.4 into maas:2.4

[Blake Rouse]

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

Backport of d0a4ec6ce2af49d00431c0d7ad4c5164d3951e1a.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1710278-2.4 lp:~blake-rouse/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6400/console
COMMIT: 06640035f714bada456cbca5b7e577e86b7e892e

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1710278-2.4 lp:~blake-rouse/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6402/console
COMMIT: 50a45ad9cd72dbcea4a5d099a864c62249ff26f5

review: Needs Fixing

[Adam Collard (adam-collard)]

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b fix-1710278-2.4 lp:~blake-rouse/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6729/console
COMMIT: c64105dbfb38863b5c7b38616864d2c379ac356c

review: Needs Fixing

[Blake Rouse (blake-rouse)]

The failures from the test running are javascript failures, which have not been touched in this branch.

[MAAS Lander (maas-lander)]

LANDING
-b fix-1710278-2.4 lp:~blake-rouse/maas/+git/maas into -b 2.4 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6730/consoleText

diff --git a/debian/extras/99-maas-common-sudoers b/debian/extras/99-maas-common-sudoers
index 1475b44..d333d65 100644
--- a/debian/extras/99-maas-common-sudoers
+++ b/debian/extras/99-maas-common-sudoers
@@ -11,6 +11,12 @@ 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, 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
+
 # Writing and deleting files as root.
 maas ALL= NOPASSWD: /usr/lib/maas/maas-delete-file
 maas ALL= NOPASSWD: /usr/lib/maas/maas-write-file
diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index e27773a..5312b42 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -42,7 +42,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
@@ -84,12 +84,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 c163420..e8303d4 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -53,6 +53,7 @@ from testtools.matchers import (
     Contains,
     Equals,
     FileContains,
+    Is,
     MatchesSetwise,
     MatchesStructure,
 )
@@ -255,7 +256,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)
@@ -314,8 +316,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 e54b0a9..cd62138 100644
--- a/src/maasserver/region_controller.py
+++ b/src/maasserver/region_controller.py
@@ -26,6 +26,7 @@ __all__ = [
 from maasserver.dns.config import dns_update_all_zones
 from maasserver.models.dnspublication import DNSPublication
 from maasserver.proxyconfig import proxy_update_config
+from maasserver.service_monitor import service_monitor
 from maasserver.utils.orm import transactional
 from maasserver.utils.threads import deferToDatabase
 from provisioningserver.logger import LegacyLogger
@@ -60,7 +61,7 @@ class RegionControllerService(Service):
     See module documentation for more details.
     """
 
-    def __init__(self, postgresListener, clock=reactor):
+    def __init__(self, postgresListener, clock=reactor, retryOnFailure=True):
         """Initialise a new `RegionControllerService`.
 
         :param postgresListener: The `PostgresListenerService` that is running
@@ -68,6 +69,7 @@ class RegionControllerService(Service):
         """
         super(RegionControllerService, self).__init__()
         self.clock = clock
+        self.retryOnFailure = retryOnFailure
         self.processing = LoopingCall(self.process)
         self.processing.clock = self.clock
         self.processingDefer = None
@@ -118,12 +120,27 @@ class RegionControllerService(Service):
 
     def process(self):
         """Process the DNS and/or proxy update."""
+
+        def _onFailureRetry(failure, attr):
+            """Retry update on failure.
+
+            Doesn't mask the failure, the failure is still raised.
+            """
+            if self.retryOnFailure:
+                setattr(self, attr, True)
+            return failure
+
         defers = []
         if self.needsDNSUpdate:
             self.needsDNSUpdate = False
             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.")
@@ -150,7 +167,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:
@@ -173,13 +193,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
@@ -190,7 +210,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(
@@ -207,6 +230,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 6efd9b7..05d235b 100644
--- a/src/maasserver/service_monitor.py
+++ b/src/maasserver/service_monitor.py
@@ -29,6 +29,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 090e590..6926783 100644
--- a/src/maasserver/tests/test_region_controller.py
+++ b/src/maasserver/tests/test_region_controller.py
@@ -183,7 +183,7 @@ class TestRegionControllerService(MAASServerTestCase):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
         dns_result = (
-            random.randint(1, 1000), [
+            random.randint(1, 1000), True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
@@ -205,6 +205,46 @@ class TestRegionControllerService(MAASServerTestCase):
 
     @wait_for_reactor
     @inlineCallbacks
+    def test_process_zones_kills_bind_on_failed_reload(self):
+        service = RegionControllerService(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 = RegionControllerService(sentinel.listener)
         service.needsProxyUpdate = True
@@ -226,6 +266,7 @@ class TestRegionControllerService(MAASServerTestCase):
     def test_process_updates_zones_logs_failure(self):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
+        service.retryOnFailure = False
         mock_dns_update_all_zones = self.patch(
             region_controller, "dns_update_all_zones")
         mock_dns_update_all_zones.side_effect = factory.make_exception()
@@ -263,7 +304,7 @@ class TestRegionControllerService(MAASServerTestCase):
         service.needsDNSUpdate = True
         service.needsProxyUpdate = True
         dns_result = (
-            random.randint(1, 1000), [
+            random.randint(1, 1000), True, [
                 factory.make_name('domain')
                 for _ in range(3)
             ])
@@ -318,7 +359,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
@@ -336,7 +377,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
@@ -354,7 +395,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
@@ -372,7 +413,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))
 
 
 class TestRegionControllerServiceTransactional(MAASTransactionServerTestCase):
@@ -393,7 +434,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)
             ])
@@ -430,7 +471,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 97ad042..5415184 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 b03b0fb..eb780d0 100644
--- a/src/provisioningserver/dns/config.py
+++ b/src/provisioningserver/dns/config.py
@@ -172,12 +172,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 a5ecf1b..adfa0e5 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
@@ -273,10 +276,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/utils/service_monitor.py b/src/provisioningserver/utils/service_monitor.py
index 43c2f83..ac4f3c6 100644
--- a/src/provisioningserver/utils/service_monitor.py
+++ b/src/provisioningserver/utils/service_monitor.py
@@ -35,8 +35,12 @@ from provisioningserver.utils import (
     typed,
 )
 from provisioningserver.utils.shell import get_env_with_bytes_locale
-from provisioningserver.utils.twisted import asynchronous
+from provisioningserver.utils.twisted import (
+    asynchronous,
+    deferWithTimeout,
+)
 from twisted.internet.defer import (
+    CancelledError,
     DeferredList,
     DeferredLock,
     inlineCallbacks,
@@ -371,6 +375,67 @@ class ServiceMonitor:
         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`."""
+
+        def decode(result):
+            out, err, code = result
+            return code, out.decode("utf-8"), err.decode("utf-8")
+
+        def log_code(result, cmd, try_num):
+            _, _, code = result
+            log.debug(
+                "[try:{try_num}] Service monitor got exit "
+                "code '{code}' from cmd: {cmd()}",
+                try_num=try_num, code=code, cmd=lambda: ' '.join(cmd))
+            return result
+
+        def call_proc(cmd, env, try_num, timeout):
+            log.debug(
+                "[try:{try_num}] Service monitor executing cmd: {cmd()}",
+                try_num=try_num, cmd=lambda: ' '.join(cmd))
+
+            d = deferWithTimeout(
+                timeout, getProcessOutputAndValue, cmd[0], cmd[1:], env=env)
+            d.addCallback(log_code, cmd, try_num)
+            return d.addCallback(decode)
+
+        for try_num in range(retries):
+            try:
+                result = yield call_proc(cmd, env, try_num + 1, timeout)
+            except CancelledError:
+                if try_num == retries - 1:
+                    # Failed on final retry.
+                    raise ServiceActionError(
+                        "Service monitor timed out after '%d' "
+                        "seconds and '%s' retries running cmd: %s" % (
+                            timeout, retries, ' '.join(cmd)))
+                else:
+                    # Try again.
+                    continue
+            return result
+
+    @asynchronous
     def _execSystemDServiceAction(self, service_name, action, extra_opts=None):
         """Perform the action with the systemctl command.
 
@@ -390,21 +455,47 @@ class ServiceMonitor:
         return d.addCallback(decode)
 
     @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)
 
-        def decode(result):
-            out, err, code = result
-            return code, out.decode("utf-8"), err.decode("utf-8")
+        # 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
 
-        d = getProcessOutputAndValue(cmd[0], cmd[1:], env=env)
-        return d.addCallback(decode)
+        cmd = (cmd, action)
+        if extra_opts is not None:
+            cmd += extra_opts
+        cmd += (service_name,)
+        return self._execCmd(cmd, env)
 
     @inlineCallbacks
     def _performServiceAction(self, service, action):
@@ -416,8 +507,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 51d39c3..8cb507c 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,10 +19,12 @@ from fixtures import FakeLogger
 from maastesting.factory import factory
 from maastesting.matchers import (
     MockCalledOnceWith,
+    MockCallsMatch,
     MockNotCalled,
 )
 from maastesting.runtest import MAASTwistedRunTest
 from maastesting.testcase import MAASTestCase
+from maastesting.twisted import always_fail_with
 from provisioningserver.utils import (
     service_monitor as service_monitor_module,
     snappy,
@@ -37,6 +40,7 @@ from provisioningserver.utils.service_monitor import (
     ServiceUnknownError,
 )
 from provisioningserver.utils.shell import get_env_with_bytes_locale
+from provisioningserver.utils.twisted import pause
 from testscenarios import multiply_scenarios
 from testtools import ExpectedException
 from testtools.matchers import (
@@ -45,7 +49,9 @@ from testtools.matchers import (
 )
 from twisted.internet import reactor
 from twisted.internet.defer import (
+    CancelledError,
     DeferredLock,
+    fail,
     inlineCallbacks,
     succeed,
 )
@@ -448,6 +454,62 @@ 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):
+            yield monitor._execCmd(
+                ["sleep", "0.3"], {}, timeout=0.1, retries=1)
+        # Pause long enough for the reactor to cleanup the process.
+        yield pause(0.5)
+
+    @inlineCallbacks
+    def test___execCmd_retries(self):
+        monitor = ServiceMonitor(make_fake_service())
+        mock_deferWithTimeout = self.patch(
+            service_monitor_module, "deferWithTimeout")
+        mock_deferWithTimeout.side_effect = always_fail_with(CancelledError())
+        with ExpectedException(ServiceActionError):
+            yield monitor._execCmd(
+                ["echo", "Hello"], {}, retries=3)
+        self.assertEqual(3, mock_deferWithTimeout.call_count)
+
+    @inlineCallbacks
     def test___execSystemDServiceAction_calls_systemctl(self):
         service_monitor = self.make_service_monitor()
         service_name = factory.make_name("service")
@@ -511,16 +573,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 = (
@@ -555,13 +645,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):
@@ -575,13 +668,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(
diff --git a/versions.cfg b/versions.cfg
index 2d8eb7e..f214d65 100644
--- a/versions.cfg
+++ b/versions.cfg
@@ -25,9 +25,11 @@ linecache2 = 1.0.0
 nose = 1.3.7
 nose-timer = 0.6.0
 pbr = 1.10.0
+pexpect = 4.6.0
 pickleshare = 0.7.4
 postgresfixture = 0.4.1
 prompt-toolkit = 1.0.9
+ptyprocess = 0.6.0
 python-mimeparse = 1.6.0
 python-subunit = 1.2.0
 selenium = 2.45