launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/maas/cpu_mem_tag_constraints into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gz/maas/cpu_mem_tag_constraints/+merge/126502

Adds constraints to the api for acquiring a node that limit by cpu, memory, and the tags set.

This is mostly straight forward, with a few wrinkles for how things are named across levels, and a new InvalidConstraint exception to try making at least some of the error responses useful.

As I understand it, repeated filter calls are okay. Were we to want to support "tag OR tag" we can do that with the django Q class.
-- 
https://code.launchpad.net/~gz/maas/cpu_mem_tag_constraints/+merge/126502
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/maas/cpu_mem_tag_constraints into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-26 10:00:14 +0000
+++ src/maasserver/api.py	2012-09-26 16:44:22 +0000
@@ -643,6 +643,10 @@
         return ('nodes_handler', [])
 
 
+_constraints_map = {s: s for s in ('name', 'tags', 'arch', 'cpu_count')}
+_constraints_map['mem'] = "memory"
+
+
 def extract_constraints(request_params):
     """Extract a dict of node allocation constraints from http parameters.
 
@@ -651,9 +655,8 @@
     :return: A mapping of applicable constraint names to their values.
     :rtype: :class:`dict`
     """
-    supported_constraints = ('name', 'arch')
-    return {constraint: request_params[constraint]
-        for constraint in supported_constraints
+    return {_constraints_map[constraint]: request_params[constraint]
+        for constraint in _constraints_map
             if constraint in request_params}
 
 

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2012-08-21 20:14:12 +0000
+++ src/maasserver/exceptions.py	2012-09-26 16:44:22 +0000
@@ -84,6 +84,20 @@
     api_error = httplib.CONFLICT
 
 
+class InvalidConstraint(MAASAPIBadRequest):
+    """Node allocation constraint given cannot be interpreted."""
+
+    def __init__(self, constraint, value, err=None):
+        super(InvalidConstraint, self).__init__(constraint, value)
+        self.err = err
+
+    def __str__(self):
+        s = "Invalid '%s' constraint '%s'" % self.args
+        if self.err:
+            return "%s: %s" % (s, str(self.err))
+        return s
+
+
 class Redirect(MAASAPIException):
     """Redirect.  The exception message is the target URL."""
     api_error = httplib.FOUND

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-09-26 08:54:29 +0000
+++ src/maasserver/models/node.py	2012-09-26 16:44:22 +0000
@@ -51,7 +51,10 @@
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
     )
-from maasserver.exceptions import NodeStateViolation
+from maasserver.exceptions import (
+    InvalidConstraint,
+    NodeStateViolation,
+    )
 from maasserver.fields import (
     JSONObjectField,
     XMLField,
@@ -264,6 +267,23 @@
             #                using an i386 image on amd64 hardware will wait.
             available_nodes = available_nodes.filter(
                 architecture=constraints['arch'])
+        for key in ('cpu_count', 'memory'):
+            string = constraints.get(key)
+            if string:
+                try:
+                    value = int(string)
+                except ValueError as e:
+                    raise InvalidConstraint(key, string, e)
+                q = {key + "__gte": value}
+                available_nodes = available_nodes.filter(**q)
+        tag_expression = constraints.get('tags')
+        if tag_expression:
+            tag_names = tag_expression.replace(",", " ").split()
+            for tag_name in tag_names:
+                # GZ 2012-09-26: Below results in a generic "Not Found" body
+                #                rather than something useful with tag name.
+                tag = Tag.objects.get_tag_or_404(tag_name, for_user)
+                available_nodes = available_nodes.filter(tags=tag)
 
         return get_first(available_nodes)
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-26 10:00:14 +0000
+++ src/maasserver/tests/test_api.py	2012-09-26 16:44:22 +0000
@@ -828,6 +828,11 @@
         self.logged_in_user.is_superuser = True
         self.logged_in_user.save()
 
+    def assertResponseCode(self, expected_code, response):
+        if response.status_code != expected_code:
+            self.fail("Expected %s response, got %s:\n%s" % (
+                expected_code, response.status_code, response.content))
+
 
 def extract_system_ids(parsed_result):
     """List the system_ids of the nodes in `parsed_result`."""
@@ -1712,6 +1717,81 @@
         })
         self.assertEqual(httplib.CONFLICT, response.status_code)
 
