launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #12953
[Merge] lp:~jameinel/maas/populate-node-rebuilds into lp:maas
John A Meinel has proposed merging lp:~jameinel/maas/populate-node-rebuilds into lp:maas.
Commit message:
Hook up Tag.populate_nodes to actually perform the work in the provisioning_server.
Lots of changes to the testing infrastructure to make that work
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jameinel/maas/populate-node-rebuilds/+merge/128096
This sort of got out of hand, but I got things working. I'm looking for some feedback as to how to make this as tasteful as we can.
I can try to split this up if it helps reviews. But the basic layering is:
1) I want to update maasserver.models.tags.Tag.populate_nodes so that it farms the work out to the provisioning server.
2) I did that by going via maasserver.populate_tags as an indirection, so that the model code wouldn't directly be calling celery tasks.
3) While doing this I did find quite a few bugs in the provisioningserver.tags code, because of inconsistencies in what we thought was the right API to call, and what was the exact api to call. (nodegroups, not nodegroup, '/api' for the Django stuff, MAASClient is very permissive and put '/' at the beginning if it isn't there.)
4) MAASDjangoTestClient is a key bit that gives you a MAASClient that actually talks to the OAuthAuthenticatedClient directly to Django code without going through an HTTP layer.
5) It is a little difficult to inject (4), but we got it working.
6) It is also tricky because in the 'real world' you want to have N provisioning servers, each with their own nodegroup uuids, and workers, and credentials. However, all of that code is written with global state. The thing that made it work, is if you call 'refresh_secrets' immediately before asking for any work to be done, the current worker effectively switches to whatever nodegroup you want.
7) With the caveat that you have to inject the MAASDjangoTestClient in there to get it to actually work during the test suite.
For now, I went with a 'patch_tags_api()' helper on TestCase that needs to be called if you want to do anything with factory.make_tag(). I'd really prefer a different solution (one option is to just always call it, given that CeleryFixture is already always active.)
Another option is that we just patch out populate_tags directly during the test suite (we have code that can evaluate tags easily with direct db access). It certainly simplifies the setup of everything, but given the failures I saw when we just use Mock for testing, I really don't want to use Mock as much as possible.
8) I think it would be good to change the provisioning server test to use more MAASDjangoTestClient rather than Mock. It would mean that we actually test against the URLs that we actually want to be running against, and we won't have to worry about API skew.
--
https://code.launchpad.net/~jameinel/maas/populate-node-rebuilds/+merge/128096
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/populate-node-rebuilds into lp:maas.
=== added file 'src/apiclient/testing/django_client_proxy.py'
--- src/apiclient/testing/django_client_proxy.py 1970-01-01 00:00:00 +0000
+++ src/apiclient/testing/django_client_proxy.py 2012-10-04 19:36:23 +0000
@@ -0,0 +1,29 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""A proxy that looks like MAASClient.
+
+This actually passes the requests on to a django.test.client.Client, to avoid
+having to go via a real HTTP server.
+"""
+
+
+class MAASDjangoTestClient:
+ """Wrap the Django testing Client to look liek a MAASClient."""
+
+ def __init__(self, django_client):
+ self.django_client = django_client
+
+ def get(self, path, op=None, **kwargs):
+ kwargs['op'] = op
+ return self.django_client.get(path, kwargs)
+
+ def post(self, path, op=None, **kwargs):
+ kwargs['op'] = op
+ return self.django_client.post(path, kwargs)
+
+ def put(self, path, **kwargs):
+ return self.django_client.put(path, kwargs)
+
+ def delete(self, path):
+ return self.django_client.delete(path)
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-10-04 10:08:26 +0000
+++ src/maasserver/api.py 2012-10-04 19:36:23 +0000
@@ -1395,7 +1395,8 @@
if not request.user.is_superuser:
uuid = request.POST.get('nodegroup', None)
if uuid is None:
- raise PermissionDenied()
+ raise PermissionDenied(
+ 'Must be a superuser or supply a nodegroup')
nodegroup = get_one(NodeGroup.objects.filter(uuid=uuid))
check_nodegroup_access(request, nodegroup)
nodes_to_add = self._get_nodes_for(request, 'add', nodegroup)
=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py 2012-10-02 09:31:47 +0000
+++ src/maasserver/models/tag.py 2012-10-04 19:36:23 +0000
@@ -116,22 +116,17 @@
def populate_nodes(self):
"""Find all nodes that match this tag, and update them."""
- # Local import to avoid circular reference
- from maasserver.models import Node
- # First make sure we have a valid definition
+ from maasserver.populate_tags import populate_tags
+ # before we pass of any work, we ensure the tag definition is valid
+ # XPATH
try:
- # Many documents, one definition: use XPath.
- xpath = etree.XPath(self.definition)
+ etree.XPath(self.definition)
except etree.XPathSyntaxError as e:
msg = 'Invalid xpath expression: %s' % (e,)
raise ValidationError({'definition': [msg]})
# Now delete the existing tags
self.node_set.clear()
- # And figure out what matches the new definition
- for node in Node.objects.filter(hardware_details__isnull=False):
- doc = etree.XML(node.hardware_details)
- if xpath(doc):
- node.tags.add(self)
+ populate_tags(self)
def save(self, *args, **kwargs):
super(Tag, self).save(*args, **kwargs)
=== added file 'src/maasserver/populate_tags.py'
--- src/maasserver/populate_tags.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/populate_tags.py 2012-10-04 19:36:23 +0000
@@ -0,0 +1,33 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Populate what nodes are associated with a tag."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'populate_tags',
+ ]
+
+from maasserver.models import NodeGroup
+from maasserver.refresh_worker import refresh_worker
+from provisioningserver.tasks import update_node_tags
+
+
+def populate_tags(tag):
+ """Send worker for all nodegroups an update_node_tags request.
+ """
+ items = {
+ 'tag_name': tag.name,
+ 'tag_definition': tag.definition,
+ }
+ # NodeGroup.objects.refresh_workers()
+ for nodegroup in NodeGroup.objects.all():
+ refresh_worker(nodegroup)
+ update_node_tags.apply_async(queue=nodegroup.work_queue, kwargs=items)
+
=== modified file 'src/maasserver/testing/testcase.py'
--- src/maasserver/testing/testcase.py 2012-08-28 08:34:39 +0000
+++ src/maasserver/testing/testcase.py 2012-10-04 19:36:23 +0000
@@ -21,6 +21,7 @@
from maasserver.testing.factory import factory
from maastesting.celery import CeleryFixture
import maastesting.djangotestcase
+from provisioningserver.testing.tags import install_test_tag_cached_knowledge
from provisioningserver.testing.worker_cache import WorkerCacheFixture
@@ -33,6 +34,9 @@
self.addCleanup(django_cache.clear)
self.celery = self.useFixture(CeleryFixture())
+ def patch_tags_api(self):
+ install_test_tag_cached_knowledge(self)
+
class TestModelTestCase(TestCase,
maastesting.djangotestcase.TestModelTestCase):
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-10-04 12:08:36 +0000
+++ src/maasserver/tests/test_api.py 2012-10-04 19:36:23 +0000
@@ -937,6 +937,7 @@
self.assertEqual(node.system_id, parsed_result['system_id'])
def test_GET_returns_associated_tag(self):
+ self.patch_tags_api()
node = factory.make_node(set_hostname=True)
tag = factory.make_tag()
node.tags.add(tag)
@@ -1837,6 +1838,7 @@
self.assertResponseCode(httplib.BAD_REQUEST, response)
def test_POST_acquire_allocates_node_by_tags(self):
+ self.patch_tags_api()
# Asking for particular tags acquires a node with those tags.
node = factory.make_node(status=NODE_STATUS.READY)
node_tag_names = ["fast", "stable", "cute"]
@@ -1850,6 +1852,7 @@
self.assertEqual(node_tag_names, response_json['tag_names'])
def test_POST_acquire_fails_without_all_tags(self):
+ self.patch_tags_api()
# Asking for particular tags does not acquire if no node has all tags.
node1 = factory.make_node(status=NODE_STATUS.READY)
node1.tags = [factory.make_tag(t) for t in ("fast", "stable", "cute")]
@@ -1862,6 +1865,7 @@
self.assertResponseCode(httplib.CONFLICT, response)
def test_POST_acquire_fails_with_unknown_tags(self):
+ self.patch_tags_api()
# Asking for a tag that does not exist gives a specific error.
node = factory.make_node(status=NODE_STATUS.READY)
node.tags = [factory.make_tag("fast")]
@@ -2337,6 +2341,10 @@
class TestTagAPI(APITestCase):
"""Tests for /api/1.0/tags/<tagname>/."""
+ def setUp(self):
+ super(TestTagAPI, self).setUp()
+ self.patch_tags_api()
+
def get_tag_uri(self, tag):
"""Get the API URI for `tag`."""
return self.get_uri('tags/%s/') % tag.name
@@ -2628,6 +2636,10 @@
class TestTagsAPI(APITestCase):
+ def setUp(self):
+ super(TestTagsAPI, self).setUp()
+ self.patch_tags_api()
+
def test_GET_list_without_tags_returns_empty_list(self):
response = self.client.get(self.get_uri('tags/'), {'op': 'list'})
self.assertItemsEqual([], json.loads(response.content))
=== added file 'src/maasserver/tests/test_populate_tags.py'
--- src/maasserver/tests/test_populate_tags.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_populate_tags.py 2012-10-04 19:36:23 +0000
@@ -0,0 +1,55 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the code that refreshes a node-group worker's information."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = []
+
+import mock
+from maasserver import populate_tags as populate_tags_module
+from maasserver.populate_tags import populate_tags
+from maasserver.testing.factory import factory
+from maasserver.testing.testcase import TestCase
+from maastesting.fakemethod import FakeMethod
+from provisioningserver import (
+ tasks,
+ tags,
+ )
+
+
+class TestPopulateTags(TestCase):
+
+ def setUp(self):
+ super(TestPopulateTags, self).setUp()
+ self.patch_tags_api()
+
+ def test_populate_tags_task_routed_to_nodegroup_worker(self):
+ nodegroup = factory.make_node_group()
+ tag = factory.make_tag()
+ task = self.patch(populate_tags_module, 'update_node_tags')
+ populate_tags(tag)
+ args, kwargs = task.apply_async.call_args
+ self.assertEqual(nodegroup.work_queue, kwargs['queue'])
+
+ def test_populate_tags_task_routed_to_all_nodegroup_workers(self):
+ nodegroups = [factory.make_node_group() for i in range(5)]
+ tag = factory.make_tag()
+ refresh = self.patch(populate_tags_module, 'refresh_worker')
+ task = self.patch(populate_tags_module, 'update_node_tags')
+ populate_tags(tag)
+ refresh_calls = [mock.call(nodegroup) for nodegroup in nodegroups]
+ refresh.assert_has_calls(refresh_calls, any_order=True)
+ task_calls = [mock.call(queue=nodegroup.work_queue,
+ kwargs={
+ 'tag_name': tag.name,
+ 'tag_definition': tag.definition,
+ })
+ for nodegroup in nodegroups]
+ task.apply_async.assert_has_calls(task_calls, any_order=True)
=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py 2012-10-02 07:26:01 +0000
+++ src/maasserver/tests/test_tag.py 2012-10-04 19:36:23 +0000
@@ -20,6 +20,10 @@
class TagTest(TestCase):
+ def setUp(self):
+ super(TagTest, self).setUp()
+ self.patch_tags_api()
+
def test_factory_make_tag(self):
"""
The generated system_id looks good.
=== modified file 'src/provisioningserver/tags.py'
--- src/provisioningserver/tags.py 2012-10-04 09:45:43 +0000
+++ src/provisioningserver/tags.py 2012-10-04 19:36:23 +0000
@@ -5,6 +5,7 @@
"""
+import httplib
import json
from lxml import etree
@@ -48,6 +49,14 @@
return client, nodegroup_uuid
+def process_response(response):
+ """All responses should be httplib.OK and contain JSON content."""
+ if response.status_code != httplib.OK:
+ text_status = httplib.responses.get(response.status_code, '<unknown>')
+ raise AssertionError('Unexpected HTTP status: %s %s, expected 200 OK'
+ % (response.status_code, text_status))
+ return json.loads(response.content)
+
def get_nodes_for_node_group(client, nodegroup_uuid):
"""Retrieve the UUIDs of nodes in a particular group.
@@ -55,10 +64,8 @@
:param nodegroup_uuid: Node group for which to retrieve nodes
:return: List of UUIDs for nodes in nodegroup
"""
- path = 'api/1.0/nodegroup/%s/' % (nodegroup_uuid)
- response = client.get(path, op='list_nodes')
- # XXX: Check the response code before we parse the content
- return json.loads(response.content)
+ path = '/api/1.0/nodegroups/%s/' % (nodegroup_uuid)
+ return process_response(client.get(path, op='list_nodes'))
def get_hardware_details_for_nodes(client, nodegroup_uuid, system_ids):
@@ -68,11 +75,9 @@
:param system_ids: List of UUIDs of systems for which to fetch lshw data
:return: Dictionary mapping node UUIDs to lshw output
"""
- path = 'api/1.0/nodegroup/%s/' % (nodegroup_uuid,)
- response = client.get(
- path, op='node_hardware_details', system_ids=system_ids)
- # XXX: Check the response code before we parse the content
- return json.loads(response.content)
+ path = '/api/1.0/nodegroups/%s/' % (nodegroup_uuid,)
+ return process_response(client.get(
+ path, op='node_hardware_details', system_ids=system_ids))
def update_node_tags(client, tag_name, uuid, added, removed):
@@ -86,10 +91,9 @@
:param added: Set of nodes to add
:param removed: Set of nodes to remove
"""
- path = 'api/1.0/tags/%s/' % (tag_name,)
- response = client.post(path, op='update_nodes', add=added, remove=removed)
- # XXX: Check the response code before we parse the content
- return json.loads(response.content)
+ path = '/api/1.0/tags/%s/' % (tag_name,)
+ return process_response(client.post(
+ path, op='update_nodes', nodegroup=uuid, add=added, remove=removed))
def process_batch(xpath, hardware_details):
@@ -99,8 +103,17 @@
matched_nodes = []
unmatched_nodes = []
for system_id, hw_xml in hardware_details:
- xml = etree.XML(hw_xml)
- if xpath(xml):
+ matched = False
+ if hw_xml is not None:
+ try:
+ xml = etree.XML(hw_xml)
+ except etree.XMLSyntaxError as e:
+ task_logger.debug('Invalid hardware_details for %s: %s'
+ % (system_id, e))
+ else:
+ if xpath(xml):
+ matched = True
+ if matched:
matched_nodes.append(system_id)
else:
unmatched_nodes.append(system_id)
=== added file 'src/provisioningserver/testing/tags.py'
--- src/provisioningserver/testing/tags.py 1970-01-01 00:00:00 +0000
+++ src/provisioningserver/testing/tags.py 2012-10-04 19:36:23 +0000
@@ -0,0 +1,40 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+
+from apiclient.testing.django_client_proxy import MAASDjangoTestClient
+from maasserver.models import NodeGroup
+from maasserver.testing.oauthclient import OAuthAuthenticatedClient
+from maasserver.utils.orm import get_one
+from maasserver.worker_user import get_worker_user
+from provisioningserver import tags
+
+
+def get_nodegroup_cached_knowledge():
+ """Get a MAASClient and nodegroup_uuid.
+
+ We make use of the fact that populate_tags refreshes the secrets before it
+ starts doing work. So effectively the single real-world worker changes
+ workers on each iteration of the loop.
+
+ The MAASDjangoTestClient that is returned proxies to the Django testing
+ Client, so we don't actually have to make HTTP calls.
+ """
+ nodegroup_uuid = tags.get_recorded_nodegroup_uuid()
+ maas_client = get_nodegroup_worker_client(nodegroup_uuid)
+ return maas_client, nodegroup_uuid
+
+
+def get_nodegroup_worker_client(nodegroup_uuid):
+ """Get a MAASClient that can do work for this nodegroup."""
+ nodegroup = get_one(NodeGroup.objects.filter(uuid=nodegroup_uuid))
+ django_client = OAuthAuthenticatedClient(
+ get_worker_user(), token=nodegroup.api_token)
+ maas_client = MAASDjangoTestClient(django_client)
+ return maas_client
+
+
+def install_test_tag_cached_knowledge(test):
+ """Install the get_nodegroup_cached_knowledge for this test."""
+ from provisioningserver import tags
+ test.patch(tags, 'get_cached_knowledge', get_nodegroup_cached_knowledge)
=== modified file 'src/provisioningserver/tests/test_tags.py'
--- src/provisioningserver/tests/test_tags.py 2012-10-04 09:45:43 +0000
+++ src/provisioningserver/tests/test_tags.py 2012-10-04 19:36:23 +0000
@@ -74,7 +74,7 @@
self.patch(client, 'get', mock)
result = tags.get_nodes_for_node_group(client, uuid)
self.assertEqual(['system-id1', 'system-id2'], result)
- url = 'api/1.0/nodegroup/%s/' % (uuid,)
+ url = '/api/1.0/nodegroups/%s/' % (uuid,)
mock.assert_called_once_with(url, op='list_nodes')
def test_get_hardware_details_calls_correct_api_and_parses_result(self):
@@ -87,7 +87,7 @@
result = tags.get_hardware_details_for_nodes(
client, uuid, ['system-id1', 'system-id2'])
self.assertEqual([['system-id1', xml_data]], result)
- url = 'api/1.0/nodegroup/%s/' % (uuid,)
+ url = '/api/1.0/nodegroups/%s/' % (uuid,)
mock.assert_called_once_with(
url, op='node_hardware_details',
system_ids=["system-id1", "system-id2"])
@@ -96,15 +96,15 @@
client, uuid = self.fake_cached_knowledge()
content = '{"added": 1, "removed": 2}'
response = FakeResponse(httplib.OK, content)
- mock = MagicMock(return_value=response)
- self.patch(client, 'post', mock)
+ post_mock = MagicMock(return_value=response)
+ self.patch(client, 'post', post_mock)
name = factory.make_name('tag')
result = tags.update_node_tags(client, name, uuid,
['add-system-id'], ['remove-1', 'remove-2'])
self.assertEqual({'added': 1, 'removed': 2}, result)
- url = 'api/1.0/tags/%s/' % (name,)
- mock.assert_called_once_with(
- url, op='update_nodes',
+ url = '/api/1.0/tags/%s/' % (name,)
+ post_mock.assert_called_once_with(
+ url, op='update_nodes', nodegroup=uuid,
add=['add-system-id'], remove=['remove-1', 'remove-2'])
def test_process_batch_evaluates_xpath(self):
@@ -118,6 +118,13 @@
(['a', 'c'], ['b']),
tags.process_batch(xpath, node_details))
+ def test_process_batch_handles_invalid_hardware(self):
+ xpath = etree.XPath('//node')
+ details = [['a', ''], ['b', 'not-xml'], ['c', None]]
+ self.assertEqual(
+ ([], ['a', 'b', 'c']),
+ tags.process_batch(xpath, details))
+
def test_process_node_tags_no_secrets(self):
self.patch(MAASClient, 'get')
self.patch(MAASClient, 'post')
@@ -141,8 +148,8 @@
tag_name = factory.make_name('tag')
nodegroup_uuid = get_recorded_nodegroup_uuid()
tags.process_node_tags(tag_name, '//node')
- nodegroup_url = 'api/1.0/nodegroup/%s/' % (nodegroup_uuid,)
- tag_url = 'api/1.0/tags/%s/' % (tag_name,)
+ nodegroup_url = '/api/1.0/nodegroups/%s/' % (nodegroup_uuid,)
+ tag_url = '/api/1.0/tags/%s/' % (tag_name,)
self.assertEqual([((nodegroup_url,), {'op': 'list_nodes'})],
get_nodes.calls)
self.assertEqual([((nodegroup_url,),
@@ -151,6 +158,7 @@
get_hw_details.calls)
self.assertEqual([((tag_url,),
{'op': 'update_nodes',
+ 'nodegroup': nodegroup_uuid,
'add': ['system-id1'],
'remove': ['system-id2'],
})], post_fake.calls)
