Merge lp:~mpontillo/maas/node-statistics-bug-1584926--part1 into lp:maas/trunk

Proposed by Mike Pontillo on 2016-06-16
Status: Rejected
Rejected by: MAAS Lander on 2017-06-22
Proposed branch: lp:~mpontillo/maas/node-statistics-bug-1584926--part1
Merge into: lp:maas/trunk
Diff against target: 588 lines (+444/-22)
5 files modified
src/maasserver/api/nodes.py (+86/-9)
src/maasserver/api/support.py (+83/-0)
src/maasserver/api/tests/test_machines.py (+40/-5)
src/maasserver/api/tests/test_nodes.py (+234/-7)
src/maasserver/testing/testclient.py (+1/-1)
To merge this branch: bzr merge lp:~mpontillo/maas/node-statistics-bug-1584926--part1
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2016-06-16 Needs Fixing on 2016-06-17
Review via email: mp+297679@code.launchpad.net

Commit message

Node API usability enhancements, part 1.

 * Refactor APIs to support summaries and counts more easily.
   - <nodes|machines|devices|region-controllers|rack-controllers> read:
     now supports the 'count' and 'summary' parameters.
 * Add new API to fetch machines using a "group by":
   - nodes by-type [summary=<bool> | count=<bool>]

To post a comment you must log in.
Mike Pontillo (mpontillo) wrote :

I moved the changes for the devices and machines API to a separate branch. This branch is now just the refactoring needed to support summaries and counts, and the change in the nodes API to use that refactoring.

Gavin Panella (allenap) wrote :

This is useful functionality but the implementation is overcomplicated.

Almost all that's needed is a function to figure out what to show:

  SHOW_COUNT, SHOW_SUMMARY = "count", "summary"

  def get_show_option(request):
      summary = get_optional_param(
          request.GET, 'summary', default=False, validator=StringBool)
      count = get_optional_param(
          request.GET, 'count', default=False, validator=StringBool)
      if count:
          return SHOW_COUNT
      elif summary:
          return SHOW_SUMMARY
      else:
          return None

(I've said elsewhere that I think a single `show` parameter would be
better than two separate parameters, but that's a separate point.)

Then, in each handler method:

  def read(self, request):
      query = ...
      show = get_show_option(request)
      if show == SHOW_COUNT:
          return query.count()
      elif show == SHOW_SUMMARY:
          return list(map(self.summarize, query))
      else:
          return query.prefetch(...)

There's no need for a framework to support this.

The code added to OperationsHandlerMixin makes it hard to follow what's
going on. It's convoluted, and OperationsHandlerMixin would be the wrong
place for it anyway.

One day it might conceivably be necessary to have some more frameworky
support for this behaviour, but not now, and this wouldn't be it. As is,
this ties us closer to Piston, and it's not well structured.

The prefetch and summary methods are not too bad, but where they're
placed is too general. Prefetching can vary; it's unlikely that a whole
handler, which can process GET, POST, DELETE, PUT and custom operations,
is always going to have the same needs for prefetching. Same goes for
summaries.

result_summary, query_or_count, and prepare_results don't fit together
well. The first especially, which has two isinstance checks to see if it
should actually get out of the way, begging the question: why is it
being called at all? Both query_or_count and result_summary could be
eliminated by small changes to prepare_results:

  def prepare_results(self, query, count=False, summary=False):
      if count:
          return query.count()
      elif summary:
          query = self._prefetch_for_summary_dump(query)
          return list(map(self._summarize, query))
      else:
          return self._prefetch_for_full_dump(query)

Even now prepare_results would be in the wrong place; every handler in
MAAS would get this method, but it's not so generally useful. And it's
so simple that it's not necessary; each handler method can figure this
stuff out on its own and not have to adopt this API. If a very clear
pattern emerges then maybe it's worth extracting a new method to do it,
but today is not that day.

I've put up a branch that removes the OperationsHandlerMixin code, adds
a get_show_option function, updates the handlers, and fixes a few broken
tests here:

  lp~allenap/maas/mpontillo--node-statistics-bug-1584926--part1

I did this to prove to myself that I wasn't talking nonsense above, but
it's good enough to use, and I think you should use it.

review: Needs Fixing
Blake Rouse (blake-rouse) wrote :

I also think this is getting rather large and not something that we should land in MAAS 2.0. We need to get rc1 today, this will just add more bug surface.

Lets target this for MAAS 2.1. Since this is a feature and not a bug, and it was requested basically in the MAAS 2.1 cycle we should be fixing it there and not in MAAS 2.0.

Mike Pontillo (mpontillo) wrote :

I'm fine with landing this in 2.1.

Gavin, did you look at the part2 branch? Because that was where the pattern emerged that led me to the refactoring (which I thought could one day be generally useful). But I'm fine with tabling this until we can have that design discussion. That was the main reason I asked you to look at this branch, to be honest.

Mike Pontillo (mpontillo) wrote :

Gavin, thanks for doing the derived branch.

I like what you did with get_show_option(), and I think it's debatable what the exact format is. We should agree on something future-proof because we'll have to live with it in 2.1, and any future APIs that mimic this functionality.

I also like your suggestion to simplify prepare_results.

I don't agree that my changes to the Mixin were too general. I think, while the code you wrote for by-type is easier to read, it's highly redundant if repeated 4 more times in the machines API (again, see part2).

So the question in my mind is, if OperationsHandlerMixin isn't the right place for it (and I somewhat suspected it wasn't), what is? Can we create another mixin class? I was on the fence, since I do feel it could be generally useful throughout the entire API.

