launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/triviplacement into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/triviplacement/+merge/99471

This supports arbitrary constraints in the “acquire” API call, where “arbitrary” is defined as: select a node name.  The code is prepared for adding more constraints in the future, although the function that extracts constraints from the http request is still absolutely minimal for now.

The constraint is implemented as a database query filter, so that it should scale about as well as is possible.  It won't load non-matching nodes into memory, or require additional queries.

Test coverage may be more than is strictly needed.  What I have in mind is:

 * Generic constraints handling is extensively integration-tested (with the name constraint as the example).
 * Each constraint gets a positive acceptance test (“can allocate nodes with this constraint”).
 * Each constraint gets a negative acceptance test (“rejects nodes not matching this constraint”).
 * Constraints extraction is unit-tested in detail, of which there is still very little.
 * Node selection is different from acquisition, and tested separately.

That last choice causes some of the duplication.  In particular, what happens if you ask for a node that does not exist?  From a bottom-up, implementation standpoint it's easy: no node matches the query filter, node selection returns None, acquisition fails, and the API returns httplib.CONFLICT.  But it's different from a top-down point of view: a Conflict error makes more sense than Not Found because the requested node is not the http resource being requested, so that's a necessary integration test.  Acquisition should fail because nothing matches constraints, whatever they be.  And node selection should return no nodes but not blow up, even though the query may seem a bit nonsensical if you look at system_id as a node's identifier.  All different reasons for what ends up being very simple code.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/triviplacement/+merge/99471
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/triviplacement into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-03-27 03:42:26 +0000
+++ src/maasserver/api.py	2012-03-27 06:37:17 +0000
@@ -379,6 +379,21 @@
         return ('nodes_handler', [])
 
 
+def extract_constraints(request_params):
+    """Extract a dict of node allocation constraints from http parameters.
+
+    :param request_params: Parameters submitted with the allocation request.
+    :type request_params: :class:`django.http.QueryDict. QueryDict`
+    :return: A mapping of applicable constraint names to their values.
+    :rtype: :class:`dict`
+    """
+    name = request_params.get('name', None)
+    if name is None:
+        return {}
+    else:
+        return {'name': name}
+
+
 @api_operations
 class NodesHandler(BaseHandler):
     """Manage collection of Nodes."""
@@ -422,9 +437,10 @@
     @api_exported('acquire', 'POST')
     def acquire(self, request):
         """Acquire an available node for deployment."""
-        node = Node.objects.get_available_node_for_acquisition(request.user)
+        node = Node.objects.get_available_node_for_acquisition(
+            request.user, constraints=extract_constraints(request.data))
         if node is None:
-            raise NodesNotAvailable("No node is available.")
+            raise NodesNotAvailable("No matching node is available.")
         auth_header = request.META.get("HTTP_AUTHORIZATION")
         assert auth_header is not None, (
             "HTTP_AUTHORIZATION not set on request")

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-03-27 04:20:39 +0000
+++ src/maasserver/models.py	2012-03-27 06:37:17 +0000
@@ -290,15 +290,26 @@
         else:
             raise PermissionDenied
 
-    def get_available_node_for_acquisition(self, for_user):
+    def get_available_node_for_acquisition(self, for_user, constraints=None):
         """Find a `Node` to be acquired by the given user.
 
         :param for_user: The user who is to acquire the node.
-        :return: A `Node`, or None if none are available.
+        :type for_user: :class:`django.contrib.auth.models.User`
+        :param constraints: Optional selection constraints.  If given, only
+            nodes matching these constraints are considered.
+        :type constraints: :class:`dict`
+        :return: A matching `Node`, or None if none are available.
         """
+        if constraints is None:
+            constraints = {}
         available_nodes = (
             self.get_visible_nodes(for_user)
                 .filter(status=NODE_STATUS.READY))
+
+        if constraints.get('name'):
+            available_nodes = available_nodes.filter(
+                system_id=constraints['name'])
+
         available_nodes = list(available_nodes[:1])
         if len(available_nodes) == 0:
             return None

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-03-23 13:40:48 +0000
+++ src/maasserver/tests/test_api.py	2012-03-27 06:37:17 +0000
@@ -15,13 +15,18 @@
 import httplib
 import json
 import os
+import random
 import shutil
 
 from django.conf import settings
 from django.db.models.signals import post_save
+from django.http import QueryDict
 from fixtures import Fixture
 from maasserver import api
