Merge lp:~jtv/maas/control-dhcpd into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Extract functions for stopping/restarting dhcpd into their own module. We'll be using them in additional places soon.

This was Blake's suggestion in review of my previous branch. It makes things a bit cleaner and also, easier to test.

Jeroen

[Julian Edwards (julian-edwards)]

I'm not sure I like the plethora of restart/start/stop methods. They are only one liners anyway and considerably bloat the code with the extra docstrings and testing.

Given you're doing this layering, it indicates that the underlying "issue_service_command" is so poorly interfaced that you need to add another wrapper to make it palatable.

What's wrong with just:

call_service_command(ip_version=4, "start") etc? You can even make the subcommands consts or enums so you get:
call_service_command(IPV4, START)

which reads *much* nicer to my eyes and has considerably less code to test and maintain.

[Julian Edwards (julian-edwards)]

You can add custom test matchers or methods now, for example

self.assertIPV4Started() which just test that call_service_command was called with the right params.

[Jeroen T. Vermeulen (jtv)]

> I'm not sure I like the plethora of restart/start/stop methods. They are only
> one liners anyway and considerably bloat the code with the extra docstrings
> and testing.

They don't bloat the code you'll normally be looking at though — only their own module, which is simple enough to figure out from the function names so you probably won't need to read it.

> Given you're doing this layering, it indicates that the underlying
> "issue_service_command" is so poorly interfaced that you need to add another
> wrapper to make it palatable.

Well, it's the other way around: the simple functions were what I wanted, and the internal helper just extracts some commonality between them.

> What's wrong with just:
>
> call_service_command(ip_version=4, "start") etc? You can even make the
> subcommands consts or enums so you get:
> call_service_command(IPV4, START)

Perhaps nothing wrong with it (apart from the obvious syntax error), but there are a few things about it I don't like. For one, the caller always knows statically whether they want to start the DHCPv4 daemon or stop the DHCPv6 daemon etc. Those things are very similar _in implementation_, but that's of no interest to the caller. In my experience turning the essentials of what the caller wants into parameters for Swiss-army-knife functions invites needless complication in itself.

For another, I wouldn't mind creating an enum for the two IP versions, but also introducing a single-purpose enum for “what do you want the dhcp service to do” to me completely negates the savings from having a single function. (Add a bit of extra confusion on top of that if the enum lives in the conventional ‘enum’ module.) Passing the parameters also weighs down tests that make use of the function: they then ought to cover not only whether the right function gets called, but also whether it gets called in the right way. Which means either verifying the parameters or integration-test the actual call.

> which reads *much* nicer to my eyes and has considerably less code to test and
> maintain.

Not “considerably less.” Four lines in total, which then effectively move into the calling code and need to be tested there, more spread out, with more specific setup, and ultimately in more places.

[Julian Edwards (julian-edwards)]

 review: approve

On Monday 25 August 2014 05:00:54 you wrote:
> They don't bloat the code you'll normally be looking at though — only their
> own module, which is simple enough to figure out from the function names so
> you probably won't need to read it.

Does this mean we can bloat any code that we don't happen to be looking at at
the particular time?

> > Given you're doing this layering, it indicates that the underlying
> > "issue_service_command" is so poorly interfaced that you need to add
> > another wrapper to make it palatable.
>
> Well, it's the other way around: the simple functions were what I wanted,
> and the internal helper just extracts some commonality between them.

OK.

> > What's wrong with just:
> >
> > call_service_command(ip_version=4, "start") etc? You can even make the
> > subcommands consts or enums so you get:
> > call_service_command(IPV4, START)
>
> Perhaps nothing wrong with it (apart from the obvious syntax error), but

It's psuedo-code ;)

