Merge lp:~rvb/maas/bug-1415538 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Do not generate the 'option routers' stanza if router IP is None.

The main problem here is that interface.router_ip is None when "unset".

[Christian Reis (kiko)]

On Wed, Jan 28, 2015 at 04:35:42PM -0000, Raphaël Badin wrote:
> === modified file 'src/maasserver/dhcp.py'
> --- src/maasserver/dhcp.py 2014-10-03 19:29:51 +0000
> +++ src/maasserver/dhcp.py 2015-01-28 16:35:10 +0000
> @@ -56,7 +56,7 @@
> 'subnet_cidr': unicode(interface.network.cidr),
> 'broadcast_ip': interface.broadcast_ip,
> 'interface': interface.interface,
> - 'router_ip': unicode(interface.router_ip),
> + 'router_ip': interface.router_ip,

Should subnet_cidr above now be unicode()d as well?
--
Christian Robottom Reis | [+1] 612 888 4935 | http://launchpad.net/~kiko
Canonical VP Hyperscale | [+55 16] 9 9112 6430

[Gavin Panella (allenap)]

Tip top.

review: Approve

[Raphaël Badin (rvb)]

> On Wed, Jan 28, 2015 at 04:35:42PM -0000, Raphaël Badin wrote:
> > === modified file 'src/maasserver/dhcp.py'
> > --- src/maasserver/dhcp.py 2014-10-03 19:29:51 +0000
> > +++ src/maasserver/dhcp.py 2015-01-28 16:35:10 +0000
> > @@ -56,7 +56,7 @@
> > 'subnet_cidr': unicode(interface.network.cidr),
> > 'broadcast_ip': interface.broadcast_ip,
> > 'interface': interface.interface,
> > - 'router_ip': unicode(interface.router_ip),
> > + 'router_ip': interface.router_ip,
>
> Should subnet_cidr above now be unicode()d as well?

We need to keep the unicode() in the expression unicode(interface.network.cidr) because the type interface.network.cidr is netaddr.IPNetwork and it needs to be converted to a string representation. A managed interface *cannot* have a None interface.network. (But it can have a None router_ip, hence the fix here).

[MAAS Lander (maas-lander)]

The attempt to merge lp:~rvb/maas/bug-1415538 into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
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:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Get:4 http://security.ubuntu.com trusty-security/main Sources [64.8 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:5 http://security.ubuntu.com trusty-security/universe Sources [17.4 kB]
Get:6 http://security.ubuntu.com trusty-security/main amd64 Packages [200 kB]
Get:7 http://nova.clouds.archive.ubuntu.com trusty-updates Release [62.0 kB]
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [85.3 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://security.ubuntu.com trusty-security/universe Translation-en
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
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [160 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [99.4 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [407 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [243 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
Fetched 1,402 kB in 2s (476 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-bson python-bzrlib 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-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-iscpy python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth python-openssl python-paramiko python-pexpect python-pip python-pocket-lint python-psycopg2 python-pyinotify python-pyparsing python-p...

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2014-10-03 19:29:51 +0000
+++ src/maasserver/dhcp.py	2015-01-30 07:57:17 +0000
@@ -56,7 +56,9 @@
         'subnet_cidr': unicode(interface.network.cidr),
         'broadcast_ip': interface.broadcast_ip,
         'interface': interface.interface,
-        'router_ip': unicode(interface.router_ip),
+        'router_ip': (
+            None if not interface.router_ip
+            else unicode(interface.router_ip)),
         'dns_servers': dns_servers,
         'ntp_server': ntp_server,
         'domain_name': interface.nodegroup.name,

=== modified file 'src/maasserver/tests/test_dhcp.py'
--- src/maasserver/tests/test_dhcp.py	2014-11-07 13:16:58 +0000
+++ src/maasserver/tests/test_dhcp.py	2015-01-30 07:57:17 +0000
@@ -200,6 +200,14 @@
         self.expectThat(
             config['ip_range_low'], Not(Equals(interface.static_ip_range_low)))
 
+    def test__doesnt_convert_None_router_ip(self):
+        interface = factory.make_NodeGroupInterface(factory.make_NodeGroup())
+        interface.router_ip = None
+        interface.save()
+        config = make_subnet_config(
+            interface, factory.make_name('dns'), factory.make_name('ntp'))
+        self.assertEqual(None, config['router_ip'])
+
 
 class TestDoConfigureDHCP(MAASServerTestCase):
     """Tests for `do_configure_dhcp`."""

=== modified file 'src/provisioningserver/dhcp/tests/test_config.py'
--- src/provisioningserver/dhcp/tests/test_config.py	2014-11-07 15:50:30 +0000
+++ src/provisioningserver/dhcp/tests/test_config.py	2015-01-30 07:57:17 +0000
@@ -147,7 +147,7 @@
             config.get_config('dhcpd.conf.template', **params),
             Contains(router_ip))
 
-    def test__renders_without_router_ip(self):
+    def test__renders_with_empty_string_router_ip(self):
         params = make_sample_params()
         params['dhcp_subnets'][0]['router_ip'] = ''
         template = self.patch_template()
@@ -157,6 +157,16 @@
             config.get_config('dhcpd.conf.template', **params))
         self.assertNotIn("routers", rendered)
 
+    def test__renders_with_None_router_ip(self):
+        params = make_sample_params()
+        params['dhcp_subnets'][0]['router_ip'] = None
+        template = self.patch_template()
+        rendered = template.substitute(params)
+        self.assertEqual(
+            rendered,
+            config.get_config('dhcpd.conf.template', **params))
+        self.assertNotIn("routers", rendered)
+
 
 class TestComposeConditionalBootloader(PservTestCase):
     """Tests for `compose_conditional_bootloader`."""