Merge lp:~rvb/maas/big-networks into lp:maas/trunk

[Raphaël Badin]

Reject networks >= /8 networks.

Reject networks >= /8 networks. This ends up pre-populating a huge zone file that neither celeryd nor bind handle properly.

[Gavin Panella (allenap)]

Looks good.

[1]

+# For instance, if MINIMUM_PREFIX_LEN is 9, a /8 will be rejected.
+MINIMUM_PREFIX_LEN = 9

This still allows for a network of ~8.3 million addresses. While we're generating the full zone maps, it seems ridiculous to support anything remotely close to that. If we want to support a flat network for 100k nodes then a /15 is all we need, but even that is slightly unhinged right now. A /16 is far more than enough for the size of cluster we're expecting (*dreaming*) of supporting, and if we want more than that we need to get rid of the pre-generation stuff, or rethink it (e.g. serve it up dynamically).

review: Approve

[Gavin Panella (allenap)]

Fwiw, I'm not against letting people specify a /8 if they want to. It's just that MAAS will grind to a halt right now, and appear desperately broken, and I think it's better to politely say "non" earlier in the process than to let it break like that.

[lp:~rvb/maas/big-networks]

1187. By Raphaël Badin on 2012-10-07

Only look for big networks in managed interfaces.

1188. By Raphaël Badin on 2012-10-07

Fix tests.

1189. By Raphaël Badin on 2012-10-07

Merge trunk.

1190. By Raphaël Badin on 2012-10-07

Fix test.

[Raphaël Badin (rvb)]

> This still allows for a network of ~8.3 million addresses. While we're
> generating the full zone maps, it seems ridiculous to support anything
> remotely close to that. If we want to support a flat network for 100k nodes
> then a /15 is all we need, but even that is slightly unhinged right now. A /16
> is far more than enough for the size of cluster we're expecting (*dreaming*)
> of supporting, and if we want more than that we need to get rid of the pre-
> generation stuff, or rethink it (e.g. serve it up dynamically).

You're definitely right… but I think it would be good to allow for a 100k network so I changed the limit to "/15". We can adjust that after doing so testing if we need to.

[Julian Edwards (julian-edwards)]

On Sunday 07 October 2012 17:34:21 you wrote:
> > This still allows for a network of ~8.3 million addresses. While we're
> > generating the full zone maps, it seems ridiculous to support anything
> > remotely close to that. If we want to support a flat network for 100k
> > nodes
> > then a /15 is all we need, but even that is slightly unhinged right now. A
> > /16 is far more than enough for the size of cluster we're expecting
> > (*dreaming*) of supporting, and if we want more than that we need to get
> > rid of the pre- generation stuff, or rethink it (e.g. serve it up
> > dynamically).
>
> You're definitely right… but I think it would be good to allow for a 100k
> network so I changed the limit to "/15". We can adjust that after doing so
> testing if we need to.

Yeah we had agreed /16 in the pre-imp I had with you :) /15 is ok too.

[Julian Edwards (julian-edwards)]

I think MINIMUM_PREFIX_LEN isn't an informative constant name as it could be.
Perhaps MINIMUM_NETMASK_BITS ?

[lp:~rvb/maas/big-networks]

1191. By Raphaël Badin on 2012-10-08

Rename MINIMUM_PREFIX_LEN into MINIMUM_NETMASK_BITS. Set minimum to /16.

[Raphaël Badin (rvb)]

>
> Yeah we had agreed /16 in the pre-imp I had with you :) /15 is ok too.

Really? I confess I don't really remember :). /16 it is then.

[Raphaël Badin (rvb)]

> I think MINIMUM_PREFIX_LEN isn't an informative constant name as it could be.
> Perhaps MINIMUM_NETMASK_BITS ?

Ok, done.

=== modified file 'src/maasserver/models/nodegroupinterface.py'
--- src/maasserver/models/nodegroupinterface.py	2012-10-04 11:43:37 +0000
+++ src/maasserver/models/nodegroupinterface.py	2012-10-08 07:37:24 +0000
@@ -38,6 +38,13 @@
     )
 from netaddr.core import AddrFormatError
 
+# The minimum number of leading bits of the routing prefix for a
+# network.  A smaller number will be rejected as it creates a huge
+# address space that is currently not well supported by the DNS
+# machinery.
+# For instance, if MINIMUM_NETMASK_BITS is 9, a /8 will be rejected.
+MINIMUM_NETMASK_BITS = 16
+
 
 class NodeGroupInterface(CleanSave, TimestampedModel):
 
@@ -93,8 +100,8 @@
         return "<NodeGroupInterface %s,%s>" % (
             self.nodegroup.uuid, self.interface)
 
-    def clean_network(self):
-        """Validate that the network is valid.
+    def clean_network_valid(self):
+        """Validate the network.
 
         This validates that the network defined by broadcast_ip and
         subnet_mask is valid.
@@ -111,7 +118,21 @@
                     'subnet_mask': [e.message],
                 })
 
