Merge lp:~mpontillo/maas/fix-1524091-unmanaged-dhcp-for-deploy-and-delete-trunk into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Don't require DHCP to be on if it should be off.

[Gavin Panella (allenap)]

Just need to change the function name.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/fix-1524091-unmanaged-dhcp-for-deploy-and-delete-trunk into lp:maas failed. Below is the output from the failed tests.

Hit http://security.ubuntu.com xenial-security InRelease
Get:1 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial InRelease [227 kB]
Hit http://ppa.launchpad.net xenial InRelease
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates InRelease
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-backports InRelease
Hit http://security.ubuntu.com xenial-security/main Sources
Hit http://security.ubuntu.com xenial-security/restricted Sources
Hit http://security.ubuntu.com xenial-security/universe Sources
Hit http://security.ubuntu.com xenial-security/multiverse Sources
Hit http://ppa.launchpad.net xenial/main amd64 Packages
Hit http://security.ubuntu.com xenial-security/main amd64 Packages
Hit http://security.ubuntu.com xenial-security/restricted amd64 Packages
Hit http://security.ubuntu.com xenial-security/universe amd64 Packages
Hit http://ppa.launchpad.net xenial/main Translation-en
Hit http://security.ubuntu.com xenial-security/multiverse amd64 Packages
Hit http://security.ubuntu.com xenial-security/main Translation-en
Hit http://security.ubuntu.com xenial-security/multiverse Translation-en
Hit http://security.ubuntu.com xenial-security/restricted Translation-en
Hit http://security.ubuntu.com xenial-security/universe Translation-en
Get:2 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/main Sources [1,120 kB]
Get:3 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/restricted Sources [7,228 B]
Get:4 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/universe Sources [7,529 kB]
Get:5 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/multiverse Sources [177 kB]
Get:6 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/main amd64 Packages [1,450 kB]
Get:7 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/restricted amd64 Packages [15.8 kB]
Get:8 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/universe amd64 Packages [7,065 kB]
Get:9 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/multiverse amd64 Packages [139 kB]
Get:10 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/main Translation-en [845 kB]
Get:11 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/multiverse Translation-en [108 kB]
Get:12 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/restricted Translation-en [4,302 B]
Get:13 http://prodstack-zone-1.clouds.archive.ubuntu.com xenial/universe Translation-en [4,745 kB]
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/main Sources
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/restricted Sources
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/universe Sources
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/multiverse Sources
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/main amd64 Packages
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/restricted amd64 Packages
Hit http://prodstack-zone-1.clouds.archive.ubuntu.com xenial-updates/universe amd64 Packag...

=== modified file 'src/provisioningserver/rpc/dhcp.py'
--- src/provisioningserver/rpc/dhcp.py	2015-12-04 21:57:28 +0000
+++ src/provisioningserver/rpc/dhcp.py	2015-12-09 21:01:33 +0000
@@ -138,9 +138,18 @@
     return False
 
 
-def _ensure_dhcpv4_is_accessible(exception):
-    """Ensure that the DHCPv4 server is accessible. Raise `exception` if
-    it will not be possible to contact the server."""
+def _is_dhcpv4_managed_and_active(exception):
+    """Ensure that the DHCPv4 server is accessible (if necessary), and return
+    its status.
+
+    Returns True if the DHCPv4 server is accessible and should be accessible.
+    Returns False if the DHCPv4 server is not accessible, and should not be
+    accessible.
+    Raise `exception` if the DHCP server should be accessible, but cannot
+    be started.
+
+    :return:bool
+    """
     service = ServiceRegistry.get_item("dhcp4")
     if service.is_on():
         if service_monitor.get_service_state("dhcp4") != SERVICE_STATE.ON:
@@ -150,6 +159,8 @@
                     raise exception(
                         "DHCPv4 server started but was unable to connect "
                         "to omshell.")
+                else:
+                    return True
             except ServiceActionError as e:
                 # Error is already logged by the service monitor, nothing to
                 # log for this exception.
@@ -160,9 +171,11 @@
                 raise exception(error_msg)
         else:
             # Service should be on and is already on, nothing needs to be done.
