Merge lp:~allenap/maas/find-nodegroup-redux into lp:~maas-committers/maas/trunk

[Gavin Panella]

Find the requester's cluster controller by also matching unmanaged interfaces.

This allows MAAS installations that use custom DHCP servers to be supported. In addition, multiple cluster controllers can have interfaces on the same network. At most one of these interfaces can be "managed"; any more and an error will result. If multiple non-managed interfaces exist the first one to have been recorded in MAAS wins, and its cluster controller is chosen.

[Julian Edwards (julian-edwards)]

Very nice branch! Clear code, clear commenting and very easy to follow. Great work.

225 + def test_more_than_1_unmanaged_interface(self):
226 + nodegroup1 = factory.make_node_group(
227 + management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
228 + network=IPNetwork("192.168.41.0/16"))
229 + nodegroup2 = factory.make_node_group(
230 + management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
231 + network=IPNetwork("192.168.41.0/24"))
232 + self.assertIn(
233 + find_nodegroup(get_request('192.168.41.199')),
234 + {nodegroup1, nodegroup2})

Since the query to find the interfaces is deterministic, should it check for a particular nodegroup?

review: Approve

[Andres Rodriguez (andreserl)]

lgtm too! I have tested this with 2 cluster controllers on different networks, and *not* managing any interfaces, and the nodes were assigned to the correct nodegroup/interface.

[Andres Rodriguez (andreserl)]

review: Approve

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2012-10-24 16:29:02 +0000
+++ src/maasserver/exceptions.py	2013-04-05 11:13:23 +0000
@@ -17,6 +17,7 @@
     "MAASAPIException",
     "MAASAPINotFound",
     "NodeStateViolation",
+    "NodeGroupMisconfiguration",
     ]
 
 
@@ -114,3 +115,12 @@
 
     def make_http_response(self):
         return HttpResponseRedirect(unicode(self))
+
+
+class NodeGroupMisconfiguration(MAASAPIException):
+    """Node Groups (aka Cluster Controllers) are misconfigured.
+
+    This might mean that more than one controller is marked as managing the
+    same network
+    """
+    api_error = httplib.CONFLICT

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2013-02-14 10:32:22 +0000
+++ src/maasserver/utils/__init__.py	2013-04-05 11:13:23 +0000
@@ -29,6 +29,7 @@
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
+from maasserver.exceptions import NodeGroupMisconfiguration
 from maasserver.utils.orm import get_one
 
 
@@ -134,33 +135,57 @@
 
 
 def find_nodegroup(request):
-    """Find the nodegroup whose subnet contains the IP Address of the
-    originating host of the request..
-
-    The matching nodegroup may have multiple interfaces on the subnet,
-    but there can be only one matching nodegroup.
+    """Find the nodegroup whose subnet contains the requester's address.
+
+    There may be multiple matching nodegroups, but this endeavours to choose
+    the most appropriate.
+
+    :raises `maasserver.exceptions.NodeGroupMisconfiguration`: When more than
+        one nodegroup claims to manage the requester's network.
     """
     # Circular imports.
     from maasserver.models import NodeGroup
     ip_address = request.META['REMOTE_ADDR']
-    if ip_address is not None:
-        management_statuses = (
-            NODEGROUPINTERFACE_MANAGEMENT.DHCP,
-            NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
-        )
+    if ip_address is None:
+        return None
+    else:
+        # Fetch nodegroups with interfaces in the requester's network,
+        # preferring those with managed networks first. The `NodeGroup`
+        # objects returned are annotated with the `management` field of the
+        # matching `NodeGroupInterface`. See https://docs.djangoproject.com
+        # /en/dev/topics/db/sql/#adding-annotations for this curious feature
+        # of Django's ORM.
         query = NodeGroup.objects.raw("""
-            SELECT *
-            FROM maasserver_nodegroup
-            WHERE id IN (
-                SELECT nodegroup_id
-                FROM maasserver_nodegroupinterface
-                WHERE (inet %s & subnet_mask) = (ip & subnet_mask)
-                AND management IN %s
-            )
-            """, [
-                ip_address,
-                management_statuses,
-                ]
-            )
-        return get_one(query)
-    return None
+            SELECT
+              ng.*,
+              ngi.management
+            FROM
+              maasserver_nodegroup AS ng,
+              maasserver_nodegroupinterface AS ngi
+            WHERE
+              ng.id = ngi.nodegroup_id
+             AND
+              (inet %s & ngi.subnet_mask) = (ngi.ip & ngi.subnet_mask)
+            ORDER BY
+              ngi.management DESC,
+              ng.id ASC
+            """, [ip_address])
+        nodegroups = list(query)
+        if len(nodegroups) == 0:
+            return None
+        elif len(nodegroups) == 1:
+            return nodegroups[0]
+        else:
+            # There are multiple matching nodegroups. Only zero or one may
+            # have a managed interface, otherwise it is a misconfiguration.
+            unmanaged = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
+            nodegroups_with_managed_interfaces = {
+                nodegroup.id for nodegroup in nodegroups
+                if nodegroup.management != unmanaged
+                }
+            if len(nodegroups_with_managed_interfaces) > 1:
+                raise NodeGroupMisconfiguration(
+                    "Multiple clusters on the same network; only "
+                    "one cluster may manage the network of which "
+                    "%s is a member." % ip_address)
+            return nodegroups[0]

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2013-02-14 10:32:22 +0000
+++ src/maasserver/utils/tests/test_utils.py	2013-04-05 11:13:23 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = []
 
