Merge lp:~jtv/maas/bug-1299374 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Make NodeGroupInterface.broadcast_ip optional; default to the network's default netmask, as a user might expect. Derive network from interface IP address and netmask, not broadcast address and netmask.

When I had to re-install my testing MAAS today, I was forced to enter a broadcast IP for a managed cluster interface. That's just silly. With perhaps some strange exceptions that sysadmins would know about, a network's broadcast address is simply the last IP address in the network's range — the one that has all host bits set to one. You're never forced to enter it anywhere because given a network's address and size, the standard value is obvious.

Nor did it make much sense to derive the network information from the interface's broadcast address. The interface's own network address has to be on the same network, and is required, so we might as well use that. There is nothing special about the broadcast address in this regard.

There was one strange thing with this branch. When I ran tests, the Selenium tests failed, as they always do on this system. But in addition to this, I got a failure in TestRegionService: test_stopping_logs_errors_when_closing_connections failed its assertDocTestMatches check, even though as far as I can tell the output does match. No discernible connection to my changes whatsoever.

It looks like a test isolation error. All test cases in test_regionservice.py are MAASTestCase, but they do modify the database — so you'd expect that they would have to be MAASServerTestCase. Turning them all into MAASServerTestCase broke more tests, but promoting just TestRegionService to MAASServerTestCase stopped the problem.

Jeroen

[Julian Edwards (julian-edwards)]

Nice!

review: Approve

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-03-27 17:52:53 +0000
+++ src/maasserver/models/nodegroup.py	2014-03-29 17:30:47 +0000
@@ -68,13 +68,13 @@
         dhcp_values = [
             interface,
             subnet_mask,
-            broadcast_ip,
             router_ip,
             ip_range_low,
             ip_range_high,
             ]
         assert all(dhcp_values) or not any(dhcp_values), (
-            "Provide all DHCP settings, or none at all.")
+            "Provide all DHCP settings, or none at all. "
+            "Only the broadcast address is optional.")
 
         if cluster_name is None:
             cluster_name = NODEGROUP_CLUSTER_NAME_TEMPLATE % {'uuid': uuid}
 
=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2014-02-07 04:10:09 +0000
+++ src/maasserver/models/nodegroupinterface.py	2014-03-29 17:30:47 +0000
@@ -86,22 +86,19 @@
 
     @property
     def network(self):
-        """Return the network defined by the broadcast address and net mask.
-
-        If either the broadcast address or the subnet mask is unset, returns
-        None.
-
-        :return: :class:`IPNetwork`
-        :raise AddrFormatError: If the combination of broadcast address and
+        """Return the network defined by the interface's address and netmask.
+
+        :return: :class:`IPNetwork`, or `None` if the netmask is unset.
+        :raise AddrFormatError: If the combination of interface address and
             subnet mask is malformed.
         """
-        broadcast = self.broadcast_ip
+        ip = self.ip
         netmask = self.subnet_mask
         # Nullness check for GenericIPAddress fields is deliberately kept
         # vague: GenericIPAddressField seems to represent nulls as empty
         # strings.
-        if broadcast and netmask:
-            return make_network(broadcast, netmask).cidr
+        if netmask:
+            return make_network(ip, netmask).cidr
         else:
             return None
 
@@ -116,20 +113,17 @@
     def clean_network_valid(self):
         """Validate the network.
 