> there are a few things about it I don't like. For one, the caller always
> knows statically whether they want to start the DHCPv4 daemon or stop the
> DHCPv6 daemon etc. Those things are very similar _in implementation_, but
> that's of no interest to the caller. In my experience turning the
> essentials of what the caller wants into parameters for Swiss-army-knife
> functions invites needless complication in itself.
>
> For another, I wouldn't mind creating an enum for the two IP versions, but
> also introducing a single-purpose enum for “what do you want the dhcp
> service to do” to me completely negates the savings from having a single
> function. (Add a bit of extra confusion on top of that if the enum lives
> in the conventional ‘enum’ module.) Passing the parameters also weighs
> down tests that make use of the function: they then ought to cover not only
> whether the right function gets called, but also whether it gets called in
> the right way. Which means either verifying the parameters or
> integration-test the actual call.

It's a shame you don't like using classes more, because a class property would
have been ideal. I also find that they nicely group functions like this
without exploding the number of files you have with separate modules.

> > which reads *much* nicer to my eyes and has considerably less code to test
> > and maintain.
>
> Not “considerably less.” Four lines in total, which then effectively move
> into the calling code and need to be tested there, more spread out, with
> more specific setup, and ultimately in more places.

The 4 lines go along with the repetitive docstrings as well, you can't
refactor those (well, maybe you can).

Anyway I don't want to block you so I'll approve this. I just wanted to
express some feelings about the code.

review: Approve

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2014-08-18 10:53:21 +0000
+++ src/maasserver/tests/test_dhcp.py	2014-08-25 04:26:47 +0000
@@ -32,6 +32,7 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.celery import CeleryFixture
+from maastesting.matchers import MockCalledOnceWith
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -172,15 +173,13 @@
 
     def test_configure_dhcp_restarts_dhcp_server(self):
         self.patch(tasks, "sudo_write_file")
