Merge lp:~rvb/maas/no-dhcp-write-of-unvalid-config into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Return early and do not generate an invalid (!) DHCP config in configure_dhcp() when the list of "managed" interfaces of the nodegroup is empty.

[Gavin Panella (allenap)]

What happens when the last interface is removed from a cluster, leaving it with no interfaces?

[Raphaël Badin (rvb)]

> What happens when the last interface is removed from a cluster, leaving it
> with no interfaces?

I see that this code has been recently modified by https://code.launchpad.net/~julian-edwards/maas/stop-dhcp-server-bug-1283114/+merge/207870. I think the behavior introduced by this branch is wrong: it created an invalid config file when the number of managed interfaces was down to zero.

I think the only sensible thing to do is to stop the DHCP server when the number of managed interfaces is zero. I see we already have a task for that so it's just a matter of hooking it up into configure_dhcp(). What do you think?

[Gavin Panella (allenap)]

> I think the only sensible thing to do is to stop the DHCP server when the
> number of managed interfaces is zero. I see we already have a task for that
> so it's just a matter of hooking it up into configure_dhcp(). What do you
> think?

Sounds sensible to me. I think it's also worth writing out an empty config (even if it is invalid) or removing the config file.

[Raphaël Badin (rvb)]

> > I think the only sensible thing to do is to stop the DHCP server when the
> > number of managed interfaces is zero. I see we already have a task for that
> > so it's just a matter of hooking it up into configure_dhcp(). What do you
> > think?
>
> Sounds sensible to me. I think it's also worth writing out an empty config
> (even if it is invalid) or removing the config file.

Done, please have another look.

[Gavin Panella (allenap)]

Looks good.

review: Approve

[Raphaël Badin (rvb)]

> Might be nice to write out an explanatory comment here.

There is one already…!?

[Raphaël Badin (rvb)]

> > Might be nice to write out an explanatory comment here.
>
> There is one already…!?

It says "Write an empty config file to avoid having an outdated config laying around.". Is there something you think is worth explaining?

[Gavin Panella (allenap)]

I mean into the config file.

[Raphaël Badin (rvb)]

> I mean into the config file.