-        This validates that the network defined by broadcast_ip and
-        subnet_mask is valid.
+        This validates that the network defined by `ip` and `subnet_mask` is
+        valid.
         """
         try:
             self.network
-        except AddrFormatError, e:
-            # Technically, this should be a global error but it's
-            # more user-friendly to precisely point out where the error
-            # comes from.
-            raise ValidationError(
-                {
-                    'broadcast_ip': [e.message],
-                    'subnet_mask': [e.message],
-                })
+        except AddrFormatError as e:
+            # The interface's address is validated separately.  If the
+            # combination with the netmask is invalid, either there's already
+            # going to be a specific validation error for the IP address, or
+            # the failure is due to an invalid netmask.
+            raise ValidationError({'subnet_mask': [e.message]})
 
     def clean_network_not_too_big(self):
         # If management is not 'UNMANAGED', refuse huge networks.
@@ -141,10 +135,7 @@
                         "Cannot create an address space bigger than "
                         "a /%d network.  This network is a /%d network." % (
                             MINIMUM_NETMASK_BITS, network.prefixlen))
-                    raise ValidationError({
-                        'broadcast_ip': [message],
-                        'subnet_mask': [message],
-                    })
+                    raise ValidationError({'subnet_mask': [message]})
 
     def clean_network_config_if_managed(self):
         # If management is not 'UNMANAGED', all the network information
@@ -152,7 +143,6 @@
         if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
             mandatory_fields = [
                 'interface',
-                'broadcast_ip',
                 'subnet_mask',
                 'router_ip',
                 'ip_range_low',
@@ -170,15 +160,18 @@
     def clean_ips_in_network(self):
         """Ensure that the network settings are all congruent.
 