-            raise ValidationError(e.message)
+    def clean_network_not_too_big(self):
+        # If management is not 'UNMANAGED', refuse huge networks.
+        if self.management != NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED:
+            network = self.network
+            if network is not None:
+                if network.prefixlen < MINIMUM_NETMASK_BITS:
+                    message = (
+                        "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],
+                    })
 
     def clean_management(self):
         # XXX: rvb 2012-09-18 bug=1052339: Only one "managed" interface
@@ -181,7 +202,8 @@
 
     def clean_fields(self, *args, **kwargs):
         super(NodeGroupInterface, self).clean_fields(*args, **kwargs)
-        self.clean_network()
+        self.clean_network_valid()
+        self.clean_network_not_too_big()
         self.clean_ips_in_network()
         self.clean_management()
         self.clean_network_config_if_managed()

=== modified file 'src/maasserver/tests/test_fields.py'
--- src/maasserver/tests/test_fields.py	2012-10-03 11:19:13 +0000
+++ src/maasserver/tests/test_fields.py	2012-10-08 07:37:24 +0000
@@ -21,6 +21,7 @@
 from maasserver.models import (
     MACAddress,
     NodeGroup,
+    nodegroupinterface,
     NodeGroupInterface,
     )
 from maasserver.testing.factory import factory
@@ -96,6 +97,7 @@
             factory.getRandomIPAddress())
 
     def test_find_nodegroup_reports_if_multiple_matches(self):
+        self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)
         factory.make_node_group(network=IPNetwork("10/8"))
         factory.make_node_group(network=IPNetwork("10.1.1/24"))
         self.assertRaises(
@@ -103,6 +105,7 @@
             NodeGroupFormField().clean, '10.1.1.2')
 
     def test_find_nodegroup_handles_multiple_matches_on_same_nodegroup(self):
+        self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)
         nodegroup = factory.make_node_group(network=IPNetwork("10/8"))
         NodeGroupInterface.objects.create(
             nodegroup=nodegroup, ip='10.0.0.2', subnet_mask='255.0.0.0',

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-10-03 11:19:13 +0000
+++ src/maasserver/tests/test_forms.py	2012-10-08 07:37:24 +0000
@@ -211,9 +211,9 @@
         # nodes.  You can't change it later.
         original_nodegroup = factory.make_node_group()
         node = factory.make_node(nodegroup=original_nodegroup)
-        factory.make_node_group(network=IPNetwork("10.0.0.0/8"))
+        factory.make_node_group(network=IPNetwork("192.168.1.0/24"))
         form = NodeWithMACAddressesForm(
-            self.make_params(nodegroup='10.0.0.1'), instance=node)
+            self.make_params(nodegroup='192.168.1.0'), instance=node)
         form.save()
         self.assertEqual(original_nodegroup, reload_object(node).nodegroup)
 

=== modified file 'src/maasserver/tests/test_nodegroupinterface.py'
--- src/maasserver/tests/test_nodegroupinterface.py	2012-10-04 11:43:37 +0000
+++ src/maasserver/tests/test_nodegroupinterface.py	2012-10-08 07:37:24 +0000
@@ -18,6 +18,8 @@
     NODEGROUPINTERFACE_MANAGEMENT,
     NODEGROUPINTERFACE_MANAGEMENT_CHOICES_DICT,
     )
+from maasserver.models import NodeGroup
+from maasserver.models.nodegroupinterface import MINIMUM_NETMASK_BITS
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from netaddr import IPNetwork
@@ -97,6 +99,33 @@
             },
             exception.message_dict)
 
+    def test_clean_network_rejects_huge_network(self):
+        big_network = IPNetwork('1.2.3.4/%d' % (MINIMUM_NETMASK_BITS - 1))
+        exception = self.assertRaises(
+            ValidationError, factory.make_node_group, network=big_network)
+        message = (
+            "Cannot create an address space bigger than a /%d network.  "
+            "This network is a /%d network." %
+                (MINIMUM_NETMASK_BITS, MINIMUM_NETMASK_BITS - 1))
+        self.assertEqual(
+            {
+                'subnet_mask': [message],
+                'broadcast_ip': [message],
+            },
+            exception.message_dict)
+
+    def test_clean_network_accepts_network_if_not_too_big(self):
+        network = IPNetwork('1.2.3.4/%d' % MINIMUM_NETMASK_BITS)
+        self.assertIsInstance(
+            factory.make_node_group(network=network), NodeGroup)
+
+    def test_clean_network_accepts_big_network_if_unmanaged(self):
+        network = IPNetwork('1.2.3.4/%d' % (MINIMUM_NETMASK_BITS - 1))
+        nodegroup = factory.make_node_group(
+            network=network,
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
+        self.assertIsInstance(nodegroup, NodeGroup)
+
     def test_clean_network_config_if_managed(self):
         network = IPNetwork('192.168.0.3/24')
         checked_fields = [