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
=== 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 @@
17 "MAASAPIException",17 "MAASAPIException",
18 "MAASAPINotFound",18 "MAASAPINotFound",
19 "NodeStateViolation",19 "NodeStateViolation",
20 "NodeGroupMisconfiguration",
20 ]21 ]
2122
2223
@@ -114,3 +115,12 @@
114115
115 def make_http_response(self):116 def make_http_response(self):
116 return HttpResponseRedirect(unicode(self))117 return HttpResponseRedirect(unicode(self))
118
119
120class NodeGroupMisconfiguration(MAASAPIException):
121 """Node Groups (aka Cluster Controllers) are misconfigured.
122
123 This might mean that more than one controller is marked as managing the
124 same network
125 """
126 api_error = httplib.CONFLICT
117127
=== 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 @@
29from django.conf import settings29from django.conf import settings
30from django.core.urlresolvers import reverse30from django.core.urlresolvers import reverse
31from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT31from maasserver.enum import NODEGROUPINTERFACE_MANAGEMENT
32from maasserver.exceptions import NodeGroupMisconfiguration
32from maasserver.utils.orm import get_one33from maasserver.utils.orm import get_one
3334
3435
@@ -134,33 +135,57 @@
134135
135136
136def find_nodegroup(request):137def find_nodegroup(request):
137 """Find the nodegroup whose subnet contains the IP Address of the138 """Find the nodegroup whose subnet contains the requester's address.
138 originating host of the request..139
139140 There may be multiple matching nodegroups, but this endeavours to choose
140 The matching nodegroup may have multiple interfaces on the subnet,141 the most appropriate.
141 but there can be only one matching nodegroup.142
143 :raises `maasserver.exceptions.NodeGroupMisconfiguration`: When more than
144 one nodegroup claims to manage the requester's network.
142 """145 """
143 # Circular imports.146 # Circular imports.
144 from maasserver.models import NodeGroup147 from maasserver.models import NodeGroup
145 ip_address = request.META['REMOTE_ADDR']148 ip_address = request.META['REMOTE_ADDR']
146 if ip_address is not None:149 if ip_address is None:
147 management_statuses = (150 return None
148 NODEGROUPINTERFACE_MANAGEMENT.DHCP,151 else:
149 NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,152 # Fetch nodegroups with interfaces in the requester's network,
150 )153 # preferring those with managed networks first. The `NodeGroup`
154 # objects returned are annotated with the `management` field of the
155 # matching `NodeGroupInterface`. See https://docs.djangoproject.com
156 # /en/dev/topics/db/sql/#adding-annotations for this curious feature
157 # of Django's ORM.
151 query = NodeGroup.objects.raw("""158 query = NodeGroup.objects.raw("""
152 SELECT *159 SELECT
153 FROM maasserver_nodegroup160 ng.*,
154 WHERE id IN (161 ngi.management
155 SELECT nodegroup_id162 FROM
156 FROM maasserver_nodegroupinterface163 maasserver_nodegroup AS ng,
157 WHERE (inet %s & subnet_mask) = (ip & subnet_mask)164 maasserver_nodegroupinterface AS ngi
158 AND management IN %s165 WHERE
159 )166 ng.id = ngi.nodegroup_id
160 """, [167 AND
161 ip_address,168 (inet %s & ngi.subnet_mask) = (ngi.ip & ngi.subnet_mask)
162 management_statuses,169 ORDER BY
163 ]170 ngi.management DESC,
164 )171 ng.id ASC
165 return get_one(query)172 """, [ip_address])
166 return None173 nodegroups = list(query)
174 if len(nodegroups) == 0:
175 return None
176 elif len(nodegroups) == 1:
177 return nodegroups[0]
178 else:
179 # There are multiple matching nodegroups. Only zero or one may
180 # have a managed interface, otherwise it is a misconfiguration.
181 unmanaged = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
182 nodegroups_with_managed_interfaces = {
183 nodegroup.id for nodegroup in nodegroups
184 if nodegroup.management != unmanaged
185 }
186 if len(nodegroups_with_managed_interfaces) > 1:
187 raise NodeGroupMisconfiguration(
188 "Multiple clusters on the same network; only "
189 "one cluster may manage the network of which "
190 "%s is a member." % ip_address)
191 return nodegroups[0]
167192
=== 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 @@
12__metaclass__ = type12__metaclass__ = type
13__all__ = []13__all__ = []
1414
15import httplib
15from urllib import urlencode16from urllib import urlencode
1617
17from django.conf import settings18from django.conf import settings
@@ -22,11 +23,7 @@
22 NODE_STATUS_CHOICES,23 NODE_STATUS_CHOICES,
23 NODEGROUPINTERFACE_MANAGEMENT,24 NODEGROUPINTERFACE_MANAGEMENT,
24 )25 )
25from maasserver.models import (26from maasserver.exceptions import NodeGroupMisconfiguration
26 NodeGroup,
27 nodegroupinterface,
28 NodeGroupInterface,
29 )
30from maasserver.testing.factory import factory27from maasserver.testing.factory import factory
31from maasserver.testing.testcase import TestCase as DjangoTestCase28from maasserver.testing.testcase import TestCase as DjangoTestCase
32from maasserver.utils import (29from maasserver.utils import (
@@ -220,13 +217,6 @@
220217
221class TestFindNodegroup(DjangoTestCase):218class TestFindNodegroup(DjangoTestCase):
222219
223 def test_finds_nodegroup_by_network_address(self):
224 nodegroup = factory.make_node_group(
225 network=IPNetwork("192.168.28.1/24"))
226 self.assertEqual(
227 nodegroup,
228 find_nodegroup(get_request('192.168.28.0')))
229
230 def test_find_nodegroup_looks_up_nodegroup_by_controller_ip(self):220 def test_find_nodegroup_looks_up_nodegroup_by_controller_ip(self):
231 nodegroup = factory.make_node_group()221 nodegroup = factory.make_node_group()
232 ip = nodegroup.get_managed_interface().ip222 ip = nodegroup.get_managed_interface().ip
@@ -234,40 +224,69 @@
234 nodegroup,224 nodegroup,
235 find_nodegroup(get_request(ip)))225 find_nodegroup(get_request(ip)))
236226
237 def test_find_nodegroup_looks_up_only_configured_interfaces(self):
238 network = IPNetwork("192.168.41.0/24")
239 factory.make_node_group(
240 network=network,
241 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED)
242 ip = factory.getRandomIPInNetwork(network)
243 self.assertIsNone(find_nodegroup(get_request(ip)))
244
245 def test_find_nodegroup_accepts_any_ip_in_nodegroup_subnet(self):
246 nodegroup = factory.make_node_group(
247 network=IPNetwork("192.168.41.0/24"))
248 self.assertEqual(
249 nodegroup,
250 find_nodegroup(get_request('192.168.41.199')))
251
252 def test_find_nodegroup_returns_None_if_not_found(self):227 def test_find_nodegroup_returns_None_if_not_found(self):
253 self.assertIsNone(228 self.assertIsNone(
254 find_nodegroup(get_request(factory.getRandomIPAddress())))229 find_nodegroup(get_request(factory.getRandomIPAddress())))
255230
256 def test_find_nodegroup_errors_if_multiple_matches(self):231 #
257 self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)232 # Finding a node's nodegroup (aka cluster controller) in a nutshell:
258 factory.make_node_group(network=IPNetwork("10/8"))233 #
259 factory.make_node_group(network=IPNetwork("10.1.1/24"))234 # when 1 managed interface on the network = choose this one
260 self.assertRaises(235 # when >1 managed interfaces on the network = misconfiguration
261 NodeGroup.MultipleObjectsReturned,236 # when 1 unmanaged interface on a network = choose this one
262 find_nodegroup, get_request('10.1.1.2'))237 # when >1 unmanaged interfaces on a network = choose any
263238 #
264 def test_find_nodegroup_handles_multiple_matches_on_same_nodegroup(self):239
265 self.patch(nodegroupinterface, "MINIMUM_NETMASK_BITS", 1)240 def test_1_managed_interface(self):
266 nodegroup = factory.make_node_group(network=IPNetwork("10/8"))241 nodegroup = factory.make_node_group(
267 NodeGroupInterface.objects.create(242 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
268 nodegroup=nodegroup, ip='10.0.0.2', subnet_mask='255.0.0.0',243 network=IPNetwork("192.168.41.0/24"))
269 broadcast_ip='10.0.0.1', interface='eth71')244 self.assertEqual(
270 NodeGroupInterface.objects.create(245 nodegroup, find_nodegroup(get_request('192.168.41.199')))
271 nodegroup=nodegroup, ip='10.0.0.3', subnet_mask='255.0.0.0',246
272 broadcast_ip='10.0.0.2', interface='eth72')247 def test_1_managed_interface_and_1_unmanaged(self):
273 self.assertEqual(nodegroup, find_nodegroup(get_request('10.0.0.9')))248 # The managed nodegroup is chosen in preference to the unmanaged
249 # nodegroup.
250 nodegroup = factory.make_node_group(
251 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
252 network=IPNetwork("192.168.41.0/24"))
253 factory.make_node_group(
254 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
255 network=IPNetwork("192.168.41.0/16"))
256 self.assertEqual(
257 nodegroup, find_nodegroup(get_request('192.168.41.199')))
258
259 def test_more_than_1_managed_interface(self):
260 factory.make_node_group(
261 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
262 network=IPNetwork("192.168.41.0/16"))
263 factory.make_node_group(
264 management=NODEGROUPINTERFACE_MANAGEMENT.DHCP_AND_DNS,
265 network=IPNetwork("192.168.41.0/24"))
266 exception = self.assertRaises(
267 NodeGroupMisconfiguration,
268 find_nodegroup, get_request('192.168.41.199'))
269 self.assertEqual(
270 (httplib.CONFLICT,
271 "Multiple clusters on the same network; only "
272 "one cluster may manage the network of which "
273 "192.168.41.199 is a member."),
274 (exception.api_error,
275 "%s" % exception))
276
277 def test_1_unmanaged_interface(self):
278 nodegroup = factory.make_node_group(
279 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
280 network=IPNetwork("192.168.41.0/24"))
281 self.assertEqual(
282 nodegroup, find_nodegroup(get_request('192.168.41.199')))
283
284 def test_more_than_1_unmanaged_interface(self):
285 nodegroup1 = factory.make_node_group(
286 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
287 network=IPNetwork("192.168.41.0/16"))
288 factory.make_node_group(
289 management=NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED,
290 network=IPNetwork("192.168.41.0/24"))
291 self.assertEqual(
292 nodegroup1, find_nodegroup(get_request('192.168.41.199')))