-        Specifically, it ensures that the interface address, router address,
-        and the address range, all fall within the network defined by the
-        broadcast address and subnet mask.
+        Specifically, it ensures that the router address, the DHCP address
+        range, and the broadcast address if given, all fall within the network
+        defined by the interface's IP address and the subnet mask.
+
+        If no broadcast address is given, the network's default broadcast
+        address will be used.
         """
         network = self.network
         if network is None:
             return
         network_settings = (
-            ("ip", self.ip),
+            ("broadcast_ip", self.broadcast_ip),
             ("router_ip", self.router_ip),
             ("ip_range_low", self.ip_range_low),
             ("ip_range_high", self.ip_range_high),
@@ -191,6 +184,12 @@
         if len(network_errors) != 0:
             raise ValidationError(network_errors)
 
+        # Deliberately vague nullness check.  A null IP address seems to be
+        # None in some situations, or an empty string in others.
+        if not self.broadcast_ip:
+            # No broadcast address given.  Set the default.
+            self.broadcast_ip = network.broadcast
+
     def clean_fields(self, *args, **kwargs):
         super(NodeGroupInterface, self).clean_fields(*args, **kwargs)
         self.clean_network_valid()
 
=== modified file 'src/maasserver/models/tests/test_nodegroupinterface.py'
--- src/maasserver/models/tests/test_nodegroupinterface.py	2014-02-07 04:10:09 +0000
+++ src/maasserver/models/tests/test_nodegroupinterface.py	2014-03-29 17:30:47 +0000
@@ -20,7 +20,10 @@
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT,
     )
-from maasserver.models import NodeGroup
+from maasserver.models import (
+    NodeGroup,
+    NodeGroupInterface,
+    )
 from maasserver.models.nodegroupinterface import MINIMUM_NETMASK_BITS
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
@@ -44,28 +47,28 @@
         interface = make_interface(network=network)
         self.assertEqual(IPNetwork("10.0.0.0/24"), interface.network)
 
-    def test_network_is_defined_when_broadcast_and_mask_are(self):
+    def test_network_is_defined_when_netmask_is(self):
         interface = make_interface()
-        interface.broadcast = "10.0.0.255"
+        interface.ip = "10.0.0.9"
         interface.subnet_mask = "255.255.255.0"
         self.assertIsInstance(interface.network, IPNetwork)
 
-    def test_network_is_undefined_when_broadcast_is_None(self):
+    def test_network_does_not_require_broadcast_address(self):
         interface = make_interface()
         interface.broadcast_ip = None
-        self.assertIsNone(interface.network)
+        self.assertIsInstance(interface.network, IPNetwork)
 
-    def test_network_is_undefined_when_broadcast_is_empty(self):
+    def test_network_does_not_require_nonempty_broadcast_address(self):
         interface = make_interface()
         interface.broadcast_ip = ""
-        self.assertIsNone(interface.network)
+        self.assertIsInstance(interface.network, IPNetwork)
 
     def test_network_is_undefined_when_subnet_mask_is_None(self):
         interface = make_interface()
         interface.subnet_mask = None
         self.assertIsNone(interface.network)
 
-    def test_network_is_undefined_subnet_mask_is_empty(self):
+    def test_network_is_undefined_when_subnet_mask_is_empty(self):
         interface = make_interface()
         interface.subnet_mask = ""
         self.assertIsNone(interface.network)
@@ -78,8 +81,9 @@
 
     def test_clean_ips_in_network_validates_IP(self):
         network = IPNetwork('192.168.0.3/24')
+        ip_outside_network = '192.168.2.1'
         checked_fields = [
-            'ip',
+            'broadcast_ip',
             'router_ip',
             'ip_range_low',
             'ip_range_high',
@@ -87,14 +91,14 @@
         for field in checked_fields:
             nodegroup = factory.make_node_group(network=network)
             [interface] = nodegroup.get_managed_interfaces()
-            ip = '192.168.2.1'
-            setattr(interface, field, '192.168.2.1')
-            message = (
-                "%s not in the %s network" % (ip, '192.168.0.0/24'))
+            setattr(interface, field, ip_outside_network)
+            message = "%s not in the %s network" % (
+                ip_outside_network,
+                '192.168.0.0/24',
+                )
             exception = self.assertRaises(
                 ValidationError, interface.full_clean)
-            self.assertEqual(
-                {field: [message]}, exception.message_dict)
+            self.assertEqual({field: [message]}, exception.message_dict)
 
     def test_clean_network(self):
         nodegroup = factory.make_node_group(
@@ -102,13 +106,10 @@
         [interface] = nodegroup.get_managed_interfaces()
         # Set a bogus subnet mask.
         interface.subnet_mask = '0.9.0.4'
-        message = 'invalid IPNetwork 192.168.0.255/0.9.0.4'
+        message = "invalid IPNetwork %s/0.9.0.4" % interface.ip
         exception = self.assertRaises(ValidationError, interface.full_clean)
         self.assertEqual(
-            {
-                'subnet_mask': [message],
-                'broadcast_ip': [message],
-            },
+            {'subnet_mask': [message]},
             exception.message_dict)
 
     def test_clean_network_rejects_huge_network(self):
@@ -120,10 +121,7 @@
             "This network is a /%d network." % (
                 MINIMUM_NETMASK_BITS, MINIMUM_NETMASK_BITS - 1))
         self.assertEqual(
-            {
-                'subnet_mask': [message],
-                'broadcast_ip': [message],
-            },
+            {'subnet_mask': [message]},
             exception.message_dict)
 
     def test_clean_network_accepts_network_if_not_too_big(self):
@@ -142,7 +140,6 @@
         network = IPNetwork('192.168.0.3/24')
         checked_fields = [
             'interface',
-            'broadcast_ip',
             'subnet_mask',
             'router_ip',
             'ip_range_low',
@@ -160,3 +157,23 @@
                 "That field cannot be empty (unless that interface is "
                 "'unmanaged')")
             self.assertEqual({field: [message]}, exception.message_dict)
+
+    def test_clean_network_config_sets_default_if_netmask_not_given(self):
+        network = factory.getRandomNetwork()
+        nodegroup = factory.make_node_group(
+            network=network,
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS)
+        [interface] = nodegroup.get_managed_interfaces()
+        interface.full_clean()
+        self.assertEqual(unicode(network.broadcast), interface.broadcast_ip)
+
+    def test_clean_network_config_sets_no_broadcast_without_netmask(self):
+        network = factory.getRandomNetwork()
+        nodegroup = factory.make_node_group(
+            network=network,
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        interface = NodeGroupInterface.objects.get(nodegroup=nodegroup)
+        interface.subnet_mask = None
+        interface.broadcast_ip = None
+        interface.full_clean()
+        self.assertIsNone(interface.broadcast_ip)
 
=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2014-03-28 03:55:55 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-03-29 17:30:47 +0000
@@ -34,6 +34,7 @@
     RegionService,
     )
 from maasserver.rpc.testing.doubles import IdentifyingRegionServer
+from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.factory import factory
 from maastesting.matchers import (
     MockCalledOnceWith,
@@ -195,7 +196,7 @@
 from zope.interface.verify import verifyObject
 
 
-class TestRegionServer(MAASTestCase):
+class TestRegionServer(MAASServerTestCase):
 
     def test_interfaces(self):
         protocol = RegionServer()