+    def test_POST_acquire_allocates_node_by_cpu(self):
+        # Asking for enough cpu acquires a node with at least that.
+        node = factory.make_node(status=NODE_STATUS.READY, cpu_count=3)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'cpu_count': 2,
+        })
+        self.assertResponseCode(httplib.OK, response)
+        response_json = json.loads(response.content)
+        self.assertEqual(node.system_id, response_json['system_id'])
+
+    def test_POST_acquire_fails_with_invalid_cpu(self):
+        # Asking for an invalid amount of cpu returns a bad request.
+        factory.make_node(status=NODE_STATUS.READY)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'cpu_count': 'plenty',
+        })
+        self.assertResponseCode(httplib.BAD_REQUEST, response)
+
+    def test_POST_acquire_allocates_node_by_mem(self):
+        # Asking for enough memory acquires a node with at least that.
+        node = factory.make_node(status=NODE_STATUS.READY, memory=1024)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'mem': 1024,
+        })
+        self.assertResponseCode(httplib.OK, response)
+        response_json = json.loads(response.content)
+        self.assertEqual(node.system_id, response_json['system_id'])
+
+    def test_POST_acquire_fails_with_invalid_mem(self):
+        # Asking for an invalid amount of cpu returns a bad request.
+        factory.make_node(status=NODE_STATUS.READY)
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'mem': 'bags',
+        })
+        self.assertResponseCode(httplib.BAD_REQUEST, response)
+
+    def test_POST_acquire_allocates_node_by_tags(self):
+        # Asking for particular tags acquires a node with those tags.
+        node = factory.make_node(status=NODE_STATUS.READY)
+        node_tag_names = ["fast", "stable", "cute"]
+        node.tags = [factory.make_tag(t) for t in node_tag_names]
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'tags': 'fast, stable',
+        })
+        self.assertResponseCode(httplib.OK, response)
+        response_json = json.loads(response.content)
+        self.assertEqual(node_tag_names, response_json['tag_names'])
+
+    def test_POST_acquire_fails_without_all_tags(self):
+        # 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")]
+        node2 = factory.make_node(status=NODE_STATUS.READY)
+        node2.tags = [factory.make_tag("cheap")]
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'tags': 'fast, cheap',
+        })
+        self.assertResponseCode(httplib.CONFLICT, response)
+
+    def test_POST_acquire_fails_with_unknown_tags(self):
+        # 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")]
+        response = self.client.post(self.get_uri('nodes/'), {
+            'op': 'acquire',
+            'tags': 'fast, hairy',
+        })
+        self.assertResponseCode(httplib.NOT_FOUND, response)
+
     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_exceptions.py'
--- src/maasserver/tests/test_exceptions.py	2012-04-27 12:38:18 +0000
+++ src/maasserver/tests/test_exceptions.py	2012-09-26 16:44:22 +0000
@@ -15,6 +15,7 @@
 import httplib
 
 from maasserver.exceptions import (
+    InvalidConstraint,
     MAASAPIBadRequest,
     Redirect,
     )
@@ -38,3 +39,21 @@
         exception = Redirect(target)
         response = exception.make_http_response()
         self.assertEqual(target, extract_redirect(response))
+
+    def test_InvalidConstraint_is_bad_request(self):
+        err = InvalidConstraint("height", "-1", ValueError("Not positive"))
+        response = err.make_http_response()
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+
+    def test_InvalidConstraint_str_without_context(self):
+        err = InvalidConstraint("hue", "shiny")
+        self.assertEqual(str(err), "Invalid 'hue' constraint 'shiny'")
+
+    def test_InvalidConstraint_str_with_context(self):
+        try:
+            int("hundreds")
+        except ValueError as e:
+            context = e
+        err = InvalidConstraint("limbs", "hundreds", context)
+        self.assertEqual(str(err),
+            "Invalid 'limbs' constraint 'hundreds': " + str(context))

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-09-26 08:54:29 +0000
+++ src/maasserver/tests/test_node.py	2012-09-26 16:44:22 +0000
@@ -19,6 +19,7 @@
     PermissionDenied,
     ValidationError,
     )
