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

Proposed by Raphaël Badin on 2014-05-30
Status: Merged
Approved by: Julian Edwards on 2014-06-02
Approved revision: 2392
Merged at revision: 2389
Proposed branch: lp:~rvb/maas/no-dhcp-write-of-unvalid-config
Merge into: lp:maas/trunk
Diff against target: 131 lines (+36/-17)
4 files modified
src/maasserver/dhcp.py (+10/-1)
src/maasserver/tests/test_dhcp.py (+13/-14)
src/provisioningserver/tasks.py (+9/-1)
src/provisioningserver/tests/test_tasks.py (+4/-1)
To merge this branch: bzr merge lp:~rvb/maas/no-dhcp-write-of-unvalid-config
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2014-05-30 Approve on 2014-05-30
Review via email: mp+221558@code.launchpad.net

Commit message

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

To post a comment you must log in.
Gavin Panella (allenap) wrote :

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

Raphaël Badin (rvb) wrote :

> 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) wrote :

> 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) wrote :

> > 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) wrote :

Looks good.

review: Approve
Raphaël Badin (rvb) wrote :

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

There is one already…!?

Raphaël Badin (rvb) wrote :

> > 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) wrote :

I mean into the config file.

Raphaël Badin (rvb) wrote :

> I mean into the config file.

Ah, right, good idea. Done.