Gavin Panella (allenap) wrote :
Download full text (3.2 KiB)

> Gavin, did you look at the part2 branch? Because that was where the
> pattern emerged that led me to the refactoring (which I thought could
> one day be generally useful). But I'm fine with tabling this until we
> can have that design discussion. That was the main reason I asked you
> to look at this branch, to be honest.

I did look at it briefly and didn't see anything that really changed my
mind. I have since looked at it in greater depth and seen something that
makes it even clearer that it's not a good pattern...

>
> ...
>
> I like what you did with get_show_option(), and I think it's debatable
> what the exact format is. We should agree on something future-proof
> because we'll have to live with it in 2.1, and any future APIs that
> mimic this functionality.

As I've said, I prefer a single parameter like show=[full|summary|count]
for example, because it's clearer (versus 2 booleans = 4 possibilities
to select only 3 options) and more concise. But, I grant it's only a
little thing, and it'll be wrapped up behind a client library for most
folk anyway.

>
> I also like your suggestion to simplify prepare_results.
>
> I don't agree that my changes to the Mixin were too general. I think,
> while the code you wrote for by-type is easier to read, it's highly
> redundant if repeated 4 more times in the machines API (again, see
> part2).

Putting it in a mix-in at all is too general; it is relevant only to GET
and POST+op, not to DELETE, PUT, or POST.

