launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/subnet-calc2 into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1045589 in MAAS: "Subnet number in dhcp config is calculated incorrectly"
  https://bugs.launchpad.net/maas/+bug/1045589

For more details, see:
https://code.launchpad.net/~rvb/maas/subnet-calc2/+merge/122702

This branch fixes the subnet used when writing the DHCP configuration file.  The value used was the nodegroup's ip_range_low value and that is obviously wrong as the nodegroup's subnet_mask needs to be applied.

= Pre-imp =

No real pre-imp for this but the fix is rather straightforward.

= Notes =

Instead of fiddling with IP addresses manually, I use the netaddr module to do the heavy lifting.

I thought it was clearer to fix the network config of the nodegroup used in the test.  The alternative would have been to continue using a random one but in this case I would have to redo the computation of the subnet in the test the same way it's done in the code and the test would not have been very interesting.
-- 
https://code.launchpad.net/~rvb/maas/subnet-calc2/+merge/122702
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/subnet-calc2 into lp:maas.

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2012-09-04 04:47:28 +0000
+++ src/maasserver/models/nodegroup.py	2012-09-04 15:07:22 +0000
@@ -25,6 +25,7 @@
 from maasserver.dhcp import is_dhcp_management_enabled
 from maasserver.models.timestampedmodel import TimestampedModel
 from maasserver.refresh_worker import refresh_worker
+from netaddr import IPAddress
 from piston.models import (
     KEY_SIZE,
     Token,
@@ -148,11 +149,10 @@
         """Write the DHCP configuration file and restart the DHCP server."""
         # Circular imports.
         from maasserver.dns import get_dns_server_address
-        # XXX bug=1045589
-        # subnet is calculated incorrectly, see the bug.
+        subnet = str(
+            IPAddress(self.ip_range_low) & IPAddress(self.subnet_mask))
         write_dhcp_config.delay(
-            subnet=self.ip_range_low,
-            next_server=self.worker_ip,
+            subnet=subnet, next_server=self.worker_ip,
             omapi_key=self.dhcp_key, subnet_mask=self.subnet_mask,
             broadcast_ip=self.broadcast_ip, router_ip=self.router_ip,
             dns_servers=get_dns_server_address(),

=== modified file 'src/maasserver/tests/test_commands_config_master_dhcp.py'
--- src/maasserver/tests/test_commands_config_master_dhcp.py	2012-09-04 10:18:29 +0000
+++ src/maasserver/tests/test_commands_config_master_dhcp.py	2012-09-04 15:07:22 +0000
@@ -22,8 +22,6 @@
     )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
-from mock import Mock
-from provisioningserver import tasks
 from testtools.matchers import MatchesStructure
 
 

=== modified file 'src/maasserver/tests/test_nodegroup.py'
--- src/maasserver/tests/test_nodegroup.py	2012-09-04 04:47:28 +0000
+++ src/maasserver/tests/test_nodegroup.py	2012-09-04 15:07:22 +0000
@@ -193,7 +193,9 @@
         mocked_task = self.patch(
             maasserver.models.nodegroup, 'write_dhcp_config')
         nodegroup = factory.make_node_group(
-            dhcp_key=factory.getRandomString())
+            dhcp_key=factory.getRandomString(),
+            ip_range_low='192.168.102.1', ip_range_high='192.168.103.254',
+            subnet_mask='255.255.252.0', broadcast_ip='192.168.103.255')
         nodegroup.set_up_dhcp()
         dhcp_params = [
             'subnet_mask', 'broadcast_ip', 'router_ip',
@@ -204,9 +206,7 @@
         expected_params["next_server"] = nodegroup.worker_ip
         expected_params["omapi_key"] = nodegroup.dhcp_key
         expected_params["dns_servers"] = get_dns_server_address()
-        # XXX bug=1045589
-        # subnet is calculated incorrectly, see the bug.
-        expected_params["subnet"] = nodegroup.ip_range_low
+        expected_params["subnet"] = '192.168.100.0'
         mocked_task.delay.assert_called_once_with(**expected_params)
 
     def test_add_dhcp_host_maps_adds_maps_if_managing_dhcp(self):