MAAS Lander (maas-lander) wrote :
Download full text (21.9 KiB)

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) wrote :
Download full text (39.2 KiB)

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...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
22from netaddr import IPAddress22from netaddr import IPAddress
23from provisioningserver.tasks import (23from provisioningserver.tasks import (
24 restart_dhcp_server,24 restart_dhcp_server,
25 stop_dhcp_server,
25 write_dhcp_config,26 write_dhcp_config,
26 )27 )
2728
@@ -52,7 +53,15 @@
52 from maasserver.dns import get_dns_server_address53 from maasserver.dns import get_dns_server_address
5354
54 interfaces = get_interfaces_managed_by(nodegroup)55 interfaces = get_interfaces_managed_by(nodegroup)
55 if interfaces is None:56 if interfaces in [None, []]:
57 # interfaces being None means the cluster isn't accepted: stop
58 # the DHCP server in case it case started.
59 # interfaces being [] means there is no interface configured: stop
60 # the DHCP server; Note that a config generated with this setup
61 # would not be valid and would result in the DHCP
62 # server failing with the error: "Not configured to listen on any
63 # interfaces!."
64 stop_dhcp_server.apply_async(queue=nodegroup.work_queue)
56 return65 return
5766
58 # Make sure this nodegroup has a key to communicate with the dhcp67 # Make sure this nodegroup has a key to communicate with the dhcp
5968
=== 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 @@
33from maasserver.testing.testcase import MAASServerTestCase33from maasserver.testing.testcase import MAASServerTestCase
34from maasserver.utils import map_enum34from maasserver.utils import map_enum
35from maastesting.celery import CeleryFixture35from maastesting.celery import CeleryFixture
36from mock import ANY
37from netaddr import (36from netaddr import (
38 IPAddress,37 IPAddress,
39 IPNetwork,38 IPNetwork,
@@ -72,6 +71,16 @@
72 {status: None for status in unaccepted_statuses},71 {status: None for status in unaccepted_statuses},
73 managed_interfaces)72 managed_interfaces)
7473
74 def test_configure_dhcp_stops_server_if_no_managed_interface(self):
75 self.patch(settings, "DHCP_CONNECT", True)
76 self.patch(dhcp, 'stop_dhcp_server')
77 nodegroup = factory.make_node_group(
78 status=NODEGROUP_STATUS.ACCEPTED,
79 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
80 )
81 configure_dhcp(nodegroup)
82 self.assertEqual(1, dhcp.stop_dhcp_server.apply_async.call_count)
83
75 def test_configure_dhcp_obeys_DHCP_CONNECT(self):84 def test_configure_dhcp_obeys_DHCP_CONNECT(self):
76 self.patch(settings, "DHCP_CONNECT", False)85 self.patch(settings, "DHCP_CONNECT", False)
77 self.patch(dhcp, 'write_dhcp_config')86 self.patch(dhcp, 'write_dhcp_config')
@@ -205,19 +214,6 @@
205 args, kwargs = task.subtask.call_args214 args, kwargs = task.subtask.call_args
206 self.assertEqual(nodegroup.work_queue, kwargs['options']['queue'])215 self.assertEqual(nodegroup.work_queue, kwargs['options']['queue'])
207216
208 def test_write_dhcp_config_called_when_no_managed_interfaces(self):
209 nodegroup = factory.make_node_group(
210 status=NODEGROUP_STATUS.ACCEPTED,
211 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
212 [interface] = nodegroup.nodegroupinterface_set.all()
213 self.patch(settings, "DHCP_CONNECT", True)
214 self.patch(tasks, 'sudo_write_file')
215 self.patch(dhcp, 'write_dhcp_config')
216 interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
217 interface.save()
218 dhcp.write_dhcp_config.apply_async.assert_called_once_with(
219 queue=nodegroup.work_queue, kwargs=ANY)
220
221 def test_dhcp_config_gets_written_when_interface_IP_changes(self):217 def test_dhcp_config_gets_written_when_interface_IP_changes(self):
222 nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)218 nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
223 [interface] = nodegroup.nodegroupinterface_set.all()219 [interface] = nodegroup.nodegroupinterface_set.all()
@@ -318,6 +314,9 @@
318 factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)314 factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
319 for x in range(num_inactive_nodegroups):315 for x in range(num_inactive_nodegroups):
320 factory.make_node_group(status=NODEGROUP_STATUS.PENDING)316 factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
317 # Silence stop_dhcp_server: it will be called for the inactive
318 # nodegroups.
319 self.patch(dhcp, 'stop_dhcp_server')
321320
322 self.patch(settings, "DHCP_CONNECT", True)321 self.patch(settings, "DHCP_CONNECT", True)
323 self.patch(dhcp, 'write_dhcp_config')322 self.patch(dhcp, 'write_dhcp_config')
324323
=== 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 @@
387 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])387 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
388388
389389
390# Message to put in the DHCP config file when the DHCP server gets stopped.
391DISABLED_DHCP_SERVER = "# DHCP server stopped."
392
393
390@task394@task
391@log_exception_text395@log_exception_text
392def stop_dhcp_server():396def stop_dhcp_server():
393 """Stop a DHCP server."""397 """Write a blank config file and stop a DHCP server."""
398 # Write an empty config file to avoid having an outdated config laying
399 # around.
400 sudo_write_file(
401 celery_config.DHCP_CONFIG_FILE, DISABLED_DHCP_SERVER)
394 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'])402 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'])
395403
396404
397405
=== 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 @@
332 self.assertThat(tasks.call_and_check, MockCalledOnceWith(332 self.assertThat(tasks.call_and_check, MockCalledOnceWith(
333 ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart']))333 ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart']))
334334
335 def test_stop_dhcp_server_sends_command(self):335 def test_stop_dhcp_server_sends_command_and_writes_empty_config(self):
336 self.patch(tasks, 'call_and_check')336 self.patch(tasks, 'call_and_check')
337 self.patch(tasks, 'sudo_write_file')
337 stop_dhcp_server()338 stop_dhcp_server()
338 self.assertThat(tasks.call_and_check, MockCalledOnceWith(339 self.assertThat(tasks.call_and_check, MockCalledOnceWith(
339 ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop']))340 ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop']))
341 self.assertThat(tasks.sudo_write_file, MockCalledOnceWith(
342 celery_config.DHCP_CONFIG_FILE, tasks.DISABLED_DHCP_SERVER))
340343
341344
342def assertTaskRetried(runner, result, nb_retries, task_name):345def assertTaskRetried(runner, result, nb_retries, task_name):