> So the question in my mind is, if OperationsHandlerMixin isn't the
> right place for it (and I somewhat suspected it wasn't), what is? Can
> we create another mixin class? I was on the fence, since I do feel it
> could be generally useful throughout the entire API.

The problematic thing I discovered is related to performance.

Dumping a node of any shade to JSON is inefficient already. Despite
prefetching there are a still a lot of queries performed for each node.
(There's plenty of work needed already to fix these kinds of issues,
should anyone want to do it.)

The prepare_results() pattern makes this worse. An example from
DevicesHandler.by_parent:

  return {
      node.system_id if by_system_id else node.hostname:
          self.prepare_results(
              self._query(request).filter(parent=node),
              count=count, summary=summary)
      for node in Node.objects.filter(children__isnull=False)
  }

Given a single machine with two child devices, a call to by_parent (not
summary, not count) needs 37 queries.

But given _two_ machines with two child devices each, a similar call to
by_parent needs 57 queries. That's 20 extra queries for 3 nodes. A MAAS
install of 100+ nodes might need 500 queries or more to service
by_parent.

For a summary the overhead is 8 queries per extra machine with two
children, and the overhead for a count is 2 queries.

I have spent some time trying to improve these figures but Django and
Piston do get in the way. Sadly, so does prepare_results, including the
alternate version I wrote. It's not a good fit however it's implemented.

I strongly think we need to start by writing these methods out the long
way, paying attention to performance too (with tests...

Read more...

MAAS Lander (maas-lander) wrote :

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

5141. By Mike Pontillo on 2016-06-16

Revert changes to devices.py and machines.py. (they'll be moved to another branch)

5140. By Mike Pontillo on 2016-06-16

Fix lint, review comments.

5139. By Mike Pontillo on 2016-06-16

More tests.

5138. By Mike Pontillo on 2016-06-16

Start adding test cases.

5137. By Mike Pontillo on 2016-06-15

Format imports.

5136. By Mike Pontillo on 2016-06-15

Add 'devices by-parent [by_system_id=<bool>'.

5135. By Mike Pontillo on 2016-06-15

Refactor result_summary() to be a little more defensive.

5134. By Mike Pontillo on 2016-06-15

Clean up API docs.

5133. By Mike Pontillo on 2016-06-15

Missed an occurance of ModelCountMixin.

5132. By Mike Pontillo on 2016-06-15

Remove ModelCountMixin. (using read with a parameter seems to make more sense)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/nodes.py'
2--- src/maasserver/api/nodes.py 2016-05-24 13:50:54 +0000
3+++ src/maasserver/api/nodes.py 2016-06-16 19:59:56 +0000
4@@ -15,6 +15,7 @@
5 import bson
6 from django.http import HttpResponse
7 from django.shortcuts import get_object_or_404
8+from formencode.validators import StringBool
9 from maasserver.api.support import (
10 admin_method,
11 AnonymousOperationsHandler,
12@@ -47,7 +48,13 @@
13 Node,
14 OwnerData,
15 )
16-from maasserver.models.node import typecast_to_node_type
17+from maasserver.models.node import (
18+ Device,
19+ Machine,
20+ RackController,
21+ RegionController,
22+ typecast_to_node_type,
23+)
24 from maasserver.models.nodeprobeddetails import get_single_probed_details
25 from piston3.utils import rc
26 from provisioningserver.power.schema import UNKNOWN_POWER_TYPE
27@@ -281,6 +288,21 @@
28 anonymous = AnonNodesHandler
29 base_model = Node
30
31+ def prefetch(self, nodes):
32+ nodes = nodes.prefetch_related('interface_set__node')
33+ nodes = nodes.prefetch_related('interface_set__ip_addresses')
34+ nodes = nodes.prefetch_related('tags')
35+ nodes = nodes.prefetch_related('zone')
36+ return nodes
37+
38+ def _query(self, request):
39+ """Helper method to filter the list of machines.
40+
41+ Based on the request, the list will be filtered based on the currently
42+ logged in user, and whatever search criteria are supplied.
43+ """
44+ return filtered_nodes_list_from_request(request, model=self.base_model)
45+
46 def read(self, request):
47 """List Nodes visible to the user, optionally filtered by criteria.
48
49@@ -311,27 +333,42 @@
50 :param agent_name: An optional agent name. Only nodes relating to the
51 nodes with matching agent names will be returned.
52 :type agent_name: unicode
53+
54+ :param summary: If True, prints a summary of each node instead of the
55+ complete object.
56+ :type summary: bool
57+
58+ :param count: If True, prints a count of nodes instead of any
59+ node details.
60+ :type count: bool
61 """
62+ summary = get_optional_param(
63+ request.GET, 'summary', default=False, validator=StringBool)
64+ count = get_optional_param(
65+ request.GET, 'count', default=False, validator=StringBool)
66+ return self.prepare_results(
67+ self._read(
68+ request, summary=summary, count=count),
69+ count=count, summary=summary)
70
71- if self.base_model == Node:
72+ def _read(self, request, summary=False, count=False):
73+ """List nodes visible to the user."""
74+ if self.base_model == Node and not summary and not count:
75 # Avoid circular dependencies
76 from maasserver.api.devices import DevicesHandler
77 from maasserver.api.machines import MachinesHandler
78 from maasserver.api.rackcontrollers import RackControllersHandler
79 nodes = list(chain(
80- DevicesHandler().read(request).order_by("id"),
81- MachinesHandler().read(request).order_by("id"),
82- RackControllersHandler().read(request).order_by("id"),
83+ DevicesHandler().read(request),
84+ MachinesHandler().read(request),
85+ RackControllersHandler().read(request),
86 ))
87 return nodes
88 else:
89 nodes = filtered_nodes_list_from_request(request, self.base_model)
90 # Prefetch related objects that are needed for rendering the
91 # result.
92- nodes = nodes.prefetch_related('interface_set__node')
93- nodes = nodes.prefetch_related('interface_set__ip_addresses')
94- nodes = nodes.prefetch_related('tags')
95- nodes = nodes.prefetch_related('zone')
96+ nodes = self.prefetch(nodes)
97 return nodes.order_by('id')
98
99 @admin_method
100@@ -356,6 +393,46 @@
101 raise MAASAPIValidationError(form.errors)
102 form.save()
103
104+ def summary(self, node):
105+ """Returns a summary a node."""
106+ return {
107+ 'system_id': node.system_id,
108+ 'hostname': node.hostname
109+ }
110+
111+ @operation(idempotent=True)
112+ def by_type(self, request):
113+ """Prints a listing of nodes grouped by their type.
114+
115+ Optional arguments:
116+ summary: If True, prints a summary of each node instead of the complete
117+ object.
118+ count: If True, prints a count of each node type instead of any
119+ node details.
120+
121+ Also accepts any filter parameter the 'read' operation accepts.
122+ """
123+ summary = get_optional_param(
124+ request.GET, 'summary', default=False, validator=StringBool)
125+ count = get_optional_param(
126+ request.GET, 'count', default=False, validator=StringBool)
127+ label_to_model = {
128+ "machines": Machine,
129+ "devices": Device,
130+ "rack-controllers": RackController,
131+ "region-controllers": RegionController
132+ }
133+ if count:
134+ # We don't have a definition for the displayed fields of a plain
135+ # node, so only include that if it's a summary or a count.
136+ label_to_model["nodes"] = Node
137+ return {
138+ label: self.prepare_results(
139+ filtered_nodes_list_from_request(request, model=model),
140+ count=count, summary=summary)
141+ for label, model in label_to_model.items()
142+ }
143+
144 @classmethod
145 def resource_uri(cls, *args, **kwargs):
146 return ('nodes_handler', [])
147
148=== modified file 'src/maasserver/api/support.py'
149--- src/maasserver/api/support.py 2016-05-20 14:31:34 +0000
150+++ src/maasserver/api/support.py 2016-06-16 19:59:56 +0000
151@@ -13,6 +13,7 @@
152 from functools import wraps
153
154 from django.core.exceptions import PermissionDenied
155+from django.db.models.query import QuerySet
156 from django.http import Http404
157 from maasserver.api.doc import get_api_description_hash
158 from maasserver.exceptions import MAASAPIBadRequest
159@@ -260,6 +261,88 @@
160 else:
161 return function(self, request, *args, **kwargs)
162
163+ def prefetch(self, query):
164+ """Pre-fetches the specified query.
165+
166+ This method should pre-fetch anything anticipated to be recursively
167+ queried for in the non-summary version of the output.
168+
169+ By default, does no prefetching.
170+
171+ Override this in a subclass to do prefetching in case result_summary()
172+ in case non-summary output is requested.
173+ """
174+ return query
175+
176+ def summary(self, obj):
177+ """Return a summarized version of the specified model object.
178+
179+ This method is intended to return the object in a condensed format,
180+ with a minimal amount of useful identifying information.
181+
182+ Override this in a subclass to allow result_summary() to work
183+ correctly.
184+ """
185+ raise NotImplementedError
186+
187+ def result_summary(self, result, summary=False, order_by='id'):
188+ """Utility method to provide a shorter, summary output for the objects
189+ in the specified query, if desired.
190+
191+ :param result: must be either a QuerySet, or an integer.
192+ If an integer is passed in, it is simply returned. (This means
193+ the user requested a count instead of the query results.)
194+ :param summary: if True, returns a smaller object containing only
195+ essential information, rather than the complete object.
196+ :param order_by: optional field to order by (defaults to 'id').
197+ """
198+ if isinstance(result, int):
199+ # The user chose to count the objects in the query, instead of
200+ # returning the results of the query. So just give them the result.
201+ return result
202+ elif isinstance(result, QuerySet):
203+ # If this is a QuerySet, we need to ensure we both prefetch related
204+ # objects, and return it in a consistent order.
205+ result = self.prefetch(result).order_by(order_by)
206+ if not summary:
207+ # Pass this through to Piston if we're not in summary mode.
208+ return result
209+ else:
210+ # It would be nice if we could return a generator expression
211+ # here, but that doesn't seem to be supported by Piston.
212+ return [
213+ self.summary(obj) for obj in result
214+ ]
215+ else:
216+ # We're not sure what this is. So just let Piston do whatever
217+ # it was going to do with it.
218+ return result
219+
220+ def query_or_count(self, filter, count=False):
221+ """Returns either the results of the filter, or a count.
222+
223+ :param count: if True, calls count() on the specified filter and
224+ returns that. Otherwise, returns the filter unchanged.
225+ """
226+ if count:
227+ return filter.count()
228+ else:
229+ return filter
230+
231+ def prepare_results(self, query, count=False, summary=False):
232+ """Prepares the result set for the specified query.
233+
234+ Calls the 'summary' and 'prefetch' methods on subclasses, as
235+ appropriate.
236+
237+ :param count: If True, return a count of the objects that would be
238+ returned, instead of the objects themselves.
239+ :param summary: If True, return summarized objects, instead of the
240+ objects themselves.
241+ """
242+ return self.result_summary(
243+ self.query_or_count(query, count=count), summary=summary)
244+
245 @classmethod
246 def decorate(cls, func):
247 """Decorate all exported operations with the given function.
248
249=== modified file 'src/maasserver/api/tests/test_machines.py'
250--- src/maasserver/api/tests/test_machines.py 2016-06-09 23:39:53 +0000
251+++ src/maasserver/api/tests/test_machines.py 2016-06-16 19:59:56 +0000
252@@ -208,14 +208,49 @@
253 machine1 = factory.make_Node()
254 machine2 = factory.make_Node(
255 status=NODE_STATUS.ALLOCATED, owner=self.user)
256+ # This device should be ignored.
257+ factory.make_Device()
258 response = self.client.get(reverse('machines_handler'))
259 parsed_result = json.loads(
260 response.content.decode(settings.DEFAULT_CHARSET))
261-
262- self.assertEqual(http.client.OK, response.status_code)
263- self.assertItemsEqual(
264- [machine1.system_id, machine2.system_id],
265- extract_system_ids(parsed_result))
266+ self.assertEqual(http.client.OK, response.status_code)
267+ self.assertItemsEqual(
268+ [machine1.system_id, machine2.system_id],
269+ extract_system_ids(parsed_result))
270+
271+ def test_GET_lists_machines_with_summary(self):
272+ # The api allows for fetching the list of Machines.
273+ machine1 = factory.make_Node()
274+ machine2 = factory.make_Node(
275+ status=NODE_STATUS.ALLOCATED, owner=self.user)
276+ # This device should be ignored.
277+ factory.make_Device()
278+ response = self.client.get(reverse('machines_handler'), {
279+ 'summary': True
280+ })
281+ parsed_result = json.loads(
282+ response.content.decode(settings.DEFAULT_CHARSET))
283+ self.assertEqual(http.client.OK, response.status_code)
284+ self.assertItemsEqual(
285+ [machine1.system_id, machine2.system_id],
286+ extract_system_ids(parsed_result))
287+ # Ensure each result is a summary (only contains two items)
288+ self.expectThat(len(parsed_result[0]), Equals(2))
289+
290+ def test_GET_lists_machines_with_count(self):
291+ # The api allows for fetching the list of Machines.
292+ num_hosts = random.choice([3, 5, 7, 9])
293+ for _ in range(num_hosts):
294+ factory.make_Node()
295+ # This device should be ignored.
296+ factory.make_Device()
297+ response = self.client.get(reverse('machines_handler'), {
298+ 'count': True
299+ })
300+ parsed_result = json.loads(
301+ response.content.decode(settings.DEFAULT_CHARSET))
302+ self.assertEqual(http.client.OK, response.status_code)
303+ self.expectThat(parsed_result, Equals(num_hosts))
304
305 def test_GET_machines_issues_constant_number_of_queries(self):
306 # XXX: GavinPanella 2014-10-03 bug=1377335
307
308=== modified file 'src/maasserver/api/tests/test_nodes.py'
309--- src/maasserver/api/tests/test_nodes.py 2016-05-24 22:05:45 +0000
310+++ src/maasserver/api/tests/test_nodes.py 2016-06-16 19:59:56 +0000
311@@ -25,6 +25,7 @@
312 from maasserver.utils import ignore_unused
313 from maasserver.utils.orm import reload_object
314 from maastesting.djangotestcase import count_queries
315+from testtools.matchers import Equals
316
317
318 class AnonymousIsRegisteredAPITest(APITestCase.ForAnonymous):
319@@ -272,8 +273,109 @@
320 extract_system_ids_from_nodes(node_list))
321
322
323-class TestNodesAPI(APITestCase.ForUser):
324- """Tests for /api/2.0/nodes/."""
325+class TestAdminNodesAPI(APITestCase.ForAdmin):
326+ """Tests for /api/2.0/nodes/. (administrative users)"""
327+
328+ def test_GET_by_type_groups_by_type(self):
329+ # The api allows for fetching the list of Machines.
330+ machine = factory.make_Node()
331+ device = factory.make_Device()
332+ regionrack = factory.make_RegionRackController()
333+ rack = factory.make_RackController()
334+ region = factory.make_RegionController()
335+ response = self.client.get(reverse('nodes_handler'), {
336+ 'op': "by_type",
337+ 'summary': False,
338+ })
339+ result = json.loads(
340+ response.content.decode(settings.DEFAULT_CHARSET))
341+ self.assertEqual(http.client.OK, response.status_code)
342+ # Since we're testing as a normal user...
343+ self.assertItemsEqual(
344+ [
345+ machine.system_id,
346+ ],
347+ extract_system_ids(result["machines"]))
348+ self.assertItemsEqual(
349+ [
350+ device.system_id,
351+ ],
352+ extract_system_ids(result["devices"]))
353+ self.assertItemsEqual(
354+ [
355+ regionrack.system_id,
356+ region.system_id
357+ ],
358+ extract_system_ids(result["region-controllers"]))
359+ self.assertItemsEqual(
360+ [
361+ regionrack.system_id,
362+ rack.system_id,
363+ ],
364+ extract_system_ids(result["rack-controllers"]))
365+
366+ def test_GET_by_type_with_summary_groups_by_type(self):
367+ # The api allows for fetching the list of Machines.
368+ machine = factory.make_Node()
369+ device = factory.make_Device()
370+ regionrack = factory.make_RegionRackController()
371+ rack = factory.make_RackController()
372+ region = factory.make_RegionController()
373+ response = self.client.get(reverse('nodes_handler'), {
374+ 'op': "by_type",
375+ 'summary': False,
376+ })
377+ result = json.loads(
378+ response.content.decode(settings.DEFAULT_CHARSET))
379+ self.assertEqual(http.client.OK, response.status_code)
380+ # Since we're testing as a normal user...
381+ self.assertItemsEqual(
382+ [
383+ machine.system_id,
384+ ],
385+ extract_system_ids(result["machines"]))
386+ self.assertItemsEqual(
387+ [
388+ device.system_id,
389+ ],
390+ extract_system_ids(result["devices"]))
391+ self.assertItemsEqual(
392+ [
393+ regionrack.system_id,
394+ region.system_id
395+ ],
396+ extract_system_ids(result["region-controllers"]))
397+ self.assertItemsEqual(
398+ [
399+ regionrack.system_id,
400+ rack.system_id,
401+ ],
402+ extract_system_ids(result["rack-controllers"]))
403+
404+ def test_GET_by_type_with_count_groups_by_type(self):
405+ # The api allows for fetching the list of Machines.
406+ factory.make_Node()
407+ factory.make_Device()
408+ factory.make_RegionRackController()
409+ factory.make_RackController()
410+ factory.make_RegionController()
411+ response = self.client.get(reverse('nodes_handler'), {
412+ 'op': "by_type",
413+ 'count': True,
414+ })
415+ result = json.loads(
416+ response.content.decode(settings.DEFAULT_CHARSET))
417+ self.assertEqual(http.client.OK, response.status_code)
418+ # Since we're testing as an admin, we can see everything.
419+ self.assertThat(result["nodes"], Equals(5))
420+ self.assertThat(result["machines"], Equals(1))
421+ self.assertThat(result["devices"], Equals(1))
422+ self.assertThat(result["rack-controllers"], Equals(2))
423+ self.assertThat(result["region-controllers"], Equals(2))
424+
425+
426+class TestUserNodesAPI(APITestCase.ForUser):
427+ """Tests for /api/2.0/nodes/. (normal users)"""
428
429 def test_handler_path(self):
430 self.assertEqual(
431@@ -284,14 +386,139 @@
432 node1 = factory.make_Node()
433 node2 = factory.make_Node(
434 status=NODE_STATUS.ALLOCATED, owner=self.user)
435+ device = factory.make_Device()
436 response = self.client.get(reverse('nodes_handler'))
437 parsed_result = json.loads(
438 response.content.decode(settings.DEFAULT_CHARSET))
439-
440- self.assertEqual(http.client.OK, response.status_code)
441- self.assertItemsEqual(
442- [node1.system_id, node2.system_id],
443- extract_system_ids(parsed_result))
444+ self.assertEqual(http.client.OK, response.status_code)
445+ self.assertItemsEqual(
446+ [node1.system_id, node2.system_id, device.system_id],
447+ extract_system_ids(parsed_result))
448+
449+ def test_GET_lists_nodes_with_summary(self):
450+ # The api allows for fetching the list of Machines.
451+ machine1 = factory.make_Node()
452+ machine2 = factory.make_Node(
453+ status=NODE_STATUS.ALLOCATED, owner=self.user)
454+ device = factory.make_Device()
455+ response = self.client.get(reverse('nodes_handler'), {
456+ 'summary': True
457+ })
458+ parsed_result = json.loads(
459+ response.content.decode(settings.DEFAULT_CHARSET))
460+
461+ self.assertEqual(http.client.OK, response.status_code)
462+ self.assertItemsEqual(
463+ [machine1.system_id, machine2.system_id, device.system_id],
464+ extract_system_ids(parsed_result))
465+ # Ensure each result is a summary (only contains two items)
466+ self.expectThat(len(parsed_result[0]), Equals(2))
467+
468+ def test_GET_lists_nodes_with_count(self):
469+ # The api allows for fetching the list of Machines.
470+ num_hosts = random.choice([3, 5, 7, 9])
471+ for _ in range(num_hosts):
472+ factory.make_Node()
473+ factory.make_Device()
474+ response = self.client.get(reverse('nodes_handler'), {
475+ 'count': True
476+ })
477+ parsed_result = json.loads(
478+ response.content.decode(settings.DEFAULT_CHARSET))
479+ self.assertEqual(http.client.OK, response.status_code)
480+ self.expectThat(parsed_result, Equals(num_hosts * 2))
481+
482+ def test_GET_by_type_groups_by_type(self):
483+ # The api allows for fetching the list of Machines.
484+ machine = factory.make_Node()
485+ device = factory.make_Device()
486+ # These should be ignored since we're a normal user.
487+ factory.make_RegionRackController()
488+ factory.make_RackController()
489+ factory.make_RegionController()
490+ response = self.client.get(reverse('nodes_handler'), {
491+ 'op': "by_type",
492+ 'summary': False,
493+ })
494+ result = json.loads(
495+ response.content.decode(settings.DEFAULT_CHARSET))
496+ self.assertEqual(http.client.OK, response.status_code)
497+ self.assertItemsEqual(
498+ [
499+ machine.system_id,
500+ ],
501+ extract_system_ids(result["machines"]))
502+ self.assertItemsEqual(
503+ [
504+ device.system_id,
505+ ],
506+ extract_system_ids(result["devices"]))
507+ # Since we're testing as a normal user, we won't be able to see
508+ # controllers.
509+ self.assertItemsEqual(
510+ [
511+ ],
512+ extract_system_ids(result["region-controllers"]))
513+ self.assertItemsEqual(
514+ [
515+ ],
516+ extract_system_ids(result["rack-controllers"]))
517+
518+ def test_GET_by_type_with_summary_groups_by_type(self):
519+ # The api allows for fetching the list of Machines.
520+ machine = factory.make_Node()
521+ device = factory.make_Device()
522+ factory.make_RegionRackController()
523+ factory.make_RackController()
524+ factory.make_RegionController()
525+ response = self.client.get(reverse('nodes_handler'), {
526+ 'op': "by_type",
527+ 'summary': False,
528+ })
529+ result = json.loads(
530+ response.content.decode(settings.DEFAULT_CHARSET))
531+ self.assertEqual(http.client.OK, response.status_code)
532+ # Since we're testing as a normal user...
533+ self.assertItemsEqual(
534+ [
535+ machine.system_id,
536+ ],
537+ extract_system_ids(result["machines"]))
538+ self.assertItemsEqual(
539+ [
540+ device.system_id,
541+ ],
542+ extract_system_ids(result["devices"]))
543+ self.assertItemsEqual(
544+ [
545+ ],
546+ extract_system_ids(result["region-controllers"]))
547+ self.assertItemsEqual(
548+ [
549+ ],
550+ extract_system_ids(result["rack-controllers"]))
551+
552+ def test_GET_by_type_with_count_groups_by_type(self):
553+ # The api allows for fetching the list of Machines.
554+ factory.make_Node()
555+ factory.make_Device()
556+ factory.make_RegionRackController()
557+ factory.make_RackController()
558+ factory.make_RegionController()
559+ response = self.client.get(reverse('nodes_handler'), {
560+ 'op': "by_type",
561+ 'count': True,
562+ })
563+ result = json.loads(
564+ response.content.decode(settings.DEFAULT_CHARSET))
565+ self.assertEqual(http.client.OK, response.status_code)
566+ # Since we're testing as a normal user, they can only see the nodes
567+ # and devices.
568+ self.assertThat(result["nodes"], Equals(2))
569+ self.assertThat(result["machines"], Equals(1))
570+ self.assertThat(result["devices"], Equals(1))
571+ self.assertThat(result["rack-controllers"], Equals(0))
572+ self.assertThat(result["region-controllers"], Equals(0))
573
574 def create_nodes(self, nodegroup, nb):
575 for _ in range(nb):
576
577=== modified file 'src/maasserver/testing/testclient.py'
578--- src/maasserver/testing/testclient.py 2016-05-24 14:53:56 +0000
579+++ src/maasserver/testing/testclient.py 2016-06-16 19:59:56 +0000
580@@ -30,7 +30,7 @@
581 """A derivative of Django's test client specially for MAAS.
582
583 This ensures that requests are performed in a transaction, and that
584- post-commit hooks are alway fired or reset.
585+ post-commit hooks are always fired or reset.
586
587 It also permits logging-in using just a user object. A password will be
588 configured and used to log-in.