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
1=== modified file 'src/maasserver/dhcp.py'
2--- src/maasserver/dhcp.py 2014-02-24 09:30:57 +0000
3+++ src/maasserver/dhcp.py 2014-05-30 17:40:20 +0000
4@@ -22,6 +22,7 @@
5 from netaddr import IPAddress
6 from provisioningserver.tasks import (
7 restart_dhcp_server,
8+ stop_dhcp_server,
9 write_dhcp_config,
10 )
11
12@@ -52,7 +53,15 @@
13 from maasserver.dns import get_dns_server_address
14
15 interfaces = get_interfaces_managed_by(nodegroup)
16- if interfaces is None:
17+ if interfaces in [None, []]:
18+ # interfaces being None means the cluster isn't accepted: stop
19+ # the DHCP server in case it case started.
20+ # interfaces being [] means there is no interface configured: stop
21+ # the DHCP server; Note that a config generated with this setup
22+ # would not be valid and would result in the DHCP
23+ # server failing with the error: "Not configured to listen on any
24+ # interfaces!."
25+ stop_dhcp_server.apply_async(queue=nodegroup.work_queue)
26 return
27
28 # Make sure this nodegroup has a key to communicate with the dhcp
29
30=== modified file 'src/maasserver/tests/test_dhcp.py'
31--- src/maasserver/tests/test_dhcp.py 2014-02-24 09:30:57 +0000
32+++ src/maasserver/tests/test_dhcp.py 2014-05-30 17:40:20 +0000
33@@ -33,7 +33,6 @@
34 from maasserver.testing.testcase import MAASServerTestCase
35 from maasserver.utils import map_enum
36 from maastesting.celery import CeleryFixture
37-from mock import ANY
38 from netaddr import (
39 IPAddress,
40 IPNetwork,
41@@ -72,6 +71,16 @@
42 {status: None for status in unaccepted_statuses},
43 managed_interfaces)
44
45+ def test_configure_dhcp_stops_server_if_no_managed_interface(self):
46+ self.patch(settings, "DHCP_CONNECT", True)
47+ self.patch(dhcp, 'stop_dhcp_server')
48+ nodegroup = factory.make_node_group(
49+ status=NODEGROUP_STATUS.ACCEPTED,
50+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
51+ )
52+ configure_dhcp(nodegroup)
53+ self.assertEqual(1, dhcp.stop_dhcp_server.apply_async.call_count)
54+
55 def test_configure_dhcp_obeys_DHCP_CONNECT(self):
56 self.patch(settings, "DHCP_CONNECT", False)
57 self.patch(dhcp, 'write_dhcp_config')
58@@ -205,19 +214,6 @@
59 args, kwargs = task.subtask.call_args
60 self.assertEqual(nodegroup.work_queue, kwargs['options']['queue'])
61
62- def test_write_dhcp_config_called_when_no_managed_interfaces(self):
63- nodegroup = factory.make_node_group(
64- status=NODEGROUP_STATUS.ACCEPTED,
65- management=NODEGROUPINTERFACE_MANAGEMENT.DHCP)
66- [interface] = nodegroup.nodegroupinterface_set.all()
67- self.patch(settings, "DHCP_CONNECT", True)
68- self.patch(tasks, 'sudo_write_file')
69- self.patch(dhcp, 'write_dhcp_config')
70- interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
71- interface.save()
72- dhcp.write_dhcp_config.apply_async.assert_called_once_with(
73- queue=nodegroup.work_queue, kwargs=ANY)
74-
75 def test_dhcp_config_gets_written_when_interface_IP_changes(self):
76 nodegroup = factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
77 [interface] = nodegroup.nodegroupinterface_set.all()
78@@ -318,6 +314,9 @@
79 factory.make_node_group(status=NODEGROUP_STATUS.ACCEPTED)
80 for x in range(num_inactive_nodegroups):
81 factory.make_node_group(status=NODEGROUP_STATUS.PENDING)
82+ # Silence stop_dhcp_server: it will be called for the inactive
83+ # nodegroups.
84+ self.patch(dhcp, 'stop_dhcp_server')
85
86 self.patch(settings, "DHCP_CONNECT", True)
87 self.patch(dhcp, 'write_dhcp_config')
88
89=== modified file 'src/provisioningserver/tasks.py'
90--- src/provisioningserver/tasks.py 2014-05-26 16:26:54 +0000
91+++ src/provisioningserver/tasks.py 2014-05-30 17:40:20 +0000
92@@ -387,10 +387,18 @@
93 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'restart'])
94
95
96+# Message to put in the DHCP config file when the DHCP server gets stopped.
97+DISABLED_DHCP_SERVER = "# DHCP server stopped."
98+
99+
100 @task
101 @log_exception_text
102 def stop_dhcp_server():
103- """Stop a DHCP server."""
104+ """Write a blank config file and stop a DHCP server."""
105+ # Write an empty config file to avoid having an outdated config laying
106+ # around.
107+ sudo_write_file(
108+ celery_config.DHCP_CONFIG_FILE, DISABLED_DHCP_SERVER)
109 call_and_check(['sudo', '-n', 'service', 'maas-dhcp-server', 'stop'])
110
111
112
113=== modified file 'src/provisioningserver/tests/test_tasks.py'
114--- src/provisioningserver/tests/test_tasks.py 2014-05-26 16:49:43 +0000
115+++ src/provisioningserver/tests/test_tasks.py 2014-05-30 17:40:20 +0000
116@@ -332,11 +332,14 @@
117 self.assertThat(tasks.call_and_check, MockCalledOnceWith(
118 ['sudo', '-n', 'service', 'maas-dhcp-server', 'restart']))
119
120- def test_stop_dhcp_server_sends_command(self):
121+ def test_stop_dhcp_server_sends_command_and_writes_empty_config(self):
122 self.patch(tasks, 'call_and_check')
123+ self.patch(tasks, 'sudo_write_file')
124 stop_dhcp_server()
125 self.assertThat(tasks.call_and_check, MockCalledOnceWith(
126 ['sudo', '-n', 'service', 'maas-dhcp-server', 'stop']))
127+ self.assertThat(tasks.sudo_write_file, MockCalledOnceWith(
128+ celery_config.DHCP_CONFIG_FILE, tasks.DISABLED_DHCP_SERVER))
129
130
131 def assertTaskRetried(runner, result, nb_retries, task_name):