-from maasserver.api import extract_oauth_key
+from maasserver.api import (
+    extract_constraints,
+    extract_oauth_key,
+    )
 from maasserver.models import (
     ARCHITECTURE_CHOICES,
     Config,
@@ -72,6 +77,15 @@
     def test_extract_oauth_key_returns_None_without_oauth_key(self):
         self.assertIs(None, extract_oauth_key(''))
 
+    def test_extract_constraints_ignores_unknown_parameters(self):
+        self.assertEqual({}, extract_constraints(QueryDict('spin=left')))
+
+    def test_extract_constraints_extracts_name(self):
+        name = factory.getRandomString()
+        self.assertEqual(
+            {'name': name},
+            extract_constraints(QueryDict('name=%s' % name)))
+
 
 class AnonymousEnlistmentAPITest(APIv10TestMixin, TestCase):
     # Nodes can be enlisted anonymously.
@@ -758,7 +772,7 @@
         available_status = NODE_STATUS.READY
         node = factory.make_node(status=available_status, owner=None)
         response = self.client.post(self.get_uri('nodes/'), {'op': 'acquire'})
-        self.assertEqual(200, response.status_code)
+        self.assertEqual(httplib.OK, response.status_code)
         parsed_result = json.loads(response.content)
         self.assertEqual(node.system_id, parsed_result['system_id'])
 
@@ -777,6 +791,81 @@
         # Fails with Conflict error: resource can't satisfy request.
         self.assertEqual(httplib.CONFLICT, response.status_code)
 
+    def test_POST_acquire_chooses_candidate_matching_constraint(self):
+        # If "acquire" is passed a constraint, it will go for a node
+        # matching that constraint even if there's tons of other nodes
+        # available.
+        # (Creating lots of nodes here to minimize the chances of this
+        # passing by accident).
+        available_nodes = [
+            factory.make_node(status=NODE_STATUS.READY, owner=None)
+            for counter in range(20)]
+        desired_node = random.choice(available_nodes)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'name': desired_node.system_id,
+        })
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual(desired_node.system_id, parsed_result['system_id'])
+
+    def test_POST_acquire_ignores_unknown_constraint(self):
+        node = factory.make_node(status=NODE_STATUS.READY, owner=None)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'scent': factory.getRandomString(),
+        })
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual(node.system_id, parsed_result['system_id'])
+
+    def test_POST_acquire_would_rather_fail_than_disobey_constraint(self):
+        # If "acquire" is passed a constraint, it won't return a node
+        # that does not meet that constraint.  Even if it means that it
+        # can't meet the request.
+        factory.make_node(status=NODE_STATUS.READY, owner=None)
+        desired_node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'name': desired_node.system_id,
+        })
+        self.assertEqual(httplib.CONFLICT, response.status_code)
+
+    def test_POST_acquire_allocates_node_by_name(self):
+        # A name constraint does not stop "acquire" from allocating a
+        # node.
+        node = factory.make_node(status=NODE_STATUS.READY, owner=None)
+        system_id = node.system_id
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'name': system_id,
+        })
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(system_id, json.loads(response.content)['system_id'])
+
+    def test_POST_acquire_constrains_by_name(self):
+        # A name constraint does limit which nodes can be allocated.
+        node = factory.make_node(status=NODE_STATUS.READY, owner=None)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'name': node.system_id + factory.getRandomString(),
+        })
+        self.assertEqual(httplib.CONFLICT, response.status_code)
+
+    def test_POST_acquire_treats_unknown_name_as_resource_conflict(self):
+        # A name constraint naming an unknown node produces a resource
+        # conflict: most likely the node existed but has changed or
+        # disappeared.
+        # Certainly it's not a 404, since the resource named in the URL
+        # is "nodes/," which does exist.
+        factory.make_node(status=NODE_STATUS.READY, owner=None)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'name': factory.getRandomString(),
+        })
+        self.assertEqual(httplib.CONFLICT, response.status_code)
+
     def test_POST_acquire_sets_a_token(self):
         # "acquire" should set the Token being used in the request on
         # the Node that is allocated.

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-27 04:20:39 +0000
+++ src/maasserver/tests/test_models.py	2012-03-27 06:37:17 +0000
@@ -293,6 +293,29 @@
         self.assertEqual(
             None, Node.objects.get_available_node_for_acquisition(user))
 
+    def test_get_available_node_combines_constraint_with_availability(self):
+        user = factory.make_user()
+        node = self.make_node(factory.make_user())
+        self.assertEqual(
+            None,
+            Node.objects.get_available_node_for_acquisition(
+                user, {'name': node.system_id}))
+
+    def test_get_available_node_constrains_by_name(self):
+        user = factory.make_user()
+        nodes = [self.make_node() for counter in range(3)]
+        self.assertEqual(
+            nodes[1],
+            Node.objects.get_available_node_for_acquisition(
+                user, {'name': nodes[1].system_id}))
+
+    def test_get_available_node_does_not_blow_up_if_name_is_unknown(self):
+        user = factory.make_user()
+        self.assertEqual(
+            None,
+            Node.objects.get_available_node_for_acquisition(
+                user, {'name': factory.getRandomString()}))
+
     def test_stop_nodes_stops_nodes(self):
         user = factory.make_user()
         node = self.make_node(user)