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

Proposed by Gavin Panella
Status: Merged
Approved by: Andres Rodriguez
Approved revision: 1469
Merged at revision: 1457
Proposed branch: lp:~allenap/maas/find-nodegroup-redux
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 261 lines (+124/-70)
3 files modified
src/maasserver/exceptions.py (+10/-0)
src/maasserver/utils/__init__.py (+50/-25)
src/maasserver/utils/tests/test_utils.py (+64/-45)
To merge this branch: bzr merge lp:~allenap/maas/find-nodegroup-redux
Reviewer Review Type Date Requested Status
Andres Rodriguez (community) Approve
Julian Edwards (community) Approve
Review via email: mp+157238@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
1465. By Gavin Panella

Result of find_nodegroup() is deterministic, so tighten up test.

1466. By Gavin Panella

Improve the flow of find_nodegroups() logic.

1467. By Gavin Panella

Improve find_nodegroup()'s docstring.

1468. By Gavin Panella

(expected, observed).

1469. By Gavin Panella

Remove duplicate/superfluous tests.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

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.

Revision history for this message
Andres Rodriguez (andreserl) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/exceptions.py'
2--- src/maasserver/exceptions.py 2012-10-24 16:29:02 +0000
3+++ src/maasserver/exceptions.py 2013-04-05 11:13:23 +0000
4@@ -17,6 +17,7 @@
5 "MAASAPIException",
6 "MAASAPINotFound",
7 "NodeStateViolation",
8+ "NodeGroupMisconfiguration",
9 ]
10
11
12@@ -114,3 +115,12 @@
13
14 def make_http_response(self):
15 return HttpResponseRedirect(unicode(self))
16+
17+
18+class NodeGroupMisconfiguration(MAASAPIException):
19+ """Node Groups (aka Cluster Controllers) are misconfigured.
20+
21+ This might mean that more than one controller is marked as managing the
22+ same network
23+ """
24+ api_error = httplib.CONFLICT
25
26=== modified file 'src/maasserver/utils/__init__.py'
27--- src/maasserver/utils/__init__.py 2013-02-14 10:32:22 +0000
28+++ src/maasserver/utils/__init__.py 2013-04-05 11:13:23 +0000
29@@ -29,6 +29,7 @@
30 from django.conf import settings
31 from django.core.urlresolvers import reverse
32 from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
33+from maasserver.exceptions import NodeGroupMisconfiguration
34 from maasserver.utils.orm import get_one
35
36
37@@ -134,33 +135,57 @@
38
39
40 def find_nodegroup(request):
41- """Find the nodegroup whose subnet contains the IP Address of the
42- originating host of the request..
43-
44- The matching nodegroup may have multiple interfaces on the subnet,
45- but there can be only one matching nodegroup.
46+ """Find the nodegroup whose subnet contains the requester's address.
47+
48+ There may be multiple matching nodegroups, but this endeavours to choose
49+ the most appropriate.
50+
51+ :raises `maasserver.exceptions.NodeGroupMisconfiguration`: When more than
52+ one nodegroup claims to manage the requester's network.
53 """
54 # Circular imports.
55 from maasserver.models import NodeGroup
56 ip_address = request.META['REMOTE_ADDR']
57- if ip_address is not None:
58- management_statuses = (
59- NODEGROUPINTERFACE_MANAGEMENT.DHCP,
60- NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
61- )
62+ if ip_address is None:
63+ return None
64+ else:
65+ # Fetch nodegroups with interfaces in the requester's network,
66+ # preferring those with managed networks first. The `NodeGroup`
67+ # objects returned are annotated with the `management` field of the
68+ # matching `NodeGroupInterface`. See https://docs.djangoproject.com
69+ # /en/dev/topics/db/sql/#adding-annotations for this curious feature
70+ # of Django's ORM.
71 query = NodeGroup.objects.raw("""
72- SELECT *
73- FROM maasserver_nodegroup
74- WHERE id IN (
75- SELECT nodegroup_id
76- FROM maasserver_nodegroupinterface
77- WHERE (inet %s & subnet_mask) = (ip & subnet_mask)
78- AND management IN %s
79- )
80- """, [
81- ip_address,
82- management_statuses,
83- ]
84- )
85- return get_one(query)
86- return None
87+ SELECT
88+ ng.*,
89+ ngi.management
90+ FROM
91+ maasserver_nodegroup AS ng,
92+ maasserver_nodegroupinterface AS ngi
93+ WHERE
94+ ng.id = ngi.nodegroup_id
95+ AND
96+ (inet %s & ngi.subnet_mask) = (ngi.ip & ngi.subnet_mask)
97+ ORDER BY
98+ ngi.management DESC,
99+ ng.id ASC
100+ """, [ip_address])
101+ nodegroups = list(query)
102+ if len(nodegroups) == 0:
103+ return None
104+ elif len(nodegroups) == 1:
105+ return nodegroups[0]
106+ else:
107+ # There are multiple matching nodegroups. Only zero or one may
108+ # have a managed interface, otherwise it is a misconfiguration.
109+ unmanaged = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
110+ nodegroups_with_managed_interfaces = {
111+ nodegroup.id for nodegroup in nodegroups
112+ if nodegroup.management != unmanaged
113+ }
114+ if len(nodegroups_with_managed_interfaces) > 1:
115+ raise NodeGroupMisconfiguration(
116+ "Multiple clusters on the same network; only "
117+ "one cluster may manage the network of which "
118+ "%s is a member." % ip_address)
119+ return nodegroups[0]
120
121=== modified file 'src/maasserver/utils/tests/test_utils.py'
122--- src/maasserver/utils/tests/test_utils.py 2013-02-14 10:32:22 +0000
123+++ src/maasserver/utils/tests/test_utils.py 2013-04-05 11:13:23 +0000
124@@ -12,6 +12,7 @@
125 __metaclass__ = type
126 __all__ = []
127
128+import httplib
129 from urllib import urlencode
130
131 from django.conf import settings
132@@ -22,11 +23,7 @@
133 NODE_STATUS_CHOICES,
134 NODEGROUPINTERFACE_MANAGEMENT,
135 )
136-from maasserver.models import (
137- NodeGroup,
138- nodegroupinterface,
139- NodeGroupInterface,
140- )
141+from maasserver.exceptions import NodeGroupMisconfiguration
142 from maasserver.testing.factory import factory
143 from maasserver.testing.testcase import TestCase as DjangoTestCase
144 from maasserver.utils import (
145@@ -220,13 +217,6 @@
146
147 class TestFindNodegroup(DjangoTestCase):
148
149- def test_finds_nodegroup_by_network_address(self):
150- nodegroup = factory.make_node_group(
151- network=IPNetwork("192.168.28.1/24"))
152- self.assertEqual(
153- nodegroup,
154- find_nodegroup(get_request('192.168.28.0')))
155-
156 def test_find_nodegroup_looks_up_nodegroup_by_controller_ip(self):
157 nodegroup = factory.make_node_group()
158 ip = nodegroup.get_managed_interface().ip
159@@ -234,40 +224,69 @@
160 nodegroup,
161 find_nodegroup(get_request(ip)))
162
163- def test_find_nodegroup_looks_up_only_configured_interfaces(self):
164- network = IPNetwork("192.168.41.0/24")
165- factory.make_node_group(
166- network=network,
167- management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
168- ip = factory.getRandomIPInNetwork(network)
169- self.assertIsNone(find_nodegroup(get_request(ip)))
170-
171- def test_find_nodegroup_accepts_any_ip_in_nodegroup_subnet(self):
172- nodegroup = factory.make_node_group(
173- network=IPNetwork("192.168.41.0/24"))
174- self.assertEqual(
175- nodegroup,
176- find_nodegroup(get_request('192.168.41.199')))
177-
178 def test_find_nodegroup_returns_None_if_not_found(self):
179 self.assertIsNone(
180 find_nodegroup(get_request(factory.getRandomIPAddress())))
181
182- def test_find_nodegroup_errors_if_multiple_matches(self):
183- self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)
184- factory.make_node_group(network=IPNetwork("10/8"))
185- factory.make_node_group(network=IPNetwork("10.1.1/24"))
186- self.assertRaises(
187- NodeGroup.MultipleObjectsReturned,
188- find_nodegroup, get_request('10.1.1.2'))
189-
190- def test_find_nodegroup_handles_multiple_matches_on_same_nodegroup(self):
191- self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)
192- nodegroup = factory.make_node_group(network=IPNetwork("10/8"))
193- NodeGroupInterface.objects.create(
194- nodegroup=nodegroup, ip='10.0.0.2', subnet_mask='255.0.0.0',
195- broadcast_ip='10.0.0.1', interface='eth71')
196- NodeGroupInterface.objects.create(
197- nodegroup=nodegroup, ip='10.0.0.3', subnet_mask='255.0.0.0',
198- broadcast_ip='10.0.0.2', interface='eth72')
199- self.assertEqual(nodegroup, find_nodegroup(get_request('10.0.0.9')))
200+ #
201+ # Finding a node's nodegroup (aka cluster controller) in a nutshell:
202+ #
203+ # when 1 managed interface on the network = choose this one
204+ # when >1 managed interfaces on the network = misconfiguration
205+ # when 1 unmanaged interface on a network = choose this one
206+ # when >1 unmanaged interfaces on a network = choose any
207+ #
208+
209+ def test_1_managed_interface(self):
210+ nodegroup = factory.make_node_group(
211+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
212+ network=IPNetwork("192.168.41.0/24"))
213+ self.assertEqual(
214+ nodegroup, find_nodegroup(get_request('192.168.41.199')))
215+
216+ def test_1_managed_interface_and_1_unmanaged(self):
217+ # The managed nodegroup is chosen in preference to the unmanaged
218+ # nodegroup.
219+ nodegroup = factory.make_node_group(
220+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
221+ network=IPNetwork("192.168.41.0/24"))
222+ factory.make_node_group(
223+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
224+ network=IPNetwork("192.168.41.0/16"))
225+ self.assertEqual(
226+ nodegroup, find_nodegroup(get_request('192.168.41.199')))
227+
228+ def test_more_than_1_managed_interface(self):
229+ factory.make_node_group(
230+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
231+ network=IPNetwork("192.168.41.0/16"))
232+ factory.make_node_group(
233+ management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
234+ network=IPNetwork("192.168.41.0/24"))
235+ exception = self.assertRaises(
236+ NodeGroupMisconfiguration,
237+ find_nodegroup, get_request('192.168.41.199'))
238+ self.assertEqual(
239+ (httplib.CONFLICT,
240+ "Multiple clusters on the same network; only "
241+ "one cluster may manage the network of which "
242+ "192.168.41.199 is a member."),
243+ (exception.api_error,
244+ "%s" % exception))
245+
246+ def test_1_unmanaged_interface(self):
247+ nodegroup = factory.make_node_group(
248+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
249+ network=IPNetwork("192.168.41.0/24"))
250+ self.assertEqual(
251+ nodegroup, find_nodegroup(get_request('192.168.41.199')))
252+
253+ def test_more_than_1_unmanaged_interface(self):
254+ nodegroup1 = factory.make_node_group(
255+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
256+ network=IPNetwork("192.168.41.0/16"))
257+ factory.make_node_group(
258+ management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
259+ network=IPNetwork("192.168.41.0/24"))
260+ self.assertEqual(
261+ nodegroup1, find_nodegroup(get_request('192.168.41.199')))