+import httplib
 from urllib import urlencode
 
 from django.conf import settings
@@ -22,11 +23,7 @@
     NODE_STATUS_CHOICES,
     NODEGROUPINTERFACE_MANAGEMENT,
     )
-from maasserver.models import (
-    NodeGroup,
-    nodegroupinterface,
-    NodeGroupInterface,
-    )
+from maasserver.exceptions import NodeGroupMisconfiguration
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase as DjangoTestCase
 from maasserver.utils import (
@@ -220,13 +217,6 @@
 
 class TestFindNodegroup(DjangoTestCase):
 
-    def test_finds_nodegroup_by_network_address(self):
-        nodegroup = factory.make_node_group(
-            network=IPNetwork("192.168.28.1/24"))
-        self.assertEqual(
-            nodegroup,
-            find_nodegroup(get_request('192.168.28.0')))
-
     def test_find_nodegroup_looks_up_nodegroup_by_controller_ip(self):
         nodegroup = factory.make_node_group()
         ip = nodegroup.get_managed_interface().ip
@@ -234,40 +224,69 @@
             nodegroup,
             find_nodegroup(get_request(ip)))
 
-    def test_find_nodegroup_looks_up_only_configured_interfaces(self):
-        network = IPNetwork("192.168.41.0/24")
-        factory.make_node_group(
-            network=network,
-            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
-        ip = factory.getRandomIPInNetwork(network)
-        self.assertIsNone(find_nodegroup(get_request(ip)))
-
-    def test_find_nodegroup_accepts_any_ip_in_nodegroup_subnet(self):
-        nodegroup = factory.make_node_group(
-            network=IPNetwork("192.168.41.0/24"))
-        self.assertEqual(
-            nodegroup,
-            find_nodegroup(get_request('192.168.41.199')))
-
     def test_find_nodegroup_returns_None_if_not_found(self):
         self.assertIsNone(
             find_nodegroup(get_request(factory.getRandomIPAddress())))
 
-    def test_find_nodegroup_errors_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(
-            NodeGroup.MultipleObjectsReturned,
-            find_nodegroup, get_request('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',
-            broadcast_ip='10.0.0.1', interface='eth71')
-        NodeGroupInterface.objects.create(
-            nodegroup=nodegroup, ip='10.0.0.3', subnet_mask='255.0.0.0',
-            broadcast_ip='10.0.0.2', interface='eth72')
-        self.assertEqual(nodegroup, find_nodegroup(get_request('10.0.0.9')))
+    #
+    # Finding a node's nodegroup (aka cluster controller) in a nutshell:
+    #
+    #   when 1 managed interface on the network = choose this one
+    #   when >1 managed interfaces on the network = misconfiguration
+    #   when 1 unmanaged interface on a network = choose this one
+    #   when >1 unmanaged interfaces on a network = choose any
+    #
+
+    def test_1_managed_interface(self):
+        nodegroup = factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
+            network=IPNetwork("192.168.41.0/24"))
+        self.assertEqual(
+            nodegroup, find_nodegroup(get_request('192.168.41.199')))
+
+    def test_1_managed_interface_and_1_unmanaged(self):
+        # The managed nodegroup is chosen in preference to the unmanaged
+        # nodegroup.
+        nodegroup = factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
+            network=IPNetwork("192.168.41.0/24"))
+        factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
+            network=IPNetwork("192.168.41.0/16"))
+        self.assertEqual(
+            nodegroup, find_nodegroup(get_request('192.168.41.199')))
+
+    def test_more_than_1_managed_interface(self):
+        factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
+            network=IPNetwork("192.168.41.0/16"))
+        factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
+            network=IPNetwork("192.168.41.0/24"))
+        exception = self.assertRaises(
+            NodeGroupMisconfiguration,
+            find_nodegroup, get_request('192.168.41.199'))
+        self.assertEqual(
+            (httplib.CONFLICT,
+             "Multiple clusters on the same network; only "
+             "one cluster may manage the network of which "
+             "192.168.41.199 is a member."),
+            (exception.api_error,
+             "%s" % exception))
+
+    def test_1_unmanaged_interface(self):
+        nodegroup = factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
+            network=IPNetwork("192.168.41.0/24"))
+        self.assertEqual(
+            nodegroup, find_nodegroup(get_request('192.168.41.199')))
+
+    def test_more_than_1_unmanaged_interface(self):
+        nodegroup1 = factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
+            network=IPNetwork("192.168.41.0/16"))
+        factory.make_node_group(
+            management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
+            network=IPNetwork("192.168.41.0/24"))
+        self.assertEqual(
+            nodegroup1, find_nodegroup(get_request('192.168.41.199')))