-            pass
+            return True
     else:
-        raise exception("DHCPv4 server is disabled.")
+        # This is not an error; it just needs to be distinguished here so it
+        # can be handled differently.
+        return False
 
 
 @synchronous
@@ -173,7 +186,12 @@
         ``mac_address`` keys.
     :param shared_key: The key used to access the DHCP server via OMAPI.
     """
-    _ensure_dhcpv4_is_accessible(CannotCreateHostMap)
+    if not _is_dhcpv4_managed_and_active(CannotCreateHostMap):
+        # This will raise if DHCP is offline, but should be online.
+        # If the server is offline *and* should be offline, we'll get False
+        # back, which means this is a no-op.
+        return
+
     # See bug 1039362 regarding server_address.
     omshell = Omshell(server_address='127.0.0.1', shared_key=shared_key)
     for mapping in mappings:
@@ -207,7 +225,12 @@
     :param mac_addresses: A list of MAC addresses.
     :param shared_key: The key used to access the DHCP server via OMAPI.
     """
-    _ensure_dhcpv4_is_accessible(CannotRemoveHostMap)
+    if not _is_dhcpv4_managed_and_active(CannotRemoveHostMap):
+        # This will raise if DHCP is offline, but should be online.
+        # If the server is offline *and* should be offline, we'll get False
+        # back, which means this is a no-op.
+        return
+
     # See bug 1039362 regarding server_address.
     omshell = Omshell(server_address='127.0.0.1', shared_key=shared_key)
     for identifier in identifiers:

=== modified file 'src/provisioningserver/rpc/tests/test_dhcp.py'
--- src/provisioningserver/rpc/tests/test_dhcp.py	2015-12-01 18:12:59 +0000
+++ src/provisioningserver/rpc/tests/test_dhcp.py	2015-12-09 21:01:33 +0000
@@ -39,6 +39,7 @@
 from provisioningserver.utils.shell import ExternalProcessError
 from provisioningserver.utils.testing import RegistryFixture
 from testtools import ExpectedException
+from testtools.matchers import Equals
 
 
 class TestConfigureDHCP(MAASTestCase):
@@ -238,14 +239,14 @@
         ServiceRegistry.register_item(service.name, service)
         return service
 
-    def test__raises_exception_if_service_should_be_off(self):
+    def test__returns_false_if_service_should_be_off(self):
         service = self.make_dhcpv4_service()
         service.off()
         exception_type = factory.make_exception_type()
-        with ExpectedException(exception_type):
-            dhcp._ensure_dhcpv4_is_accessible(exception_type)
+        return_value = dhcp._is_dhcpv4_managed_and_active(exception_type)
+        self.assertThat(return_value, Equals(False))
 