Ah, right, good idea. Done.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/no-dhcp-write-of-unvalid-config into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [45.7 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [28.2 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [109 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [74.9 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 318 kB in 0s (1,306 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-net...

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/no-dhcp-write-of-unvalid-config into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [45.7 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [28.2 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [109 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [74.9 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 318 kB in 0s (1,181 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-net...

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2014-02-24 09:30:57 +0000
+++ src/maasserver/dhcp.py	2014-05-30 17:40:20 +0000
@@ -22,6 +22,7 @@
 from netaddr import IPAddress
 from provisioningserver.tasks import (
     restart_dhcp_server,
+    stop_dhcp_server,
     write_dhcp_config,
     )
 
@@ -52,7 +53,15 @@
     from maasserver.dns import get_dns_server_address
 
     interfaces = get_interfaces_managed_by(nodegroup)
-    if interfaces is None:
+    if interfaces in [None, []]:
+        # interfaces being None means the cluster isn't accepted: stop
+        # the DHCP server in case it case started.
+        # interfaces being [] means there is no interface configured: stop
+        # the DHCP server;  Note that a config generated with this setup
+        # would not be valid and would result in the DHCP
+        # server failing with the error: "Not configured to listen on any
+        # interfaces!."
+        stop_dhcp_server.apply_async(queue=nodegroup.work_queue)
         return
 
     # Make sure this nodegroup has a key to communicate with the dhcp

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2014-02-24 09:30:57 +0000
+++ src/maasserver/tests/test_dhcp.py	2014-05-30 17:40:20 +0000
@@ -33,7 +33,6 @@
 from maasserver.testing.testcase import MAASServerTestCase
 from maasserver.utils import map_enum
 from maastesting.celery import CeleryFixture
-from mock import ANY
 from netaddr import (
     IPAddress,
     IPNetwork,
@@ -72,6 +71,16 @@
             {status: None for status in unaccepted_statuses},
             managed_interfaces)
 
+    def test_configure_dhcp_stops_server_if_no_managed_interface(self):
+        self.patch(settings, "DHCP_CONNECT", True)
+        self.patch(dhcp, 'stop_dhcp_server')
+        nodegroup = factory.make_node_group(
+            status=NODEGROUP_STATUS.ACCEPTED,
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
+            )
+        configure_dhcp(nodegroup)
+        self.assertEqual(1, dhcp.stop_dhcp_server.apply_async.call_count)
+
     def test_configure_dhcp_obeys_DHCP_CONNECT(self):
         self.patch(settings, "DHCP_CONNECT", False)
         self.patch(dhcp, 'write_dhcp_config')
@@ -205,19 +214,6 @@
         args, kwargs = task.subtask.call_args
         self.assertEqual(nodegroup.work_queue, kwargs['options']['queue'])
 
-    def test_write_dhcp_config_called_when_no_managed_interfaces(self):
-        nodegroup = factory.make_node_group(
-            status=NODEGROUP_STATUS.ACCEPTED,
-            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
-        [interface] = nodegroup.nodegroupinterface_set.all()
-        self.patch(settings, "DHCP_CONNECT", True)
-        self.patch(tasks, 'sudo_write_file')
-        self.patch(dhcp, 'write_dhcp_config')
-        interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
-        interface.save()
-        dhcp.write_dhcp_config.apply_async.assert_called_once_with(
-            queue=nodegroup.work_queue, kwargs=ANY)
-
     def test_dhcp_config_gets_written_when_interface_IP_changes(self):
         nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
         [interface] = nodegroup.nodegroupinterface_set.all()
@@ -318,6 +314,9 @@
             factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
         for x in range(num_inactive_nodegroups):
             factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
+        # Silence stop_dhcp_server: it will be called for the inactive
+        # nodegroups.
+        self.patch(dhcp, 'stop_dhcp_server')
 
         self.patch(settings, "DHCP_CONNECT", True)
         self.patch(dhcp, 'write_dhcp_config')

=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py	2014-05-26 16:26:54 +0000
+++ src/provisioningserver/tasks.py	2014-05-30 17:40:20 +0000
@@ -387,10 +387,18 @@
     call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
 
 
+# Message to put in the DHCP config file when the DHCP server gets stopped.
+DISABLED_DHCP_SERVER = "# DHCP server stopped."
+
+
 @task
 @log_exception_text
 def stop_dhcp_server():
-    """Stop a DHCP server."""
+    """Write a blank config file and stop a DHCP server."""
+    # Write an empty config file to avoid having an outdated config laying
+    # around.
+    sudo_write_file(
+        celery_config.DHCP_CONFIG_FILE, DISABLED_DHCP_SERVER)
     call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'])
 
 

=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py	2014-05-26 16:49:43 +0000
+++ src/provisioningserver/tests/test_tasks.py	2014-05-30 17:40:20 +0000
@@ -332,11 +332,14 @@
         self.assertThat(tasks.call_and_check, MockCalledOnceWith(
             ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart']))
 
-    def test_stop_dhcp_server_sends_command(self):
+    def test_stop_dhcp_server_sends_command_and_writes_empty_config(self):
         self.patch(tasks, 'call_and_check')
+        self.patch(tasks, 'sudo_write_file')
         stop_dhcp_server()
         self.assertThat(tasks.call_and_check, MockCalledOnceWith(
             ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop']))
+        self.assertThat(tasks.sudo_write_file, MockCalledOnceWith(
+            celery_config.DHCP_CONFIG_FILE, tasks.DISABLED_DHCP_SERVER))
 
 
 def assertTaskRetried(runner, result, nb_retries, task_name):