Merge lp:~allenap/maas/rpc-tags-region into lp:~maas-committers/maas/trunk
- rpc-tags-region
- Merge into trunk
Proposed by
Gavin Panella
| Status: | Merged |
|---|---|
| Approved by: | Gavin Panella |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 3055 |
| Proposed branch: | lp:~allenap/maas/rpc-tags-region |
| Merge into: | lp:~maas-committers/maas/trunk |
| Prerequisite: | lp:~allenap/maas/rpc-tags-cluster |
| Diff against target: |
683 lines (+414/-116) 5 files modified
src/maasserver/api/tests/test_tag.py (+31/-40) src/maasserver/models/tag.py (+3/-3) src/maasserver/models/tests/test_tag.py (+6/-54) src/maasserver/populate_tags.py (+120/-18) src/maasserver/tests/test_populate_tags.py (+254/-1) |
| To merge this branch: | bzr merge lp:~allenap/maas/rpc-tags-region |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Jeroen T. Vermeulen (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Use the EvaluateTag RPC command from the region.
Description of the change
There are problems with the approach taken in this branch. Crucially, tags won't be evaluated for clusters that are temporarily down. Additional work is needed to fix this. However, the benefit of this branch is that it is the final branch (or penultimate, depending on which lands first) before we can remove Celery from clusters (Celery is still needed for the master cluster, for DNS, but this can be run independently).
To post a comment you must log in.
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
Download full text (5.5 KiB)
Thanks for the review! It wasn't as scary as I had feared.
> Looks like the Tag API tests suffered from doing unit-testing at an
> integration-testing level. Good thing to see some tests lose the XML
> parsing. The last of the API tests you edited looks like it might
> benefit from self.expectThat().
I've switched to MatchesStructur
>
> .
>
> In the docstring for populate_tags, can you say why the return value
> is not meant to be used in production? Would there be any harm? The
> caller doesn't want or need to block on it, of course, but that sort
> of seems like the caller's problem.
I've added the following:
- You must not use the `Deferred` in the calling thread; it must
only be manipulated in the reactor thread. Pretending it's not
there is safer than chaining code onto it because it's easy to get
wrong.
- The call may not finish for 10 minutes or more. It is therefore
not a good thing to be waiting for in a web request.
>
> .
>
> The docstring for _do_populate_tags confuses me no end. Did you mean
> to rewrite it? There's also a typo in there: “oaram.”
My mistake. It's a copy-n-paste from _get_clients_
(or something like that; there's some shared history in there). Fixed, I
hope.
>
> More dangerous, of course, is the terminal preposition. Julian will
> never forgive you.
Done.
>
> .
>
> As you know I hate nontrivial nested callbacks with implicit closures.
> Was there no way to use @inlineCallbacks in _do_populate_tags?
I didn't know that. They're not nested though, and I wouldn't be able to
have control over the returned Deferred if I used inlineCallbacks. I did
try having a single nested function using inlineCallbacks, but it was no
clearer than adding callbacks. It was worse if anything. Using callbacks
with Deferreds gives a very clear control flow, and gives each function
its own namespace, helping refactoring.
>
> .
>
> Why “list((uuid, name) for (uuid, name, _) in clusters)” and not
> “[(uuid, name) for (uuid, name, _) in clusters]”? Just to avoid
> variables leaking out of the comprehension? I hate that too, but it
> doesn't seem worth our list comprehensions. To me, the diversity in
> closing brackets, braces, and parentheses provides a valuable visual
> cue of what the nesting levels are.
Okay, I've changed it. Don't like it, but I've changed it ;)
>
> .
>
> Can you add a bug number to the XXX comment in
> _get_clients_
Yes! https:/
>
> .
>
> test__returns_
> me. I thought _get_clients_
> the successfully obtained clients, and only left out clients for any
> clusters that it couldn't get clients for. The tests don't cover the
> difference between those two things AFAICS.
Okay, done.
>
> .
>
> Shouldn't test__obtains_
> clients _for the given clusters_, not just that it gets three clients?
Done.
>
> .
>
> In TestDoPopulateT
> a fake client. Saves you having to loop over the clients and modify
> th...
Revision history for this message
| Jeroen T. Vermeulen (jtv) wrote : | # |
> > Not that it's exactly pretty, but it gives an immediate overview of
> > the nesting and which part belongs at which level. It's useful when
> > you're scanning the code looking for things that you need to read more
> > closely.
>
> I've used this style for months. I find long tests (or any code) with
> lots of vertical space harder to read. My guess is that's it's something
> like: long scans/jumps up and down, missing the mark, finding my place
> again. I honestly cope better when I can keep the bulk of the test close
> together. If I find myself squinting at something I've written, I do
> spread things out, add blank lines, or comments more often. I do try not
> to obfuscate.
Sounds like we've hit on an interesting question about code legibility. I'll try to pay more attention to what my eyes are doing in these situations.
Revision history for this message
| MAAS Lander (maas-lander) wrote : | # |
There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.
Revision history for this message
| Julian Edwards (julian-edwards) wrote : | # |
On Monday 22 Sep 2014 21:27:18 you wrote:
> I've used this style for months. I find long tests (or any code) with
> lots of vertical space harder to read. My guess is that's it's something
> like: long scans/jumps up and down, missing the mark, finding my place
> again. I honestly cope better when I can keep the bulk of the test close
> together. If I find myself squinting at something I've written, I do
> spread things out, add blank lines, or comments more often. I do try not
> to obfuscate.
This is something that's hard to quantify for sure. Maybe we need to take a
reviewing decision on this and put it in the coding guidelines?
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'src/maasserver/api/tests/test_tag.py' |
| 2 | --- src/maasserver/api/tests/test_tag.py 2014-09-23 09:14:04 +0000 |
| 3 | +++ src/maasserver/api/tests/test_tag.py 2014-09-23 09:14:04 +0000 |
| 4 | @@ -28,7 +28,21 @@ |
| 5 | from maasserver.testing.factory import factory |
| 6 | from maasserver.testing.oauthclient import OAuthAuthenticatedClient |
| 7 | from maasserver.testing.orm import reload_object |
| 8 | +from maastesting.matchers import ( |
| 9 | + MockCalledOnceWith, |
| 10 | + MockCallsMatch, |
| 11 | + ) |
| 12 | from metadataserver.models.commissioningscript import inject_lshw_result |
| 13 | +from mock import ( |
| 14 | + ANY, |
| 15 | + call, |
| 16 | + ) |
| 17 | +from testtools.matchers import MatchesStructure |
| 18 | + |
| 19 | + |
| 20 | +def patch_populate_tags(test): |
| 21 | + from maasserver import populate_tags |
| 22 | + return test.patch_autospec(populate_tags, "populate_tags") |
| 23 | |
| 24 | |
| 25 | class TestTagAPI(APITestCase): |
| 26 | @@ -104,22 +118,15 @@ |
| 27 | self.assertTrue(Tag.objects.filter(name='new-tag-name').exists()) |
| 28 | |
| 29 | def test_PUT_updates_node_associations(self): |
| 30 | - self.skip("Tag evaluation is being ported to RPC.") |
| 31 | - |
| 32 | - node1 = factory.make_Node() |
| 33 | - inject_lshw_result(node1, b'<node><foo/></node>') |
| 34 | - node2 = factory.make_Node() |
| 35 | - inject_lshw_result(node2, b'<node><bar/></node>') |
| 36 | + populate_tags = patch_populate_tags(self) |
| 37 | tag = factory.make_Tag(definition='//node/foo') |
| 38 | - self.assertItemsEqual([tag.name], node1.tag_names()) |
| 39 | - self.assertItemsEqual([], node2.tag_names()) |
| 40 | + self.expectThat(populate_tags, MockCalledOnceWith(tag)) |
| 41 | self.become_admin() |
| 42 | response = self.client_put( |
| 43 | self.get_tag_uri(tag), |
| 44 | {'definition': '//node/bar'}) |
| 45 | self.assertEqual(httplib.OK, response.status_code) |
| 46 | - self.assertItemsEqual([], node1.tag_names()) |
| 47 | - self.assertItemsEqual([tag.name], node2.tag_names()) |
| 48 | + self.expectThat(populate_tags, MockCallsMatch(call(tag), call(tag))) |
| 49 | |
| 50 | def test_GET_nodes_with_no_nodes(self): |
| 51 | tag = factory.make_Tag() |
| 52 | @@ -143,14 +150,13 @@ |
| 53 | [r['system_id'] for r in parsed_result]) |
| 54 | |
| 55 | def test_GET_nodes_hides_invisible_nodes(self): |
| 56 | - self.skip("Tag evaluation is being ported to RPC.") |
| 57 | - |
| 58 | user2 = factory.make_User() |
| 59 | node1 = factory.make_Node() |
| 60 | - inject_lshw_result(node1, b'<node><foo/></node>') |
| 61 | node2 = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=user2) |
| 62 | - inject_lshw_result(node2, b'<node><bar/></node>') |
| 63 | - tag = factory.make_Tag(definition='//node') |
| 64 | + tag = factory.make_Tag() |
| 65 | + node1.tags.add(tag) |
| 66 | + node2.tags.add(tag) |
| 67 | + |
| 68 | response = self.client.get(self.get_tag_uri(tag), {'op': 'nodes'}) |
| 69 | |
| 70 | self.assertEqual(httplib.OK, response.status_code) |
| 71 | @@ -166,12 +172,11 @@ |
| 72 | [r['system_id'] for r in parsed_result]) |
| 73 | |
| 74 | def test_PUT_invalid_definition(self): |
| 75 | - self.skip("Tag evaluation is being ported to RPC.") |
| 76 | - |
| 77 | self.become_admin() |
| 78 | node = factory.make_Node() |
| 79 | inject_lshw_result(node, b'<node ><child/></node>') |
| 80 | tag = factory.make_Tag(definition='//child') |
| 81 | + node.tags.add(tag) |
| 82 | self.assertItemsEqual([tag.name], node.tag_names()) |
| 83 | response = self.client_put( |
| 84 | self.get_tag_uri(tag), {'definition': 'invalid::tag'}) |
| 85 | @@ -338,24 +343,15 @@ |
| 86 | self.assertItemsEqual([], node.tags.all()) |
| 87 | |
| 88 | def test_POST_rebuild_rebuilds_node_mapping(self): |
| 89 | - self.skip("Tag evaluation is being ported to RPC.") |
| 90 | - |
| 91 | + populate_tags = patch_populate_tags(self) |
| 92 | tag = factory.make_Tag(definition='//foo/bar') |
| 93 | - # Only one node matches the tag definition, rebuilding should notice |
| 94 | - node_matching = factory.make_Node() |
| 95 | - inject_lshw_result(node_matching, b'<foo><bar/></foo>') |
| 96 | - node_bogus = factory.make_Node() |
| 97 | - inject_lshw_result(node_bogus, b'<foo/>') |
| 98 | - node_matching.tags.add(tag) |
| 99 | - node_bogus.tags.add(tag) |
| 100 | - self.assertItemsEqual( |
| 101 | - [node_matching, node_bogus], tag.node_set.all()) |
| 102 | self.become_admin() |
| 103 | + self.assertThat(populate_tags, MockCalledOnceWith(tag)) |
| 104 | response = self.client.post(self.get_tag_uri(tag), {'op': 'rebuild'}) |
| 105 | self.assertEqual(httplib.OK, response.status_code) |
| 106 | parsed_result = json.loads(response.content) |
| 107 | self.assertEqual({'rebuilding': tag.name}, parsed_result) |
| 108 | - self.assertItemsEqual([node_matching], tag.node_set.all()) |
| 109 | + self.assertThat(populate_tags, MockCallsMatch(call(tag), call(tag))) |
| 110 | |
| 111 | def test_POST_rebuild_leaves_manual_tags(self): |
| 112 | tag = factory.make_Tag(definition='') |
| 113 | @@ -491,16 +487,8 @@ |
| 114 | extra_kernel_opts, Tag.objects.filter(name=name)[0].kernel_opts) |
| 115 | |
| 116 | def test_POST_new_populates_nodes(self): |
| 117 | - self.skip("Tag evaluation is being ported to RPC.") |
| 118 | - |
| 119 | + populate_tags = patch_populate_tags(self) |
| 120 | self.become_admin() |
| 121 | - node1 = factory.make_Node() |
| 122 | - inject_lshw_result(node1, b'<node><child/></node>') |
| 123 | - # Create another node that doesn't have a 'child' |
| 124 | - node2 = factory.make_Node() |
| 125 | - inject_lshw_result(node2, b'<node/>') |
| 126 | - self.assertItemsEqual([], node1.tag_names()) |
| 127 | - self.assertItemsEqual([], node2.tag_names()) |
| 128 | name = factory.make_string() |
| 129 | definition = '//node/child' |
| 130 | comment = factory.make_string() |
| 131 | @@ -513,5 +501,8 @@ |
| 132 | 'definition': definition, |
| 133 | }) |
| 134 | self.assertEqual(httplib.OK, response.status_code) |
| 135 | - self.assertItemsEqual([name], node1.tag_names()) |
| 136 | - self.assertItemsEqual([], node2.tag_names()) |
| 137 | + self.assertThat(populate_tags, MockCalledOnceWith(ANY)) |
| 138 | + # The tag passed to populate_tags() is the one created above. |
| 139 | + [tag], _ = populate_tags.call_args |
| 140 | + self.assertThat(tag, MatchesStructure.byEquality( |
| 141 | + name=name, comment=comment, definition=definition)) |
| 142 | |
| 143 | === modified file 'src/maasserver/models/tag.py' |
| 144 | --- src/maasserver/models/tag.py 2014-09-23 09:14:04 +0000 |
| 145 | +++ src/maasserver/models/tag.py 2014-09-23 09:14:04 +0000 |
| 146 | @@ -111,9 +111,9 @@ |
| 147 | raise ValidationError({'definition': [msg]}) |
| 148 | # Now delete the existing tags |
| 149 | self.node_set.clear() |
| 150 | - # Being ported to RPC. |
| 151 | - # from maasserver.populate_tags import populate_tags |
| 152 | - # populate_tags(self) |
| 153 | + # Avoid circular imports. |
| 154 | + from maasserver.populate_tags import populate_tags |
| 155 | + populate_tags(self) |
| 156 | |
| 157 | def save(self, *args, **kwargs): |
| 158 | super(Tag, self).save(*args, **kwargs) |
| 159 | |
| 160 | === modified file 'src/maasserver/models/tests/test_tag.py' |
| 161 | --- src/maasserver/models/tests/test_tag.py 2014-09-23 09:14:04 +0000 |
| 162 | +++ src/maasserver/models/tests/test_tag.py 2014-09-23 09:14:04 +0000 |
| 163 | @@ -20,7 +20,6 @@ |
| 164 | from maasserver.testing.factory import factory |
| 165 | from maasserver.testing.testcase import MAASServerTestCase |
| 166 | from maastesting.matchers import MockCalledOnceWith |
| 167 | -from metadataserver.models.commissioningscript import inject_lshw_result |
| 168 | |
| 169 | |
| 170 | class TagTest(MAASServerTestCase): |
| 171 | @@ -67,60 +66,15 @@ |
| 172 | self.assertRaises(ValidationError, factory.make_Tag, name=invalid) |
| 173 | |
| 174 | def test_applies_tags_to_nodes(self): |
| 175 | - self.skip("Tag evaluation is being ported to RPC.") |
| 176 | - |
| 177 | - node1 = factory.make_Node() |
| 178 | - inject_lshw_result(node1, b'<node><child /></node>') |
| 179 | - node2 = factory.make_Node() |
| 180 | - inject_lshw_result(node2, b'<node />') |
| 181 | + populate_tags = self.patch_autospec( |
| 182 | + populate_tags_module, "populate_tags") |
| 183 | tag = factory.make_Tag(definition='//node/child') |
| 184 | - self.assertItemsEqual([tag.name], node1.tag_names()) |
| 185 | - self.assertItemsEqual([], node2.tag_names()) |
| 186 | - |
| 187 | - def test_removes_old_values(self): |
| 188 | - self.skip("Tag evaluation is being ported to RPC.") |
| 189 | - |
| 190 | - node1 = factory.make_Node() |
| 191 | - inject_lshw_result(node1, b'<node><foo /></node>') |
| 192 | - node2 = factory.make_Node() |
| 193 | - inject_lshw_result(node2, b'<node><bar /></node>') |
| 194 | - tag = factory.make_Tag(definition='//node/foo') |
| 195 | - self.assertItemsEqual([tag.name], node1.tag_names()) |
| 196 | - self.assertItemsEqual([], node2.tag_names()) |
| 197 | - tag.definition = '//node/bar' |
| 198 | - tag.save() |
| 199 | - self.assertItemsEqual([], node1.tag_names()) |
| 200 | - self.assertItemsEqual([tag.name], node2.tag_names()) |
| 201 | - # And we notice if we change it *again* and then save. |
| 202 | - tag.definition = '//node/foo' |
| 203 | - tag.save() |
| 204 | - self.assertItemsEqual([tag.name], node1.tag_names()) |
| 205 | - self.assertItemsEqual([], node2.tag_names()) |
| 206 | - |
| 207 | - def test_doesnt_touch_other_tags(self): |
| 208 | - self.skip("Tag evaluation is being ported to RPC.") |
| 209 | - |
| 210 | - node1 = factory.make_Node() |
| 211 | - inject_lshw_result(node1, b'<node><foo /></node>') |
| 212 | - node2 = factory.make_Node() |
| 213 | - inject_lshw_result(node2, b'<node><bar /></node>') |
| 214 | - tag1 = factory.make_Tag(definition='//node/foo') |
| 215 | - self.assertItemsEqual([tag1.name], node1.tag_names()) |
| 216 | - self.assertItemsEqual([], node2.tag_names()) |
| 217 | - tag2 = factory.make_Tag(definition='//node/bar') |
| 218 | - self.assertItemsEqual([tag1.name], node1.tag_names()) |
| 219 | - self.assertItemsEqual([tag2.name], node2.tag_names()) |
| 220 | - |
| 221 | - def test_rollsback_invalid_xpath(self): |
| 222 | - self.skip("Tag evaluation is being ported to RPC.") |
| 223 | - |
| 224 | - node = factory.make_Node() |
| 225 | - inject_lshw_result(node, b'<node><foo /></node>') |
| 226 | - tag = factory.make_Tag(definition='//node/foo') |
| 227 | - self.assertItemsEqual([tag.name], node.tag_names()) |
| 228 | + self.assertThat(populate_tags, MockCalledOnceWith(tag)) |
| 229 | + |
| 230 | + def test_will_not_save_invalid_xpath(self): |
| 231 | + tag = factory.make_Tag(definition='//node/foo') |
| 232 | tag.definition = 'invalid::tag' |
| 233 | self.assertRaises(ValidationError, tag.save) |
| 234 | - self.assertItemsEqual([tag.name], node.tag_names()) |
| 235 | |
| 236 | |
| 237 | class TestTagIsDefined(MAASServerTestCase): |
| 238 | @@ -167,8 +121,6 @@ |
| 239 | self.assertItemsEqual([], tag.node_set.all()) |
| 240 | |
| 241 | def test__calls_populate_tags(self): |
| 242 | - self.skip("Tag evaluation is being ported to RPC.") |
| 243 | - |
| 244 | populate_tags = self.patch_autospec( |
| 245 | populate_tags_module, "populate_tags") |
| 246 | |
| 247 | |
| 248 | === modified file 'src/maasserver/populate_tags.py' |
| 249 | --- src/maasserver/populate_tags.py 2014-09-23 09:14:04 +0000 |
| 250 | +++ src/maasserver/populate_tags.py 2014-09-23 09:14:04 +0000 |
| 251 | @@ -18,6 +18,7 @@ |
| 252 | ] |
| 253 | |
| 254 | from functools import partial |
| 255 | +from itertools import izip |
| 256 | |
| 257 | from lxml import etree |
| 258 | from maasserver import logger |
| 259 | @@ -26,10 +27,23 @@ |
| 260 | get_single_probed_details, |
| 261 | script_output_nsmap, |
| 262 | ) |
| 263 | -from maasserver.refresh_worker import refresh_worker |
| 264 | +from maasserver.rpc import getClientFor |
| 265 | +from provisioningserver.logger import get_maas_logger |
| 266 | +from provisioningserver.rpc.cluster import EvaluateTag |
| 267 | from provisioningserver.tags import merge_details |
| 268 | from provisioningserver.utils import classify |
| 269 | +from provisioningserver.utils.twisted import ( |
| 270 | + asynchronous, |
| 271 | + FOREVER, |
| 272 | + synchronous, |
| 273 | + ) |
| 274 | from provisioningserver.utils.xpath import try_match_xpath |
| 275 | +from twisted.internet.defer import DeferredList |
| 276 | +from twisted.python import log |
| 277 | + |
| 278 | + |
| 279 | +maaslog = get_maas_logger("tags") |
| 280 | + |
| 281 | |
| 282 | # The nsmap that XPath expression must be compiled with. This will |
| 283 | # ensure that expressions like //lshw:something will work correctly. |
| 284 | @@ -39,24 +53,112 @@ |
| 285 | } |
| 286 | |
| 287 | |
| 288 | +@synchronous |
| 289 | def populate_tags(tag): |
| 290 | - """Send worker for all nodegroups an update_node_tags request. |
| 291 | - """ |
| 292 | - items = { |
| 293 | - 'tag_name': tag.name, |
| 294 | - 'tag_definition': tag.definition, |
| 295 | - 'tag_nsmap': tag_nsmap, |
| 296 | - } |
| 297 | - # Rather than using NodeGroup.objects.refresh_workers() we call |
| 298 | - # refresh_worker immediately before we pass the requet. This is mostly for |
| 299 | - # the test suite, where we need the single real worker to switch to the |
| 300 | - # worker for a given nodegroup, before we have that worker process the |
| 301 | - # request. |
| 302 | - logger.debug('Refreshing tag definition for %s' % (items,)) |
| 303 | - for nodegroup in NodeGroup.objects.all(): |
| 304 | - refresh_worker(nodegroup) |
| 305 | - raise NotImplementedError( |
| 306 | - "Tag evaluation is being ported to RPC.") |
| 307 | + """Evaluate `tag` for all nodes. |
| 308 | + |
| 309 | + This returns a `Deferred` that will fire when all tags have been |
| 310 | + evaluated. This is intended FOR TESTING ONLY because: |
| 311 | + |
| 312 | + - You must not use the `Deferred` in the calling thread; it must only be |
| 313 | + manipulated in the reactor thread. Pretending it's not there is safer |
| 314 | + than chaining code onto it because it's easy to get wrong. |
| 315 | + |
| 316 | + - The call may not finish for 10 minutes or more. It is therefore not a |
| 317 | + good thing to be waiting for in a web request. |
| 318 | + |
| 319 | + """ |
| 320 | + logger.debug('Evaluating the "%s" tag for all nodes', tag.name) |
| 321 | + clusters = tuple( |
| 322 | + (nodegroup.uuid, nodegroup.cluster_name, nodegroup.api_credentials) |
| 323 | + for nodegroup in NodeGroup.objects.all_accepted()) |
| 324 | + [d] = _do_populate_tags(clusters, tag.name, tag.definition, tag_nsmap) |
| 325 | + return d |
| 326 | + |
| 327 | + |
| 328 | +@asynchronous(timeout=FOREVER) |
| 329 | +def _do_populate_tags(clusters, tag_name, tag_definition, tag_nsmap): |
| 330 | + """Send RPC calls to each cluster, requesting evaluation of tags. |
| 331 | + |
| 332 | + :param clusters: A sequence of ``(uuid, cluster-name, creds)`` tuples for |
| 333 | + each cluster. The name is used for logging only. |
| 334 | + :param tag_name: The name of the tag being evaluated. |
| 335 | + :param tag_definition: The definition of the tag, an XPath expression. |
| 336 | + :oaram tag_nsmap: The NS mapping used to compile the expression. |
| 337 | + """ |
| 338 | + creds = {uuid: creds for uuid, _, creds in clusters} |
| 339 | + tag_nsmap_on_wire = [ |
| 340 | + {"prefix": prefix, "uri": uri} |
| 341 | + for prefix, uri in tag_nsmap.viewitems() |
| 342 | + ] |
| 343 | + |
| 344 | + def make_call(client): |
| 345 | + return client( |
| 346 | + EvaluateTag, tag_name=tag_name, tag_definition=tag_definition, |
| 347 | + tag_nsmap=tag_nsmap_on_wire, credentials=creds[client.ident]) |
| 348 | + |
| 349 | + def evaluate_tags(clients): |
| 350 | + # Call EvaluateTag on each cluster concurrently. |
| 351 | + return DeferredList( |
| 352 | + (make_call(client) for client in clients), |
| 353 | + consumeErrors=True) |
| 354 | + |
| 355 | + def check_results(results): |
| 356 | + for (uuid, name, creds), (success, result) in izip(clusters, results): |
| 357 | + if success: |
| 358 | + maaslog.info( |
| 359 | + "Tag %s (%s) evaluated on cluster %s (%s)", tag_name, |
| 360 | + tag_definition, name, uuid) |
| 361 | + else: |
| 362 | + maaslog.error( |
| 363 | + "Tag %s (%s) could not be evaluated on cluster %s (%s): " |
| 364 | + "%s", tag_name, tag_definition, name, uuid, |
| 365 | + result.getErrorMessage()) |
| 366 | + |
| 367 | + d = _get_clients_for_populating_tags( |
| 368 | + [(uuid, name) for (uuid, name, _) in clusters], tag_name) |
| 369 | + d.addCallback(evaluate_tags) |
| 370 | + d.addCallback(check_results) |
| 371 | + d.addErrback(log.err) |
| 372 | + |
| 373 | + # Do *not* return a Deferred; the caller is not meant to wait around for |
| 374 | + # this to finish. However, for the sake of testing, we return the Deferred |
| 375 | + # wrapped up in a list so that crochet does not block. |
| 376 | + return [d] |
| 377 | + |
| 378 | + |
| 379 | +@asynchronous(timeout=FOREVER) |
| 380 | +def _get_clients_for_populating_tags(clusters, tag_name): |
| 381 | + """Obtain RPC clients for the given clusters. |
| 382 | + |
| 383 | + :param clusters: A sequence of ``(uuid, name)`` tuples for each cluster. |
| 384 | + The name is used for logging only. |
| 385 | + :param tag_name: The tag for which these clients are being obtained. This |
| 386 | + is used only for logging. |
| 387 | + """ |
| 388 | + # Wait up to `timeout` seconds to obtain clients to all clusters. |
| 389 | + d = DeferredList( |
| 390 | + (getClientFor(uuid, timeout=30) for uuid, _ in clusters), |
| 391 | + consumeErrors=True) |
| 392 | + |
| 393 | + def got_clients(results): |
| 394 | + clients = [] |
| 395 | + for (uuid, name), (success, result) in izip(clusters, results): |
| 396 | + if success: |
| 397 | + clients.append(result) |
| 398 | + else: |
| 399 | + # XXX: allenap 2014-09-22 bug=1372544: Filtering out |
| 400 | + # unavailable (or otherwise broken) clients means that tags |
| 401 | + # aren't going to be evaluated for that cluster's nodes. |
| 402 | + # That's bad. It would be good to be able to ask another |
| 403 | + # cluster to evaluate them, but the cluster-side code needs |
| 404 | + # changing to allow that. For now, we just skip. |
| 405 | + maaslog.warning( |
| 406 | + "Cannot evaluate tag %s on cluster %s (%s): %s", |
| 407 | + tag_name, name, uuid, result.getErrorMessage()) |
| 408 | + return clients |
| 409 | + |
| 410 | + return d.addCallback(got_clients) |
| 411 | |
| 412 | |
| 413 | def populate_tags_for_single_node(tags, node): |
| 414 | |
| 415 | === modified file 'src/maasserver/tests/test_populate_tags.py' |
| 416 | --- src/maasserver/tests/test_populate_tags.py 2014-09-23 09:14:04 +0000 |
| 417 | +++ src/maasserver/tests/test_populate_tags.py 2014-09-23 09:14:04 +0000 |
| 418 | @@ -14,11 +14,264 @@ |
| 419 | __metaclass__ = type |
| 420 | __all__ = [] |
| 421 | |
| 422 | +from itertools import izip |
| 423 | + |
| 424 | +from fixtures import FakeLogger |
| 425 | +from maasserver import populate_tags as populate_tags_module |
| 426 | +from maasserver.enum import NODEGROUP_STATUS |
| 427 | from maasserver.models import Tag |
| 428 | -from maasserver.populate_tags import populate_tags_for_single_node |
| 429 | +from maasserver.populate_tags import ( |
| 430 | + _do_populate_tags, |
| 431 | + _get_clients_for_populating_tags, |
| 432 | + populate_tags, |
| 433 | + populate_tags_for_single_node, |
| 434 | + ) |
| 435 | +from maasserver.rpc.testing.fixtures import MockLiveRegionToClusterRPCFixture |
| 436 | +from maasserver.testing.eventloop import ( |
| 437 | + RegionEventLoopFixture, |
| 438 | + RunningEventLoopFixture, |
| 439 | + ) |
| 440 | from maasserver.testing.factory import factory |
| 441 | from maasserver.testing.testcase import MAASServerTestCase |
| 442 | +from maastesting.matchers import MockCalledOnceWith |
| 443 | from metadataserver.models import commissioningscript |
| 444 | +from mock import ( |
| 445 | + ANY, |
| 446 | + create_autospec, |
| 447 | + sentinel, |
| 448 | + ) |
| 449 | +from provisioningserver.rpc.cluster import EvaluateTag |
| 450 | +from provisioningserver.rpc.common import Client |
| 451 | +from provisioningserver.rpc.testing import ( |
| 452 | + always_fail_with, |
| 453 | + always_succeed_with, |
| 454 | + ) |
| 455 | +from provisioningserver.utils.twisted import asynchronous |
| 456 | +from testtools.deferredruntest import extract_result |
| 457 | +from testtools.monkey import MonkeyPatcher |
| 458 | +from twisted.internet import defer |
| 459 | + |
| 460 | + |
| 461 | +def make_accepted_NodeGroup(): |
| 462 | + return factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED) |
| 463 | + |
| 464 | + |
| 465 | +def make_Tag_without_populating(): |
| 466 | + # Create a tag but prevent evaluation when saving. |
| 467 | + dont_populate = MonkeyPatcher((Tag, "populate_nodes", lambda self: None)) |
| 468 | + return dont_populate.run_with_patches(factory.make_Tag) |
| 469 | + |
| 470 | + |
| 471 | +class TestGetClientsForPopulatingTags(MAASServerTestCase): |
| 472 | + |
| 473 | + def test__returns_no_clients_when_there_are_no_clusters(self): |
| 474 | + tag_name = factory.make_name("tag") |
| 475 | + clients = _get_clients_for_populating_tags([], tag_name) |
| 476 | + self.assertEqual([], clients) |
| 477 | + |
| 478 | + def patch_getClientFor(self): |
| 479 | + return self.patch_autospec(populate_tags_module, "getClientFor") |
| 480 | + |
| 481 | + def test__returns_no_clients_when_there_is_an_error(self): |
| 482 | + nodegroup_with_connection = make_accepted_NodeGroup() |
| 483 | + nodegroup_without_connection = make_accepted_NodeGroup() |
| 484 | + |
| 485 | + def getClientFor(uuid, timeout): |
| 486 | + if uuid == nodegroup_with_connection.uuid: |
| 487 | + return defer.succeed(sentinel.client) |
| 488 | + else: |
| 489 | + return defer.fail(ZeroDivisionError()) |
| 490 | + self.patch_getClientFor().side_effect = getClientFor |
| 491 | + |
| 492 | + tag_name = factory.make_name("tag") |
| 493 | + clusters = [ |
| 494 | + (nodegroup_with_connection.uuid, |
| 495 | + nodegroup_with_connection.cluster_name), |
| 496 | + (nodegroup_without_connection.uuid, |
| 497 | + nodegroup_without_connection.cluster_name), |
| 498 | + ] |
| 499 | + clients = _get_clients_for_populating_tags(clusters, tag_name) |
| 500 | + self.assertEqual([sentinel.client], clients) |
| 501 | + |
| 502 | + def test__logs_errors_obtaining_clients(self): |
| 503 | + getClientFor = self.patch_getClientFor() |
| 504 | + getClientFor.side_effect = always_fail_with( |
| 505 | + ZeroDivisionError("an error message one would surmise")) |
| 506 | + nodegroup = make_accepted_NodeGroup() |
| 507 | + tag_name = factory.make_name("tag") |
| 508 | + clusters = [(nodegroup.uuid, nodegroup.cluster_name)] |
| 509 | + with FakeLogger("maas") as log: |
| 510 | + _get_clients_for_populating_tags(clusters, tag_name) |
| 511 | + self.assertDocTestMatches( |
| 512 | + "Cannot evaluate tag ... on cluster ... (...): ... surmise", |
| 513 | + log.output) |
| 514 | + |
| 515 | + def test__waits_for_clients_for_30_seconds_by_default(self): |
| 516 | + getClientFor = self.patch_getClientFor() |
| 517 | + getClientFor.side_effect = always_succeed_with(sentinel.client) |
| 518 | + nodegroup = make_accepted_NodeGroup() |
| 519 | + tag_name = factory.make_name("tag") |
| 520 | + clusters = [(nodegroup.uuid, nodegroup.cluster_name)] |
| 521 | + clients = _get_clients_for_populating_tags(clusters, tag_name) |
| 522 | + self.assertEqual([sentinel.client], clients) |
| 523 | + self.assertThat( |
| 524 | + getClientFor, MockCalledOnceWith( |
| 525 | + nodegroup.uuid, timeout=30)) |
| 526 | + |
| 527 | + def test__obtains_multiple_clients(self): |
| 528 | + getClientFor = self.patch_getClientFor() |
| 529 | + # Return a 2-tuple as a stand-in for a real client. |
| 530 | + getClientFor.side_effect = lambda uuid, timeout: ( |
| 531 | + defer.succeed((sentinel.client, uuid))) |
| 532 | + nodegroups = [make_accepted_NodeGroup() for _ in xrange(3)] |
| 533 | + tag_name = factory.make_name("tag") |
| 534 | + clusters = [(ng.uuid, ng.cluster_name) for ng in nodegroups] |
| 535 | + clients = _get_clients_for_populating_tags(clusters, tag_name) |
| 536 | + self.assertItemsEqual( |
| 537 | + [(sentinel.client, nodegroup.uuid) for nodegroup in nodegroups], |
| 538 | + clients) |
| 539 | + |
| 540 | + |
| 541 | +class TestDoPopulateTags(MAASServerTestCase): |
| 542 | + |
| 543 | + def patch_clients(self, nodegroups): |
| 544 | + clients = [create_autospec(Client, instance=True) for _ in nodegroups] |
| 545 | + for nodegroup, client in izip(nodegroups, clients): |
| 546 | + client.side_effect = always_succeed_with(None) |
| 547 | + client.ident = nodegroup.uuid |
| 548 | + |
| 549 | + _get_clients = self.patch_autospec( |
| 550 | + populate_tags_module, "_get_clients_for_populating_tags") |
| 551 | + _get_clients.return_value = defer.succeed(clients) |
| 552 | + |
| 553 | + return clients |
| 554 | + |
| 555 | + def test__makes_calls_to_each_client_given(self): |
| 556 | + nodegroups = [make_accepted_NodeGroup() for _ in xrange(3)] |
| 557 | + clients = self.patch_clients(nodegroups) |
| 558 | + |
| 559 | + tag_name = factory.make_name("tag") |
| 560 | + tag_definition = factory.make_name("definition") |
| 561 | + tag_nsmap_prefix = factory.make_name("prefix") |
| 562 | + tag_nsmap_uri = factory.make_name("uri") |
| 563 | + tag_nsmap = {tag_nsmap_prefix: tag_nsmap_uri} |
| 564 | + |
| 565 | + clusters = list( |
| 566 | + (ng.uuid, ng.cluster_name, ng.api_credentials) |
| 567 | + for ng in nodegroups) |
| 568 | + |
| 569 | + [d] = _do_populate_tags( |
| 570 | + clusters, tag_name, tag_definition, tag_nsmap) |
| 571 | + |
| 572 | + self.assertIsNone(extract_result(d)) |
| 573 | + |
| 574 | + for nodegroup, client in izip(nodegroups, clients): |
| 575 | + self.expectThat(client, MockCalledOnceWith( |
| 576 | + EvaluateTag, tag_name=tag_name, tag_definition=tag_definition, |
| 577 | + tag_nsmap=[{"prefix": tag_nsmap_prefix, "uri": tag_nsmap_uri}], |
| 578 | + credentials=nodegroup.api_credentials)) |
| 579 | + |
| 580 | + def test__logs_successes(self): |
| 581 | + nodegroups = [make_accepted_NodeGroup()] |
| 582 | + self.patch_clients(nodegroups) |
| 583 | + |
| 584 | + tag_name = factory.make_name("tag") |
| 585 | + tag_definition = factory.make_name("definition") |
| 586 | + tag_nsmap = {} |
| 587 | + |
| 588 | + clusters = list( |
| 589 | + (ng.uuid, ng.cluster_name, ng.api_credentials) |
| 590 | + for ng in nodegroups) |
| 591 | + |
| 592 | + with FakeLogger("maas") as log: |
| 593 | + [d] = _do_populate_tags( |
| 594 | + clusters, tag_name, tag_definition, tag_nsmap) |
| 595 | + self.assertIsNone(extract_result(d)) |
| 596 | + |
| 597 | + self.assertDocTestMatches( |
| 598 | + "Tag tag-... (definition-...) evaluated on cluster ... (...)", |
| 599 | + log.output) |
| 600 | + |
| 601 | + def test__logs_failures(self): |
| 602 | + nodegroups = [make_accepted_NodeGroup()] |
| 603 | + [client] = self.patch_clients(nodegroups) |
| 604 | + client.side_effect = always_fail_with( |
| 605 | + ZeroDivisionError("splendid day for a spot of cricket")) |
| 606 | + |
| 607 | + tag_name = factory.make_name("tag") |
| 608 | + tag_definition = factory.make_name("definition") |
| 609 | + tag_nsmap = {} |
| 610 | + |
| 611 | + clusters = list( |
| 612 | + (ng.uuid, ng.cluster_name, ng.api_credentials) |
| 613 | + for ng in nodegroups) |
| 614 | + |
| 615 | + with FakeLogger("maas") as log: |
| 616 | + [d] = _do_populate_tags( |
| 617 | + clusters, tag_name, tag_definition, tag_nsmap) |
| 618 | + self.assertIsNone(extract_result(d)) |
| 619 | + |
| 620 | + self.assertDocTestMatches( |
| 621 | + "Tag tag-... (definition-...) could not be evaluated ... (...): " |
| 622 | + "splendid day for a spot of cricket", log.output) |
| 623 | + |
| 624 | + |
| 625 | +class TestPopulateTags(MAASServerTestCase): |
| 626 | + |
| 627 | + def patch_do_populate_tags(self): |
| 628 | + do_populate_tags = self.patch_autospec( |
| 629 | + populate_tags_module, "_do_populate_tags") |
| 630 | + do_populate_tags.return_value = [sentinel.d] |
| 631 | + return do_populate_tags |
| 632 | + |
| 633 | + def test__calls_do_populate_tags_with_no_clusters(self): |
| 634 | + do_populate_tags = self.patch_do_populate_tags() |
| 635 | + tag = make_Tag_without_populating() |
| 636 | + populate_tags(tag) |
| 637 | + self.assertThat(do_populate_tags, MockCalledOnceWith( |
| 638 | + (), tag.name, tag.definition, populate_tags_module.tag_nsmap)) |
| 639 | + |
| 640 | + def test__calls_do_populate_tags_with_clusters(self): |
| 641 | + do_populate_tags = self.patch_do_populate_tags() |
| 642 | + nodegroups = [make_accepted_NodeGroup() for _ in xrange(3)] |
| 643 | + tag = make_Tag_without_populating() |
| 644 | + populate_tags(tag) |
| 645 | + clusters_expected = tuple( |
| 646 | + (ng.uuid, ng.cluster_name, ng.api_credentials) |
| 647 | + for ng in nodegroups) |
| 648 | + self.assertThat(do_populate_tags, MockCalledOnceWith( |
| 649 | + clusters_expected, tag.name, tag.definition, |
| 650 | + populate_tags_module.tag_nsmap)) |
| 651 | + |
| 652 | + |
| 653 | +class TestPopulateTagsEndToNearlyEnd(MAASServerTestCase): |
| 654 | + |
| 655 | + def prepare_live_rpc(self): |
| 656 | + self.useFixture(RegionEventLoopFixture("rpc")) |
| 657 | + self.useFixture(RunningEventLoopFixture()) |
| 658 | + return self.useFixture(MockLiveRegionToClusterRPCFixture()) |
| 659 | + |
| 660 | + def test__calls_are_made_to_all_clusters(self): |
| 661 | + rpc_fixture = self.prepare_live_rpc() |
| 662 | + nodegroups = [make_accepted_NodeGroup() for _ in xrange(3)] |
| 663 | + protocols = [] |
| 664 | + for nodegroup in nodegroups: |
| 665 | + protocol = rpc_fixture.makeCluster(nodegroup, EvaluateTag) |
| 666 | + protocol.EvaluateTag.side_effect = always_succeed_with({}) |
| 667 | + protocols.append(protocol) |
| 668 | + tag = make_Tag_without_populating() |
| 669 | + |
| 670 | + d = populate_tags(tag) |
| 671 | + |
| 672 | + # `d` is a testing-only convenience. We must wait for it to fire, and |
| 673 | + # we must do that from the reactor thread. |
| 674 | + wait_for_populate = asynchronous(lambda: d) |
| 675 | + wait_for_populate().wait(10) |
| 676 | + |
| 677 | + for nodegroup, protocol in izip(nodegroups, protocols): |
| 678 | + self.expectThat(protocol.EvaluateTag, MockCalledOnceWith( |
| 679 | + protocol, tag_name=tag.name, tag_definition=tag.definition, |
| 680 | + tag_nsmap=ANY, credentials=nodegroup.api_credentials)) |
| 681 | |
| 682 | |
| 683 | class TestPopulateTagsForSingleNode(MAASServerTestCase): |
Looks like the Tag API tests suffered from doing unit-testing at an integration-testing level. Good thing to see some tests lose the XML parsing. The last of the API tests you edited looks like it might benefit from self.expectThat().
.
In the docstring for populate_tags, can you say why the return value is not meant to be used in production? Would there be any harm? The caller doesn't want or need to block on it, of course, but that sort of seems like the caller's problem.
.
The docstring for _do_populate_tags confuses me no end. Did you mean to rewrite it? There's also a typo in there: “oaram.”
More dangerous, of course, is the terminal preposition. Julian will never forgive you.
.
As you know I hate nontrivial nested callbacks with implicit closures. Was there no way to use @inlineCallbacks in _do_populate_tags?
.
Why “list((uuid, name) for (uuid, name, _) in clusters)” and not “[(uuid, name) for (uuid, name, _) in clusters]”? Just to avoid variables leaking out of the comprehension? I hate that too, but it doesn't seem worth our list comprehensions. To me, the diversity in closing brackets, braces, and parentheses provides a valuable visual cue of what the nesting levels are.
.
Can you add a bug number to the XXX comment in _get_clients_
.
test__returns_
.
Shouldn't test__obtains_
.
In TestDoPopulateT
.
Could TestPopulateTag
.
I find line breaks like these inconsistent with what we do elsewhere:
(), tag.name, tag.definition, populate_
for nodegroup, protocol in izip(nodegroups, protocols):
If you're going to line-break the arguments list to self.assertThat “inside one of the arguments,” I think that still makes it a multi-line arguments list:
...