-        mocked_check_call = self.patch(tasks, "call_and_check")
+        restart_dhcpv4 = self.patch(tasks, "restart_dhcpv4")
         self.patch(settings, "DHCP_CONNECT", True)
         nodegroup = factory.make_node_group(
             status=NODEGROUP_STATUS.ACCEPTED,
             management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
         configure_dhcp(nodegroup)
-        self.assertEqual(
-            mocked_check_call.call_args[0][0],
-            ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
+        self.assertThat(restart_dhcpv4, MockCalledOnceWith())
 
     def test_configure_dhcp_is_called_with_valid_dhcp_key(self):
         self.patch(dhcp, 'write_dhcp_config')

=== added file 'src/provisioningserver/dhcp/control.py'
--- src/provisioningserver/dhcp/control.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/control.py	2014-08-25 04:26:47 +0000
@@ -0,0 +1,79 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Control the MAAS DHCP daemons."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    'restart_dhcpv4',
+    'restart_dhcpv6',
+    'stop_dhcpv4',
+    'stop_dhcpv6',
+    ]
+
+from provisioningserver.utils.shell import call_and_check
+
+
+def call_service_script(ip_version, subcommand):
+    """Issue a subcommand to one of the DHCP daemon services.
+
+    Shells out using `sudo`, and with the `C` locale.
+
+    :raise ExternalProcessError: if the restart command fails.
+    """
+    service_names = {
+        4: 'maas-dhcp-server',
+        6: 'maas-dhcpv6-server',
+        }
+    name = service_names[ip_version]
+    call_and_check(
+        ['sudo', '-n', 'service', name, subcommand],
+        env={'LC_ALL': 'C'})
+
+
+def restart_dhcpv4():
+    """Restart the (IPv4) DHCP daemon.
+
+    Shells out using `sudo`, and with the `C` locale.
+
+    :raise ExternalProcessError: if the restart command fails.
+    """
+    call_service_script(4, 'restart')
+
+
+def restart_dhcpv6():
+    """Restart the DHCPv6 daemon.
+
+    Shells out using `sudo`, and with the `C` locale.
+
+    :raise ExternalProcessError: if the restart command fails.
+    """
+    call_service_script(6, 'restart')
+
+
+def stop_dhcpv4():
+    """Stop the (IPv4) DHCP daemon.
+
+    Shells out using `sudo`, and with the `C` locale.
+
+    :raise ExternalProcessError: if the restart command fails.
+    """
+    call_service_script(4, 'stop')
+
+
+def stop_dhcpv6():
+    """Stop the DHCPv6 daemon.
+
+    Shells out using `sudo`, and with the `C` locale.
+
+    :raise ExternalProcessError: if the restart command fails.
+    """
+    call_service_script(6, 'stop')

=== added file 'src/provisioningserver/dhcp/tests/test_control.py'
--- src/provisioningserver/dhcp/tests/test_control.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/dhcp/tests/test_control.py	2014-08-25 04:26:47 +0000
@@ -0,0 +1,103 @@
+# Copyright 2014 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for DHCP control functions."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = []
+
+from maastesting.matchers import MockCalledOnceWith
+from maastesting.testcase import MAASTestCase
+from mock import ANY
+from provisioningserver.dhcp import control
+from provisioningserver.dhcp.control import (
+    call_service_script,
+    restart_dhcpv4,
+    restart_dhcpv6,
+    stop_dhcpv4,
+    stop_dhcpv6,
+    )
+
+
+def patch_call_and_check(testcase):
+    """Patch `call_and_check`."""
+    return testcase.patch(control, 'call_and_check')
+
+
+class TestCallServiceScript(MAASTestCase):
+    """Tests for `call_service_script`."""
+
+    def test__fails_on_weird_IP_version(self):
+        self.assertRaises(KeyError, call_service_script, 5, 'restart')
+
+    def test__controls_maas_dhcp_server_for_IP_version_4(self):
+        call_and_check = patch_call_and_check(self)
+        call_service_script(4, 'stop')
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'],
+                env=ANY))
+
+    def test__controls_maas_dhcpv6_server_for_IP_version_6(self):
+        call_and_check = patch_call_and_check(self)
+        call_service_script(6, 'stop')
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcpv6-server', 'stop'],
+                env=ANY))
+
+    def test__sets_C_locale(self):
+        call_and_check = patch_call_and_check(self)
+        call_service_script(4, 'stop')
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(ANY, env={'LC_ALL': 'C'}))
+
+
+class TestControl(MAASTestCase):
+
+    def test_restart_dhcpv4(self):
+        call_and_check = patch_call_and_check(self)
+        restart_dhcpv4()
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'],
+                env={'LC_ALL': 'C'}))
+
+    def test_restart_dhcpv6(self):
+        call_and_check = patch_call_and_check(self)
+        restart_dhcpv6()
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcpv6-server', 'restart'],
+                env={'LC_ALL': 'C'}))
+
+    def test_stop_dhcpv4(self):
+        call_and_check = patch_call_and_check(self)
+        stop_dhcpv4()
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'],
+                env={'LC_ALL': 'C'}))
+
+    def test_stop_dhcpv6(self):
+        call_and_check = patch_call_and_check(self)
+        stop_dhcpv6()
+        self.assertThat(
+            call_and_check,
+            MockCalledOnceWith(
+                ['sudo', '-n', 'service', 'maas-dhcpv6-server', 'stop'],
+                env={'LC_ALL': 'C'}))

=== modified file 'src/provisioningserver/rpc/dhcp.py'
--- src/provisioningserver/rpc/dhcp.py	2014-08-25 02:20:15 +0000
+++ src/provisioningserver/rpc/dhcp.py	2014-08-25 04:26:47 +0000
@@ -20,6 +20,7 @@
 
 from celery.app import app_or_default
 from provisioningserver.dhcp.config import get_config
+from provisioningserver.dhcp.control import restart_dhcpv6
 from provisioningserver.logger import get_maas_logger
 from provisioningserver.omshell import Omshell
 from provisioningserver.rpc.exceptions import (
@@ -28,10 +29,7 @@
     CannotRemoveHostMap,
     )
 from provisioningserver.utils.fs import sudo_write_file
-from provisioningserver.utils.shell import (
-    call_and_check,
-    ExternalProcessError,
-    )
+from provisioningserver.utils.shell import ExternalProcessError
 
 
 maaslog = get_maas_logger("dhcp")
@@ -66,8 +64,7 @@
             % e.output_as_unicode)
 
     try:
-        call_and_check(
-            ['sudo', '-n', 'service', 'maas-dhcpv6-server', 'restart'])
+        restart_dhcpv6()
     except ExternalProcessError as e:
         maaslog.error(
             "DHCPv6 server failed to restart (for network interfaces %s): %s",

=== modified file 'src/provisioningserver/rpc/tests/test_dhcp.py'
--- src/provisioningserver/rpc/tests/test_dhcp.py	2014-08-22 11:23:35 +0000
+++ src/provisioningserver/rpc/tests/test_dhcp.py	2014-08-25 04:26:47 +0000
@@ -42,15 +42,15 @@
     def patch_sudo_write_file(self):
         return self.patch(dhcp, 'sudo_write_file')
 
-    def patch_call_and_check(self):
-        return self.patch(dhcp, 'call_and_check')
+    def patch_restart_dhcpv6(self):
+        return self.patch(dhcp, 'restart_dhcpv6')
 
     def patch_get_config(self):
         return self.patch(dhcp, 'get_config')
 
     def test__extracts_interfaces(self):
         write_file = self.patch_sudo_write_file()
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
         subnets = [make_subnet_config() for _ in range(3)]
         dhcp.configure_dhcpv6(factory.make_name('key'), subnets)
         self.assertThat(
@@ -61,7 +61,7 @@
 
     def test__eliminates_duplicate_interfaces(self):
         write_file = self.patch_sudo_write_file()
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
         interface = factory.make_name('interface')
         subnets = [make_subnet_config() for _ in range(2)]
         for subnet in subnets:
@@ -71,7 +71,7 @@
 
     def test__composees_dhcpv6_config(self):
         self.patch_sudo_write_file()
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
         get_config = self.patch_get_config()
         omapi_key = factory.make_name('key')
         subnet = make_subnet_config()
@@ -84,7 +84,7 @@
 
     def test__writes_dhcpv6_config(self):
         write_file = self.patch_sudo_write_file()
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
 
         subnet = make_subnet_config()
         expected_config = factory.make_name('config')
@@ -99,7 +99,7 @@
 
     def test__writes_interfaces_file(self):
         write_file = self.patch_sudo_write_file()
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
         dhcp.configure_dhcpv6(factory.make_name('key'), [make_subnet_config()])
         self.assertThat(
             write_file,
@@ -107,17 +107,14 @@
 
     def test__restarts_dhcpv6_server(self):
         self.patch_sudo_write_file()
-        call_and_check = self.patch_call_and_check()
+        call_and_check = self.patch_restart_dhcpv6()
         dhcp.configure_dhcpv6(factory.make_name('key'), [make_subnet_config()])
-        self.assertThat(
-            call_and_check,
-            MockCalledWith(
-                ['sudo', '-n', 'service', 'maas-dhcpv6-server', 'restart']))
+        self.assertThat(call_and_check, MockCalledWith())
 
     def test__converts_failure_writing_file_to_CannotConfigureDHCP(self):
         self.patch_sudo_write_file().side_effect = (
             ExternalProcessError(1, "sudo something"))
-        self.patch_call_and_check()
+        self.patch_restart_dhcpv6()
         self.assertRaises(
             exceptions.CannotConfigureDHCP,
             dhcp.configure_dhcpv6,
@@ -125,7 +122,7 @@
 
     def test__converts_dhcp_restart_failure_to_CannotConfigureDHCP(self):
         self.patch_sudo_write_file()
-        self.patch_call_and_check().side_effect = (
+        self.patch_restart_dhcpv6().side_effect = (
             ExternalProcessError(1, "sudo something"))
         self.assertRaises(
             exceptions.CannotConfigureDHCP,

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-08-21 13:27:59 +0000
+++ src/provisioningserver/tasks.py	2014-08-25 04:26:47 +0000
@@ -47,6 +47,10 @@
     config,
     detect,
     )
+from provisioningserver.dhcp.control import (
+    restart_dhcpv4,
+    stop_dhcpv4,
+    )
 from provisioningserver.dhcp.leases import upload_leases
 from provisioningserver.dns.config import (
     DNSConfig,
@@ -68,10 +72,7 @@
 from provisioningserver.utils.env import environment_variables
 from provisioningserver.utils.fs import sudo_write_file
 from provisioningserver.utils.network import find_ip_via_arp
-from provisioningserver.utils.shell import (
-    call_and_check,
-    ExternalProcessError,
-    )
+from provisioningserver.utils.shell import ExternalProcessError
 
 # For each item passed to refresh_secrets, a refresh function to give it to.
 refresh_functions = {
@@ -407,7 +408,7 @@
 @log_exception_text
 def restart_dhcp_server():
     """Restart the DHCP server."""
-    call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
+    restart_dhcpv4()
 
 
 # Message to put in the DHCP config file when the DHCP server gets stopped.
@@ -432,12 +433,9 @@
     sudo_write_file(
         celery_config.DHCP_CONFIG_FILE, DISABLED_DHCP_SERVER)
     try:
-        # Use LC_ALL=C because we use the returned error message when
-        # errors occur.
-        call_and_check(
-            ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'],
-            env={'LC_ALL': 'C'}
-        )
+        # This relies on stop_dhcpv4 running in the "C" locale: if it fails,
+        # we need to parse its error output.
+        stop_dhcpv4()
     except ExternalProcessError as error:
         # Upstart issues an "Unknown instance"-type error when trying to
         # stop an already-stopped service.  Ignore this error.

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-08-22 10:03:18 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-08-25 04:26:47 +0000
@@ -284,18 +284,15 @@
             stdin=PIPE))
 
     def test_restart_dhcp_server_sends_command(self):
-        self.patch(tasks, 'call_and_check')
+        self.patch(tasks, 'restart_dhcpv4')
         restart_dhcp_server()
-        self.assertThat(tasks.call_and_check, MockCalledOnceWith(
-            ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart']))
+        self.assertThat(tasks.restart_dhcpv4, MockCalledOnceWith())
 
     def test_stop_dhcp_server_sends_command_and_writes_empty_config(self):
-        self.patch(tasks, 'call_and_check')
+        self.patch(tasks, 'stop_dhcpv4')
         self.patch(tasks, 'sudo_write_file')
         stop_dhcp_server()
-        self.assertThat(tasks.call_and_check, MockCalledOnceWith(
-            ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'],
-            env={'LC_ALL': 'C'}))
+        self.assertThat(tasks.stop_dhcpv4, MockCalledOnceWith())
         self.assertThat(tasks.sudo_write_file, MockCalledOnceWith(
             celery_config.DHCP_CONFIG_FILE, tasks.DISABLED_DHCP_SERVER))
 
@@ -305,7 +302,7 @@
         output = ' ' + ALREADY_STOPPED_MESSAGE + '\n'
         exception = ExternalProcessError(
             ALREADY_STOPPED_RETURNCODE, [], output=output)
-        self.patch(tasks, 'call_and_check', Mock(side_effect=exception))
+        self.patch(tasks, 'stop_dhcpv4', Mock(side_effect=exception))
         self.patch(tasks, 'sudo_write_file')
         self.assertIsNone(stop_dhcp_server())
 
@@ -314,7 +311,7 @@
         returncode = ALREADY_STOPPED_RETURNCODE + 1
         exception = ExternalProcessError(
             returncode, [], output=ALREADY_STOPPED_MESSAGE)
-        self.patch(tasks, 'call_and_check', Mock(side_effect=exception))
+        self.patch(tasks, 'stop_dhcpv4', Mock(side_effect=exception))
         self.patch(tasks, 'sudo_write_file')
         self.assertRaises(ExternalProcessError, stop_dhcp_server)
 
@@ -323,7 +320,7 @@
         output = factory.make_string()
         exception = ExternalProcessError(
             ALREADY_STOPPED_RETURNCODE, [], output=output)
-        self.patch(tasks, 'call_and_check', Mock(side_effect=exception))
+        self.patch(tasks, 'stop_dhcpv4', Mock(side_effect=exception))
         self.patch(tasks, 'sudo_write_file')
         self.assertRaises(ExternalProcessError, stop_dhcp_server)