-    def test__does_nothing_if_service_already_on(self):
+    def test__returns_true_if_service_already_on(self):
         service = self.make_dhcpv4_service()
         service.on()
         mock_get_state = self.patch(
@@ -253,8 +254,10 @@
         mock_get_state.return_value = SERVICE_STATE.ON
         mock_ensure_service = self.patch(
             dhcp.service_monitor, "ensure_service")
-        dhcp._ensure_dhcpv4_is_accessible(factory.make_exception_type())
+        return_value = dhcp._is_dhcpv4_managed_and_active(
+            factory.make_exception_type())
         self.assertThat(mock_ensure_service, MockNotCalled())
+        self.assertThat(return_value, Equals(True))
 
     def test__calls_try_connection_to_check_omshell(self):
         service = self.make_dhcpv4_service()
@@ -267,7 +270,7 @@
         mock_omshell = self.patch_autospec(dhcp, "Omshell")
         mock_try_connection = mock_omshell.return_value.try_connection
         mock_try_connection.return_value = True
-        dhcp._ensure_dhcpv4_is_accessible(factory.make_exception_type())
+        dhcp._is_dhcpv4_managed_and_active(factory.make_exception_type())
         self.assertThat(mock_ensure_service, MockCalledOnceWith("dhcp4"))
         self.assertThat(mock_try_connection, MockCalledOnceWith())
 
@@ -285,7 +288,7 @@
         mock_try_connection.return_value = False
         fake_exception_type = factory.make_exception_type()
         with ExpectedException(fake_exception_type):
-            dhcp._ensure_dhcpv4_is_accessible(fake_exception_type)
+            dhcp._is_dhcpv4_managed_and_active(fake_exception_type)
         self.assertThat(mock_ensure_service, MockCalledOnceWith("dhcp4"))
         self.assertEqual(mock_try_connection.call_count, 3)
 
@@ -300,7 +303,7 @@
         mock_ensure_service.side_effect = ServiceActionError()
         exception_type = factory.make_exception_type()
         with ExpectedException(exception_type):
-            dhcp._ensure_dhcpv4_is_accessible(exception_type)
+            dhcp._is_dhcpv4_managed_and_active(exception_type)
 
     def test__raises_exception_on_other_exceptions(self):
         service = self.make_dhcpv4_service()
@@ -313,22 +316,22 @@
         mock_ensure_service.side_effect = factory.make_exception()
         exception_type = factory.make_exception_type()
         with ExpectedException(exception_type):
-            dhcp._ensure_dhcpv4_is_accessible(exception_type)
+            dhcp._is_dhcpv4_managed_and_active(exception_type)
 
 
 class TestCreateHostMaps(MAASTestCase):
 
     def setUp(self):
         super(TestCreateHostMaps, self).setUp()
-        # Patch _ensure_dhcpv4_is_accessible.
-        self._ensure_dhcpv4_is_accessible = self.patch_autospec(
-            dhcp, "_ensure_dhcpv4_is_accessible")
+        # Patch _is_dhcpv4_managed_and_active.
+        self._is_dhcpv4_managed_and_active = self.patch_autospec(
+            dhcp, "_is_dhcpv4_managed_and_active")
 
-    def test_calls__ensure_dhcpv4_is_accessible(self):
+    def test_calls__is_dhcpv4_managed_and_active(self):
         self.patch(dhcp, "Omshell")
         dhcp.create_host_maps([], sentinel.shared_key)
         self.assertThat(
-            self._ensure_dhcpv4_is_accessible,
+            self._is_dhcpv4_managed_and_active,
             MockCalledOnceWith(CannotCreateHostMap))
 
     def test_creates_omshell(self):
@@ -381,15 +384,15 @@
         super(TestRemoveHostMaps, self).setUp()
         self.patch(Omshell, "remove")
         self.patch(Omshell, "nullify_lease")
-        # Patch _ensure_dhcpv4_is_accessible.
-        self._ensure_dhcpv4_is_accessible = self.patch_autospec(
-            dhcp, "_ensure_dhcpv4_is_accessible")
+        # Patch _is_dhcpv4_managed_and_active.
+        self._is_dhcpv4_managed_and_active = self.patch_autospec(
+            dhcp, "_is_dhcpv4_managed_and_active")
 
-    def test_calls__ensure_dhcpv4_is_accessible(self):
+    def test_calls__is_dhcpv4_managed_and_active(self):
         self.patch(dhcp, "Omshell")
         dhcp.remove_host_maps([], sentinel.shared_key)
         self.assertThat(
-            self._ensure_dhcpv4_is_accessible,
+            self._is_dhcpv4_managed_and_active,
             MockCalledOnceWith(CannotRemoveHostMap))
 
     def test_removes_omshell(self):
@@ -438,8 +441,8 @@
     def setUp(self):
         super(TestOmshellError, self).setUp()
         # Patch _ensure_dhcpv4_is_accessible.
-        self._ensure_dhcpv4_is_accessible = self.patch_autospec(
-            dhcp, "_ensure_dhcpv4_is_accessible")
+        self._is_dhcpv4_managed_and_active = self.patch_autospec(
+            dhcp, "_is_dhcpv4_managed_and_active")
         self.patch(ExternalProcessError, '__str__', lambda x: 'Error')
 
         def raise_ExternalProcessError(*args, **kwargs):