+from django.http import Http404
 from maasserver.enum import (
     ARCHITECTURE,
     DISTRO_SERIES,
@@ -27,7 +28,10 @@
     NODE_STATUS_CHOICES,
     NODE_STATUS_CHOICES_DICT,
     )
-from maasserver.exceptions import NodeStateViolation
+from maasserver.exceptions import (
+    InvalidConstraint,
+    NodeStateViolation,
+    )
 from maasserver.models import (
     Config,
     MACAddress,
@@ -722,6 +726,88 @@
             Node.objects.get_available_node_for_acquisition(
                 user, {'arch': "sparc"}))
 
+    def test_get_available_node_with_cpu_enough(self):
+        user = factory.make_user()
+        node = self.make_node(cpu_count=2)
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'cpu_count': "2"})
+        self.assertEqual(node, available_node)
+
+    def test_get_available_node_with_cpu_not_enough(self):
+        user = factory.make_user()
+        node = self.make_node(cpu_count=1)
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'cpu_count': "2"})
+        self.assertEqual(None, available_node)
+
+    def test_get_available_node_with_cpu_invalid(self):
+        user = factory.make_user()
+        err = self.assertRaises(InvalidConstraint,
+            Node.objects.get_available_node_for_acquisition,
+            user, {'cpu_count': "lots"})
+        self.assertEqual(("cpu_count", "lots"), err.args)
+
+    def test_get_available_node_with_mem_enough(self):
+        user = factory.make_user()
+        node = self.make_node(memory=4096)
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'memory': "2048"})
+        self.assertEqual(node, available_node)
+
+    def test_get_available_node_with_mem_not_enough(self):
+        user = factory.make_user()
+        node = self.make_node(memory=4096)
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'memory': "4097"})
+        self.assertEqual(None, available_node)
+
+    def test_get_available_node_with_mem_invalid(self):
+        user = factory.make_user()
+        err = self.assertRaises(InvalidConstraint,
+            Node.objects.get_available_node_for_acquisition,
+            user, {'memory': "forgetful"})
+        self.assertEqual(("memory", "forgetful"), err.args)
+
+    def test_get_available_node_with_single_matching_tag(self):
+        user = factory.make_user()
+        node = self.make_node()
+        node.tags = [factory.make_tag("happy")]
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'tags': "happy"})
+        self.assertEqual(node, available_node)
+
+    def test_get_available_node_with_multiple_matching_tags(self):
+        user = factory.make_user()
+        node = self.make_node()
+        node.tags = [factory.make_tag("happy"), factory.make_tag("friendly")]
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'tags': "happy, friendly"})
+        self.assertEqual(node, available_node)
+
+    def test_get_available_node_with_single_mismatching_tag(self):
+        user = factory.make_user()
+        node = self.make_node()
+        node.tags = [factory.make_tag("happy")]
+        factory.make_tag("unhappy")
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'tags': "unhappy"})
+        self.assertEqual(None, available_node)
+
+    def test_get_available_node_with_partially_mismatching_tags(self):
+        user = factory.make_user()
+        node = self.make_node()
+        node.tags = [factory.make_tag("happy"), factory.make_tag("friendly")]
+        factory.make_tag("unfriendly")
+        available_node = Node.objects.get_available_node_for_acquisition(
+            user, {'tags': "happy, unfriendly"})
+        self.assertEqual(None, available_node)
+
+    def test_get_available_node_with_single_missing_tag(self):
+        user = factory.make_user()
+        self.assertRaises(Http404,
+            Node.objects.get_available_node_for_acquisition,
+            user, {'tags': "lonely"})
+
     def test_stop_nodes_stops_nodes(self):
         # We don't actually want to fire off power events, but we'll go
         # through the motions right up to the point where we'd normally