Merge lp:~mpontillo/maas/networking-constraints-refactor-part4 into lp:~maas-committers/maas/trunk
- networking-constraints-refactor-part4
- Merge into trunk
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Mike Pontillo | ||||
| Approved revision: | no longer in the source branch. | ||||
| Merged at revision: | 4465 | ||||
| Proposed branch: | lp:~mpontillo/maas/networking-constraints-refactor-part4 | ||||
| Merge into: | lp:~maas-committers/maas/trunk | ||||
| Prerequisite: | lp:~mpontillo/maas/networking-constraints-refactor-part3 | ||||
| Diff against target: |
604 lines (+325/-39) 8 files modified
src/maasserver/api/nodes.py (+48/-5) src/maasserver/api/tests/test_nodes.py (+137/-11) src/maasserver/models/interface.py (+16/-0) src/maasserver/models/tests/test_interface.py (+52/-0) src/maasserver/node_constraint_filter_forms.py (+31/-11) src/maasserver/tests/test_node_constraint_filter_forms.py (+12/-12) src/maasserver/utils/orm.py (+26/-0) src/maasserver/websockets/tests/test_listener.py (+3/-0) |
||||
| To merge this branch: | bzr merge lp:~mpontillo/maas/networking-constraints-refactor-part4 | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Ricardo Bánffy (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Network interfaces constraint refactoring, part 4.
* Return a data structure from the AcquireNodeForm indicating which interfaces matched a constraint.
* Annotate the Node object returned from Nodes.acquire() with a constraints_by_type field to indicate which labels matched which constraints.
* Added a 'verbose' option to Nodes.acquire() so that users can determine the full set of nodes that matched storage or interface constraints.
* Added a 'dry_run' option to Nodes.acquire() so that users can try out constraints without actually making any changes.
* Refactor logic to compute interface/node matches into model, so that any (sub-object, object) can be mapped in the future. (interface.py, orm.py)
Description of the change
| Mike Pontillo (mpontillo) wrote : | # |
| Mike Pontillo (mpontillo) wrote : | # |
Well, I've done a "best-effort" approach to be backward-compatible and still return the new constraints data in the returned node. It's a little less-than-perfect since I don't think I should change the name of the existing storage constraint map.
| Mike Pontillo (mpontillo) wrote : | # |
Note: I still need to add unit tests for what is returned from the API. I'll do that tomorrow after we talk to the Juju team in the morning (Pacific time).
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~mpontillo/maas/networking-constraints-refactor-part4 into lp:maas failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Fetched 2,357 kB in 4s (548 kB/s)
Reading package lists...
sudo DEBIAN_
--
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~mpontillo/maas/networking-constraints-refactor-part4 into lp:maas failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Hit http://
Get:9 http://
Hit http://
Get:10 http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Fetched 2,577 kB in 4s (580 kB/s)
Reading package lists...
sudo DEBIAN_
--
Preview Diff
| 1 | === modified file 'src/maasserver/api/nodes.py' |
| 2 | --- src/maasserver/api/nodes.py 2015-10-30 20:50:01 +0000 |
| 3 | +++ src/maasserver/api/nodes.py 2015-11-05 00:32:58 +0000 |
| 4 | @@ -26,6 +26,7 @@ |
| 5 | from django.core.exceptions import PermissionDenied |
| 6 | from django.http import HttpResponse |
| 7 | from django.shortcuts import get_object_or_404 |
| 8 | +from formencode.validators import StringBool |
| 9 | from maasserver import ( |
| 10 | eventloop, |
| 11 | locks, |
| 12 | @@ -138,6 +139,7 @@ |
| 13 | 'zone', |
| 14 | 'disable_ipv4', |
| 15 | 'constraint_map', |
| 16 | + 'constraints_by_type', |
| 17 | 'boot_disk', |
| 18 | 'blockdevice_set', |
| 19 | 'physicalblockdevice_set', |
| 20 | @@ -1398,6 +1400,16 @@ |
| 21 | :type agent_name: unicode |
| 22 | :param comment: Optional comment for the event log. |
| 23 | :type comment: unicode |
| 24 | + :param dry_run: Optional boolean to indicate that the node should not |
| 25 | + actually be acquired (this is for support/troubleshooting, or users |
| 26 | + who want to see which node would match a constraint, without |
| 27 | + acquiring a node). Defaults to False. |
| 28 | + :type dry_run: bool |
| 29 | + :param verbose: Optional boolean to indicate that the user would like |
| 30 | + additional verbosity in the constraints_by_type field (each |
| 31 | + constraint will be prefixed by `verbose_`, and contain the full |
| 32 | + data structure that indicates which node(s) matched). |
| 33 | + :type verbose: bool |
| 34 | |
| 35 | Returns 409 if a suitable node matching the constraints could not be |
| 36 | found. |
| 37 | @@ -1407,6 +1419,10 @@ |
| 38 | maaslog.info( |
| 39 | "Request from user %s to acquire a node with constraints %s", |
| 40 | request.user.username, request.data) |
| 41 | + verbose = get_optional_param( |
| 42 | + request.POST, 'verbose', default=False, validator=StringBool) |
| 43 | + dry_run = get_optional_param( |
| 44 | + request.POST, 'dry_run', default=False, validator=StringBool) |
| 45 | |
| 46 | if not form.is_valid(): |
| 47 | raise MAASAPIValidationError(form.errors) |
| 48 | @@ -1416,7 +1432,7 @@ |
| 49 | with locks.node_acquire: |
| 50 | nodes = Node.nodes.get_available_nodes_for_acquisition( |
| 51 | request.user) |
| 52 | - nodes, constraint_map = form.filter_nodes(nodes) |
| 53 | + nodes, storage, interfaces = form.filter_nodes(nodes) |
| 54 | node = get_first(nodes) |
| 55 | if node is None: |
| 56 | constraints = form.describe_constraints() |
| 57 | @@ -1430,10 +1446,37 @@ |
| 58 | % constraints) |
| 59 | raise NodesNotAvailable(message) |
| 60 | agent_name = request.data.get('agent_name', '') |
| 61 | - node.acquire( |
| 62 | - request.user, get_oauth_token(request), |
| 63 | - agent_name=agent_name, comment=comment) |
| 64 | - node.constraint_map = constraint_map.get(node.id, {}) |
| 65 | + if not dry_run: |
| 66 | + node.acquire( |
| 67 | + request.user, get_oauth_token(request), |
| 68 | + agent_name=agent_name, comment=comment) |
| 69 | + node.constraint_map = storage.get(node.id, {}) |
| 70 | + node.constraints_by_type = {} |
| 71 | + # Need to get the interface constraints map into the proper format |
| 72 | + # to return it here. |
| 73 | + # Backward compatibility: provide the storage constraints in both |
| 74 | + # formats. |
| 75 | + if len(node.constraint_map) > 0: |
| 76 | + node.constraints_by_type['storage'] = {} |
| 77 | + new_storage = node.constraints_by_type['storage'] |
| 78 | + # Convert this to the "new style" constraints map format. |
| 79 | + for storage_key in node.constraint_map.iterkeys(): |
| 80 | + # Each key in the storage map is actually a value which |
| 81 | + # contains the ID of the matching storage device. |
| 82 | + # Convert this to a label: list-of-matches format, to |
| 83 | + # match how the constraints will be done going forward. |
| 84 | + new_key = node.constraint_map[storage_key] |
| 85 | + matches = new_storage.get(new_key, []) |
| 86 | + matches.append(storage_key) |
| 87 | + new_storage[new_key] = matches |
| 88 | + if len(interfaces) > 0: |
| 89 | + node.constraints_by_type['interfaces'] = { |
| 90 | + label: interfaces.get(label, {}).get(node.id) |
| 91 | + for label in interfaces |
| 92 | + } |
| 93 | + if verbose: |
| 94 | + node.constraints_by_type['verbose_storage'] = storage |
| 95 | + node.constraints_by_type['verbose_interfaces'] = interfaces |
| 96 | return node |
| 97 | |
| 98 | @admin_method |
| 99 | |
| 100 | === modified file 'src/maasserver/api/tests/test_nodes.py' |
| 101 | --- src/maasserver/api/tests/test_nodes.py 2015-10-30 14:59:16 +0000 |
| 102 | +++ src/maasserver/api/tests/test_nodes.py 2015-11-05 00:32:58 +0000 |
| 103 | @@ -80,6 +80,7 @@ |
| 104 | Equals, |
| 105 | HasLength, |
| 106 | MatchesListwise, |
| 107 | + Not, |
| 108 | ) |
| 109 | |
| 110 | |
| 111 | @@ -1139,17 +1140,142 @@ |
| 112 | """Storage label is returned alongside node data""" |
| 113 | node = factory.make_Node( |
| 114 | status=NODE_STATUS.READY, with_boot_disk=False) |
| 115 | - factory.make_PhysicalBlockDevice( |
| 116 | - node=node, size=11 * (1000 ** 3), tags=['ssd']) |
| 117 | - response = self.client.post(reverse('nodes_handler'), { |
| 118 | - 'op': 'acquire', |
| 119 | - 'storage': 'needed:10(ssd)', |
| 120 | - }) |
| 121 | - self.assertResponseCode(httplib.OK, response) |
| 122 | - response_json = json.loads(response.content) |
| 123 | - device_id = response_json['physicalblockdevice_set'][0]['id'].__str__() |
| 124 | - constraint_name = response_json['constraint_map'][device_id] |
| 125 | - self.assertItemsEqual(constraint_name, 'needed') |
| 126 | + # The ID may always be '1', which won't be interesting for testing. |
| 127 | + for _ in range(1, random.choice([1, 3, 5])): |
| 128 | + factory.make_PhysicalBlockDevice() |
| 129 | + factory.make_PhysicalBlockDevice( |
| 130 | + node=node, size=11 * (1000 ** 3), tags=['ssd']) |
| 131 | + response = self.client.post(reverse('nodes_handler'), { |
| 132 | + 'op': 'acquire', |
| 133 | + 'storage': 'needed:10(ssd)', |
| 134 | + }) |
| 135 | + self.assertResponseCode(httplib.OK, response) |
| 136 | + response_json = json.loads(response.content) |
| 137 | + device_id = response_json['physicalblockdevice_set'][0]['id'] |
| 138 | + constraint_map = response_json.get('constraint_map') |
| 139 | + constraint_name = constraint_map[unicode(device_id)] |
| 140 | + self.assertItemsEqual(constraint_name, 'needed') |
| 141 | + constraints = response_json['constraints_by_type'] |
| 142 | + self.expectThat(constraints, Contains('storage')) |
| 143 | + self.expectThat(constraints['storage'], Contains('needed')) |
| 144 | + self.expectThat(constraints['storage']['needed'], Contains(device_id)) |
| 145 | + self.expectThat(constraints, Not(Contains('verbose_storage'))) |
| 146 | + |
| 147 | + def test_POST_acquire_allocates_node_by_storage_with_verbose(self): |
| 148 | + """Storage label is returned alongside node data""" |
| 149 | + node = factory.make_Node( |
| 150 | + status=NODE_STATUS.READY, with_boot_disk=False) |
| 151 | + # The ID may always be '1', which won't be interesting for testing. |
| 152 | + for _ in range(1, random.choice([1, 3, 5])): |
| 153 | + factory.make_PhysicalBlockDevice() |
| 154 | + factory.make_PhysicalBlockDevice( |
| 155 | + node=node, size=11 * (1000 ** 3), tags=['ssd']) |
| 156 | + response = self.client.post(reverse('nodes_handler'), { |
| 157 | + 'op': 'acquire', |
| 158 | + 'storage': 'needed:10(ssd)', |
| 159 | + 'verbose': 'true', |
| 160 | + }) |
| 161 | + self.assertResponseCode(httplib.OK, response) |
| 162 | + response_json = json.loads(response.content) |
| 163 | + device_id = response_json['physicalblockdevice_set'][0]['id'] |
| 164 | + constraint_map = response_json.get('constraint_map') |
| 165 | + constraint_name = constraint_map[unicode(device_id)] |
| 166 | + self.assertItemsEqual(constraint_name, 'needed') |
| 167 | + constraints = response_json['constraints_by_type'] |
| 168 | + self.expectThat(constraints, Contains('storage')) |
| 169 | + self.expectThat(constraints['storage'], Contains('needed')) |
| 170 | + self.expectThat(constraints['storage']['needed'], Contains(device_id)) |
| 171 | + verbose_storage = constraints.get('verbose_storage') |
| 172 | + self.expectThat(verbose_storage, Contains(unicode(node.id))) |
| 173 | + self.expectThat( |
| 174 | + verbose_storage[unicode(node.id)], Equals(constraint_map)) |
| 175 | + |
| 176 | + def test_POST_acquire_allocates_node_by_interfaces(self): |
| 177 | + """Interface label is returned alongside node data""" |
| 178 | + fabric = factory.make_Fabric('ubuntu') |
| 179 | + # The ID may always be '1', which won't be interesting for testing. |
| 180 | + for _ in range(1, random.choice([1, 3, 5])): |
| 181 | + factory.make_Interface() |
| 182 | + node = factory.make_Node_with_Interface_on_Subnet( |
| 183 | + status=NODE_STATUS.READY, fabric=fabric) |
| 184 | + iface = node.get_boot_interface() |
| 185 | + response = self.client.post(reverse('nodes_handler'), { |
| 186 | + 'op': 'acquire', |
| 187 | + 'interfaces': 'needed:fabric=ubuntu', |
| 188 | + }) |
| 189 | + self.assertResponseCode(httplib.OK, response) |
| 190 | + response_json = json.loads(response.content) |
| 191 | + self.expectThat( |
| 192 | + response_json['substatus'], Equals(NODE_STATUS.ALLOCATED)) |
| 193 | + constraints = response_json['constraints_by_type'] |
| 194 | + self.expectThat(constraints, Contains('interfaces')) |
| 195 | + interfaces = constraints.get('interfaces') |
| 196 | + self.expectThat(interfaces, Contains('needed')) |
| 197 | + self.expectThat(interfaces['needed'], Contains(iface.id)) |
| 198 | + self.expectThat(constraints, Not(Contains('verbose_interfaces'))) |
| 199 | + |
| 200 | + def test_POST_acquire_allocates_node_by_interfaces_dry_run_with_verbose( |
| 201 | + self): |
| 202 | + """Interface label is returned alongside node data""" |
| 203 | + fabric = factory.make_Fabric('ubuntu') |
| 204 | + # The ID may always be '1', which won't be interesting for testing. |
| 205 | + for _ in range(1, random.choice([1, 3, 5])): |
| 206 | + factory.make_Interface() |
| 207 | + node = factory.make_Node_with_Interface_on_Subnet( |
| 208 | + status=NODE_STATUS.READY, fabric=fabric) |
| 209 | + iface = node.get_boot_interface() |
| 210 | + response = self.client.post(reverse('nodes_handler'), { |
| 211 | + 'op': 'acquire', |
| 212 | + 'interfaces': 'needed:fabric=ubuntu', |
| 213 | + 'verbose': 'true', |
| 214 | + 'dry_run': 'true', |
| 215 | + }) |
| 216 | + self.assertResponseCode(httplib.OK, response) |
| 217 | + response_json = json.loads(response.content) |
| 218 | + self.expectThat( |
| 219 | + response_json['substatus'], Equals(NODE_STATUS.READY)) |
| 220 | + # Check that we still got the verbose constraints output even if |
| 221 | + # it was a dry run. |
| 222 | + constraints = response_json['constraints_by_type'] |
| 223 | + self.expectThat(constraints, Contains('interfaces')) |
| 224 | + interfaces = constraints.get('interfaces') |
| 225 | + self.expectThat(interfaces, Contains('needed')) |
| 226 | + self.expectThat(interfaces['needed'], Contains(iface.id)) |
| 227 | + verbose_interfaces = constraints.get('verbose_interfaces') |
| 228 | + self.expectThat( |
| 229 | + verbose_interfaces['needed'], Contains(unicode(node.id))) |
| 230 | + self.expectThat( |
| 231 | + verbose_interfaces['needed'][unicode(node.id)], |
| 232 | + Contains(iface.id)) |
| 233 | + |
| 234 | + def test_POST_acquire_allocates_node_by_interfaces_with_verbose(self): |
| 235 | + """Interface label is returned alongside node data""" |
| 236 | + fabric = factory.make_Fabric('ubuntu') |
| 237 | + # The ID may always be '1', which won't be interesting for testing. |
| 238 | + for _ in range(1, random.choice([1, 3, 5])): |
| 239 | + factory.make_Interface() |
| 240 | + factory.make_Node() |
| 241 | + node = factory.make_Node_with_Interface_on_Subnet( |
| 242 | + status=NODE_STATUS.READY, fabric=fabric) |
| 243 | + iface = node.get_boot_interface() |
| 244 | + response = self.client.post(reverse('nodes_handler'), { |
| 245 | + 'op': 'acquire', |
| 246 | + 'interfaces': 'needed:fabric=ubuntu', |
| 247 | + 'verbose': 'true', |
| 248 | + }) |
| 249 | + self.assertResponseCode(httplib.OK, response) |
| 250 | + response_json = json.loads(response.content) |
| 251 | + constraints = response_json['constraints_by_type'] |
| 252 | + self.expectThat(constraints, Contains('interfaces')) |
| 253 | + interfaces = constraints.get('interfaces') |
| 254 | + self.expectThat(interfaces, Contains('needed')) |
| 255 | + self.expectThat(interfaces['needed'], Equals([iface.id])) |
| 256 | + verbose_interfaces = constraints.get('verbose_interfaces') |
| 257 | + self.expectThat( |
| 258 | + verbose_interfaces['needed'], Contains(unicode(node.id))) |
| 259 | + self.expectThat( |
| 260 | + verbose_interfaces['needed'][unicode(node.id)], |
| 261 | + Contains(iface.id)) |
| 262 | |
| 263 | def test_POST_acquire_fails_without_all_tags(self): |
| 264 | # Asking for particular tags does not acquire if no node has all tags. |
| 265 | |
| 266 | === modified file 'src/maasserver/models/interface.py' |
| 267 | --- src/maasserver/models/interface.py 2015-11-03 00:36:13 +0000 |
| 268 | +++ src/maasserver/models/interface.py 2015-11-05 00:32:58 +0000 |
| 269 | @@ -180,6 +180,22 @@ |
| 270 | current_q, |
| 271 | Q(ip_addresses__ip__isnull=True) | Q(ip_addresses__ip='')) |
| 272 | |
| 273 | + def get_matching_node_map(self, specifiers): |
| 274 | + """Returns a tuple where the first element is a set of matching node |
| 275 | + IDs, and the second element is a dictionary mapping a node ID to a list |
| 276 | + of matching interfaces, such as: |
| 277 | + |
| 278 | + { |
| 279 | + <node1>: [<interface1>, <interface2>, ...] |
| 280 | + <node2>: [<interface3>, ...] |
| 281 | + ... |
| 282 | + } |
| 283 | + |
| 284 | + :returns: tuple (set, dict) |
| 285 | + """ |
| 286 | + return super(InterfaceQueriesMixin, self).get_matching_object_map( |
| 287 | + specifiers, 'node__id') |
| 288 | + |
| 289 | |
| 290 | class InterfaceQuerySet(InterfaceQueriesMixin, QuerySet): |
| 291 | """Custom QuerySet which mixes in some additional queries specific to |
| 292 | |
| 293 | === modified file 'src/maasserver/models/tests/test_interface.py' |
| 294 | --- src/maasserver/models/tests/test_interface.py 2015-11-02 20:24:09 +0000 |
| 295 | +++ src/maasserver/models/tests/test_interface.py 2015-11-05 00:32:58 +0000 |
| 296 | @@ -333,6 +333,58 @@ |
| 297 | Interface.objects.filter_by_specifiers( |
| 298 | "mode:unconfigured")) |
| 299 | |
| 300 | + def test__get_matching_node_map(self): |
| 301 | + space1 = factory.make_Space() |
| 302 | + space2 = factory.make_Space() |
| 303 | + subnet1 = factory.make_Subnet(space=space1) |
| 304 | + subnet2 = factory.make_Subnet(space=space2) |
| 305 | + node1 = factory.make_Node_with_Interface_on_Subnet(subnet=subnet1) |
| 306 | + node2 = factory.make_Node_with_Interface_on_Subnet(subnet=subnet2) |
| 307 | + iface1 = node1.get_boot_interface() |
| 308 | + iface2 = node2.get_boot_interface() |
| 309 | + nodes1, map1 = Interface.objects.get_matching_node_map( |
| 310 | + "space:%s" % space1.name) |
| 311 | + self.assertItemsEqual(nodes1, [node1.id]) |
| 312 | + self.assertItemsEqual(map1, {node1.id: [iface1.id]}) |
| 313 | + nodes2, map2 = Interface.objects.get_matching_node_map( |
| 314 | + "space:%s" % space2.name) |
| 315 | + self.assertItemsEqual(nodes2, [node2.id]) |
| 316 | + self.assertItemsEqual(map2, {node2.id: [iface2.id]}) |
| 317 | + nodes3, map3 = Interface.objects.get_matching_node_map( |
| 318 | + ["space:%s" % space1.name, "space:%s" % space2.name]) |
| 319 | + self.assertItemsEqual(nodes3, [node1.id, node2.id]) |
| 320 | + self.assertItemsEqual(map3, { |
| 321 | + node1.id: [iface1.id], |
| 322 | + node2.id: [iface2.id], |
| 323 | + }) |
| 324 | + |
| 325 | + def test__get_matching_node_map_with_multiple_interfaces(self): |
| 326 | + space1 = factory.make_Space() |
| 327 | + space2 = factory.make_Space() |
| 328 | + subnet1 = factory.make_Subnet(space=space1) |
| 329 | + subnet2 = factory.make_Subnet(space=space2) |
| 330 | + node1 = factory.make_Node_with_Interface_on_Subnet(subnet=subnet1) |
| 331 | + node2 = factory.make_Node_with_Interface_on_Subnet(subnet=subnet2) |
| 332 | + iface1 = node1.get_boot_interface() |
| 333 | + iface2 = node2.get_boot_interface() |
| 334 | + iface3 = factory.make_Interface(node=node1) |
| 335 | + factory.make_StaticIPAddress(interface=iface3, subnet=subnet1) |
| 336 | + nodes1, map1 = Interface.objects.get_matching_node_map( |
| 337 | + "space:%s" % space1.name) |
| 338 | + self.assertItemsEqual(nodes1, [node1.id]) |
| 339 | + self.assertItemsEqual(map1, {node1.id: [iface1.id, iface3.id]}) |
| 340 | + nodes2, map2 = Interface.objects.get_matching_node_map( |
| 341 | + "space:%s" % space2.name) |
| 342 | + self.assertItemsEqual(nodes2, [node2.id]) |
| 343 | + self.assertItemsEqual(map2, {node2.id: [iface2.id]}) |
| 344 | + nodes3, map3 = Interface.objects.get_matching_node_map( |
| 345 | + ["space:%s" % space1.name, "space:%s" % space2.name]) |
| 346 | + self.assertItemsEqual(nodes3, [node1.id, node2.id]) |
| 347 | + self.assertItemsEqual(map3, { |
| 348 | + node1.id: [iface1.id, iface3.id], |
| 349 | + node2.id: [iface2.id], |
| 350 | + }) |
| 351 | + |
| 352 | |
| 353 | class InterfaceTest(MAASServerTestCase): |
| 354 | |
| 355 | |
| 356 | === modified file 'src/maasserver/node_constraint_filter_forms.py' |
| 357 | --- src/maasserver/node_constraint_filter_forms.py 2015-11-03 00:19:51 +0000 |
| 358 | +++ src/maasserver/node_constraint_filter_forms.py 2015-11-05 00:32:58 +0000 |
| 359 | @@ -382,10 +382,21 @@ |
| 360 | """Determines the set of nodes that match the specified |
| 361 | LabeledConstraintMap (which must be a map of interface constraints.) |
| 362 | |
| 363 | + Returns a dictionary in the format: |
| 364 | + { |
| 365 | + <label1>: { |
| 366 | + <node1>: [<interface1>, <interface2>, ...] |
| 367 | + <node2>: ... |
| 368 | + ... |
| 369 | + } |
| 370 | + <label2>: ... |
| 371 | + } |
| 372 | + |
| 373 | :param interfaces_label_map: LabeledConstraintMap |
| 374 | - :return: set |
| 375 | + :return: dict |
| 376 | """ |
| 377 | node_ids = None |
| 378 | + label_map = {} |
| 379 | for label in interfaces_label_map: |
| 380 | specifiers = [] |
| 381 | constraints = interfaces_label_map[label] |
| 382 | @@ -398,18 +409,22 @@ |
| 383 | if node_ids is None: |
| 384 | # The first time through the filter, build the list |
| 385 | # of candidate nodes. |
| 386 | - nodes = Interface.objects.filter_by_specifiers( |
| 387 | + node_ids, node_map = Interface.objects.get_matching_node_map( |
| 388 | specifiers) |
| 389 | - node_ids = set(nodes.values_list('node__id', flat=True)) |
| 390 | + label_map[label] = node_map |
| 391 | else: |
| 392 | # For subsequent labels, only match nodes that already matched a |
| 393 | - # preceding label. |
| 394 | - current_matching_nodes = Interface.objects.filter( |
| 395 | - node__id__in=node_ids) |
| 396 | - filtered_nodes = current_matching_nodes.filter_by_specifiers( |
| 397 | + # preceding label. Use the set intersection operator to do this, |
| 398 | + # because that will yield more complete data in the label_map. |
| 399 | + # (which is less efficient, but may be needed for troubleshooting.) |
| 400 | + # If a more efficient approach is desired, this could be changed |
| 401 | + # to filter the nodes starting from an 'id__in' filter using the |
| 402 | + # current 'node_ids' set. |
| 403 | + new_node_ids, node_map = Interface.objects.get_matching_node_map( |
| 404 | specifiers) |
| 405 | - node_ids = filtered_nodes.values_list('node__id', flat=True) |
| 406 | - return node_ids |
| 407 | + label_map[label] = node_map |
| 408 | + node_ids &= new_node_ids |
| 409 | + return node_ids, label_map |
| 410 | |
| 411 | |
| 412 | class LabeledConstraintMapField(Field): |
| 413 | @@ -801,10 +816,12 @@ |
| 414 | filtered_nodes = filtered_nodes.filter(id__in=node_ids) |
| 415 | |
| 416 | # Filter by interfaces (networking) |
| 417 | + compatible_interfaces = {} |
| 418 | interfaces_label_map = self.cleaned_data.get( |
| 419 | self.get_field_name('interfaces')) |
| 420 | if interfaces_label_map is not None: |
| 421 | - node_ids = nodes_by_interface(interfaces_label_map) |
| 422 | + node_ids, compatible_interfaces = nodes_by_interface( |
| 423 | + interfaces_label_map) |
| 424 | if node_ids is not None: |
| 425 | filtered_nodes = filtered_nodes.filter(id__in=node_ids) |
| 426 | |
| 427 | @@ -816,4 +833,7 @@ |
| 428 | # constraints. |
| 429 | filtered_nodes = filtered_nodes.distinct().extra( |
| 430 | select={'cost': "cpu_count + memory / 1024"}) |
| 431 | - return filtered_nodes.order_by("cost"), compatible_nodes |
| 432 | + return ( |
| 433 | + filtered_nodes.order_by("cost"), compatible_nodes, |
| 434 | + compatible_interfaces |
| 435 | + ) |
| 436 | |
| 437 | === modified file 'src/maasserver/tests/test_node_constraint_filter_forms.py' |
| 438 | --- src/maasserver/tests/test_node_constraint_filter_forms.py 2015-11-02 22:58:39 +0000 |
| 439 | +++ src/maasserver/tests/test_node_constraint_filter_forms.py 2015-11-05 00:32:58 +0000 |
| 440 | @@ -234,7 +234,7 @@ |
| 441 | def assertConstrainedNodes(self, nodes, data): |
| 442 | form = AcquireNodeForm(data=data) |
| 443 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 444 | - filtered_nodes, _ = form.filter_nodes(Node.objects.all()) |
| 445 | + filtered_nodes, _, _ = form.filter_nodes(Node.objects.all()) |
| 446 | self.assertItemsEqual(nodes, filtered_nodes) |
| 447 | |
| 448 | def test_no_constraints(self): |
| 449 | @@ -921,7 +921,7 @@ |
| 450 | form = AcquireNodeForm({ |
| 451 | u'fabrics': [u'fabric2']}) |
| 452 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 453 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 454 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 455 | self.assertItemsEqual([node2], filtered_nodes) |
| 456 | |
| 457 | def test_not_fabrics_constraint(self): |
| 458 | @@ -932,7 +932,7 @@ |
| 459 | form = AcquireNodeForm({ |
| 460 | u'not_fabrics': [u'fabric1']}) |
| 461 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 462 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 463 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 464 | self.assertItemsEqual([node2], filtered_nodes) |
| 465 | |
| 466 | def test_fabric_classes_constraint(self): |
| 467 | @@ -943,7 +943,7 @@ |
| 468 | form = AcquireNodeForm({ |
| 469 | u'fabric_classes': [u'1g']}) |
| 470 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 471 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 472 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 473 | self.assertItemsEqual([node2], filtered_nodes) |
| 474 | |
| 475 | def test_not_fabric_classes_constraint(self): |
| 476 | @@ -954,7 +954,7 @@ |
| 477 | form = AcquireNodeForm({ |
| 478 | u'not_fabric_classes': [u'10g']}) |
| 479 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 480 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 481 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 482 | self.assertItemsEqual([node2], filtered_nodes) |
| 483 | |
| 484 | def test_interfaces_constraint_rejected_if_syntax_is_invalid(self): |
| 485 | @@ -995,13 +995,13 @@ |
| 486 | form = AcquireNodeForm({ |
| 487 | u'interfaces': u'label:fabric_class=10g'}) |
| 488 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 489 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 490 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 491 | self.assertItemsEqual([node2], filtered_nodes) |
| 492 | |
| 493 | form = AcquireNodeForm({ |
| 494 | u'interfaces': u'label:fabric_class=1g'}) |
| 495 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 496 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 497 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 498 | self.assertItemsEqual([node1], filtered_nodes) |
| 499 | |
| 500 | def test_interfaces_filters_work_with_multiple_labels(self): |
| 501 | @@ -1017,13 +1017,13 @@ |
| 502 | form = AcquireNodeForm({ |
| 503 | u'interfaces': u'fabric:fabric_class=1g;vlan:vid=1'}) |
| 504 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 505 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 506 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 507 | self.assertItemsEqual([node1], filtered_nodes) |
| 508 | |
| 509 | form = AcquireNodeForm({ |
| 510 | u'interfaces': u'label:fabric_class=10g;vlan:vid=2'}) |
| 511 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 512 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 513 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 514 | self.assertItemsEqual([node2], filtered_nodes) |
| 515 | |
| 516 | def test_interfaces_filters_multiples_treated_as_OR_operation(self): |
| 517 | @@ -1040,14 +1040,14 @@ |
| 518 | u'interfaces': |
| 519 | u'fabric:fabric_class=1g,fabric_class=10g;vlan:vid=1'}) |
| 520 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 521 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 522 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 523 | self.assertItemsEqual([node1], filtered_nodes) |
| 524 | |
| 525 | form = AcquireNodeForm({ |
| 526 | u'interfaces': |
| 527 | u'label:fabric_class=10g,fabric_class=1g;vlan:vid=2'}) |
| 528 | self.assertTrue(form.is_valid(), dict(form.errors)) |
| 529 | - filtered_nodes, _ = form.filter_nodes(Node.nodes) |
| 530 | + filtered_nodes, _, _ = form.filter_nodes(Node.nodes) |
| 531 | self.assertItemsEqual([node2], filtered_nodes) |
| 532 | |
| 533 | def test_combined_constraints(self): |
| 534 | @@ -1186,7 +1186,7 @@ |
| 535 | # in here is the ordering. |
| 536 | form = AcquireNodeForm(data={'cpu_count': 4}) |
| 537 | self.assertTrue(form.is_valid(), form.errors) |
| 538 | - filtered_nodes, _ = form.filter_nodes(Node.objects.all()) |
| 539 | + filtered_nodes, _, _ = form.filter_nodes(Node.objects.all()) |
| 540 | self.assertEqual( |
| 541 | sorted_nodes, |
| 542 | list(filtered_nodes)) |
| 543 | |
| 544 | === modified file 'src/maasserver/utils/orm.py' |
| 545 | --- src/maasserver/utils/orm.py 2015-11-03 01:02:03 +0000 |
| 546 | +++ src/maasserver/utils/orm.py 2015-11-05 00:32:58 +0000 |
| 547 | @@ -888,3 +888,29 @@ |
| 548 | "(0-4094; 0 for untagged.)") |
| 549 | current_q = op(current_q, Q(vlan__vid=vid)) |
| 550 | return current_q |
| 551 | + |
| 552 | + def get_matching_object_map(self, specifiers, query): |
| 553 | + filter = self.filter_by_specifiers(specifiers) |
| 554 | + # We'll be looping through the list assuming a particular order later |
| 555 | + # in this function, so make sure the interfaces are grouped by their |
| 556 | + # attached nodes. |
| 557 | + matches = filter.order_by(query) |
| 558 | + matches = matches.values_list('id', query) |
| 559 | + foreign_object_map = {} |
| 560 | + object_ids = set() |
| 561 | + object_id = None |
| 562 | + foreign_object_matches = None |
| 563 | + for foreign_id, current_id in matches: |
| 564 | + if foreign_id is None: |
| 565 | + # Skip objects that do not have a corresponding foreign key. |
| 566 | + continue |
| 567 | + if current_id != object_id: |
| 568 | + # Encountered a new node ID in the list, so create an empty |
| 569 | + # list and add it to the map. (and add it to the set of matched |
| 570 | + # nodes) |
| 571 | + foreign_object_matches = [] |
| 572 | + foreign_object_map[current_id] = foreign_object_matches |
| 573 | + object_ids.add(current_id) |
| 574 | + object_id = current_id |
| 575 | + foreign_object_matches.append(foreign_id) |
| 576 | + return object_ids, foreign_object_map |
| 577 | |
| 578 | === modified file 'src/maasserver/websockets/tests/test_listener.py' |
| 579 | --- src/maasserver/websockets/tests/test_listener.py 2015-11-02 20:28:44 +0000 |
| 580 | +++ src/maasserver/websockets/tests/test_listener.py 2015-11-05 00:32:58 +0000 |
| 581 | @@ -2915,6 +2915,7 @@ |
| 582 | def test__calls_handler_with_update_on_create(self): |
| 583 | yield deferToDatabase(register_all_triggers) |
| 584 | node = yield deferToDatabase(self.create_node, self.params) |
| 585 | + yield deferToDatabase(self.create_partitiontable, {'node': node}) |
| 586 | |
| 587 | listener = PostgresListener() |
| 588 | dv = DeferredValue() |
| 589 | @@ -2932,6 +2933,7 @@ |
| 590 | def test__calls_handler_with_update_on_delete(self): |
| 591 | yield deferToDatabase(register_all_triggers) |
| 592 | node = yield deferToDatabase(self.create_node, self.params) |
| 593 | + yield deferToDatabase(self.create_partitiontable, {'node': node}) |
| 594 | filesystemgroup = yield deferToDatabase( |
| 595 | self.create_filesystemgroup, { |
| 596 | "node": node, "group_type": "raid-5"}) |
| 597 | @@ -2953,6 +2955,7 @@ |
| 598 | def test__calls_handler_with_update_on_update(self): |
| 599 | yield deferToDatabase(register_all_triggers) |
| 600 | node = yield deferToDatabase(self.create_node, self.params) |
| 601 | + yield deferToDatabase(self.create_partitiontable, {'node': node}) |
| 602 | filesystemgroup = yield deferToDatabase( |
| 603 | self.create_filesystemgroup, { |
| 604 | "node": node, "group_type": "raid-5"}) |
While this branch seems to be functionally complete, there are a lot of tests that assume the Node.acquire() API only returns two values that need to be updated, since it now returns three.
I patched the Nodes.acquire() API so that it is compatible for now, but I'd like to discuss the best way to change this for 1.9.