Merge lp:~trapnine/maas/fix-1499378 into lp:~maas-committers/maas/trunk

[Jeffrey C Jones]

Prevent rndc from raising an exception that causes regiond to crash.

[Blake Rouse (blake-rouse)]

Looks good.

Please make the commit message more of a sentence.

"Prevent rndc from raising an exception that causes regiond to crash."

review: Approve

=== modified file 'src/provisioningserver/dns/actions.py'
--- src/provisioningserver/dns/actions.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/actions.py	2015-10-22 00:13:47 +0000
@@ -58,14 +58,18 @@
 
 
 def bind_reload():
-    """Ask BIND to reload its configuration and all zone files."""
+    """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.
+
+    :return: True if success, False otherwise.
+    """
     try:
         execute_rndc_command(("reload",))
+        return True
     except CalledProcessError as exc:
-        maaslog.error("Reloading BIND failed: %s", exc)
-        # Log before upgrade so that the output does not go to maaslog.
-        ExternalProcessError.upgrade(exc)
-        raise
+        maaslog.error("Reloading BIND failed (is it running?): %s", exc)
+        return False
 
 
 def bind_reload_with_retries(attempts=10, interval=2):
@@ -75,29 +79,29 @@
     :param interval: The time in seconds to sleep between each attempt.
     """
     for countdown in xrange(attempts - 1, -1, -1):
-        try:
-            bind_reload()
-        except CalledProcessError:
-            if countdown == 0:
-                raise
-            else:
-                sleep(interval)
+        if bind_reload():
+            break
+        if countdown == 0:
+            break
         else:
-            break
+            sleep(interval)
 
 
 def bind_reload_zone(zone_name):
     """Ask BIND to reload the zone file for the given zone.
 
     :param zone_name: The name of the zone to reload.
+    :return: True if success, False otherwise.
     """
     try:
         execute_rndc_command(("reload", zone_name))
+        return True
     except CalledProcessError as exc:
-        maaslog.error("Reloading BIND zone %r failed: %s", zone_name, exc)
-        # Log before upgrade so that the output does not go to maaslog.
-        ExternalProcessError.upgrade(exc)
-        raise
+        maaslog.error(
+            "Reloading BIND zone %r failed (is it running?): %s",
+            zone_name,
+            exc)
+        return False
 
 
 def bind_write_configuration(zones, trusted_networks):

=== modified file 'src/provisioningserver/dns/tests/test_actions.py'
--- src/provisioningserver/dns/tests/test_actions.py	2015-05-07 18:14:38 +0000
+++ src/provisioningserver/dns/tests/test_actions.py	2015-10-22 00:13:47 +0000
@@ -93,16 +93,16 @@
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
         with FakeLogger("maas") as logger:
-            self.assertRaises(CalledProcessError, actions.bind_reload)
+            self.assertFalse(actions.bind_reload())
         self.assertDocTestMatches(
-            "Reloading BIND failed: "
+            "Reloading BIND failed (is it running?): "
             "Command ... returned non-zero exit status ...",
             logger.output)
 
-    def test__upgrades_subprocess_error(self):
+    def test__false_on_subprocess_error(self):
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
-        self.assertRaises(ExternalProcessError, actions.bind_reload)
+        self.assertFalse(actions.bind_reload())
 
 
 class TestReloadWithRetries(MAASTestCase):
@@ -111,11 +111,9 @@
     def test__calls_bind_reload_count_times(self):
         self.patch_autospec(actions, "sleep")  # Disable.
         bind_reload = self.patch_autospec(actions, "bind_reload")
-        bind_reload.side_effect = factory.make_CalledProcessError()
+        bind_reload.return_value = False
         attempts = randint(3, 13)
-        self.assertRaises(
-            CalledProcessError, actions.bind_reload_with_retries,
-            attempts=attempts)
+        actions.bind_reload_with_retries(attempts=attempts)
         expected_calls = [call()] * attempts
         self.assertThat(
             actions.bind_reload,
@@ -124,11 +122,10 @@
     def test__returns_on_success(self):
         self.patch_autospec(actions, "sleep")  # Disable.
         bind_reload = self.patch(actions, "bind_reload")
-        bind_reload.side_effect = [
-            factory.make_CalledProcessError(),
-            factory.make_CalledProcessError(),
-            None,  # Success
-        ]
+        bind_reload_return_values = [False, False, True, ]
+        bind_reload.side_effect = lambda: (
+            bind_reload_return_values.pop(0))
+
         actions.bind_reload_with_retries(attempts=5)
         expected_calls = [call(), call(), call()]
         self.assertThat(
@@ -138,10 +135,9 @@
     def test__sleeps_interval_seconds_between_attempts(self):
         self.patch_autospec(actions, "sleep")  # Disable.
         bind_reload = self.patch_autospec(actions, "bind_reload")
-        bind_reload.side_effect = factory.make_CalledProcessError()
+        bind_reload.return_value = False
         attempts = randint(3, 13)
-        self.assertRaises(
-            CalledProcessError, actions.bind_reload_with_retries,
+        actions.bind_reload_with_retries(
             attempts=attempts, interval=sentinel.interval)
         expected_sleep_calls = [call(sentinel.interval)] * (attempts - 1)
         self.assertThat(actions.sleep, MockCallsMatch(*expected_sleep_calls))
@@ -152,7 +148,7 @@
 
     def test__executes_rndc_command(self):
         self.patch_autospec(actions, "execute_rndc_command")
-        actions.bind_reload_zone(sentinel.zone)
+        self.assertTrue(actions.bind_reload_zone(sentinel.zone))
         self.assertThat(
             actions.execute_rndc_command,
             MockCalledOnceWith(("reload", sentinel.zone)))
@@ -161,18 +157,16 @@
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
         with FakeLogger("maas") as logger:
-            self.assertRaises(
-                CalledProcessError, actions.bind_reload_zone, sentinel.zone)
+            self.assertFalse(actions.bind_reload_zone(sentinel.zone))
         self.assertDocTestMatches(
-            "Reloading BIND zone ... failed: "
+            "Reloading BIND zone ... failed (is it running?): "
             "Command ... returned non-zero exit status ...",
             logger.output)
 
-    def test__upgrades_subprocess_error(self):
+    def test__false_on_subprocess_error(self):
         erc = self.patch_autospec(actions, "execute_rndc_command")
         erc.side_effect = factory.make_CalledProcessError()
-        self.assertRaises(
-            ExternalProcessError, actions.bind_reload_zone, sentinel.zone)
+        self.assertFalse(actions.bind_reload_zone(sentinel.zone))
 
 
 class TestConfiguration(PservTestCase):