launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/release-node-api into lp:maas with lp:~jtv/maas/enum as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/release-node-api/+merge/95855

This adds a “release” method (using POST) to the Node API, as a counterpart to “acquire.”

Releasing a node in any other state than “ready,” “allocated,” or “reserved” is an error.  For the rest, the rules are similar to those for node visibility:
 * Users can release their own allocated or reserved nodes.
 * Releasing an already-released node does nothing.
 * Admins can release anyone's allocated or reserved nodes.

I added two helper functions for reloading objects from the database, so that the test can see what changes the API implementation made to them.  There may be some standard way of doing this; if not, we may want to extract these helpers into the testing module later.


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

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-03-01 14:34:18 +0000
+++ src/maasserver/api.py	2012-03-05 07:43:20 +0000
@@ -41,6 +41,7 @@
     MaaSAPIBadRequest,
     MaaSAPINotFound,
     NodesNotAvailable,
+    NodeStateViolation,
     PermissionDenied,
     )
 from maasserver.fields import validate_mac
@@ -49,6 +50,7 @@
     FileStorage,
     MACAddress,
     Node,
+    NODE_STATUS,
     )
 from piston.doc import generate_doc
 from piston.handler import (
@@ -272,6 +274,23 @@
                 "You are not allowed to start up this node.")
         return nodes[0]
 
+    @api_exported('release', 'POST')
+    def release(self, request, system_id):
+        """Release a node.  Opposite of `NodesHandler.acquire`."""
+        node = Node.objects.get_visible_node_or_404(
+            system_id=system_id, user=request.user)
+        if node.status == NODE_STATUS.READY:
+            # Nothing to do.  This may be a redundant retry, and the
+            # postcondition is achieved, so call this success.
+            pass
+        elif node.status in [NODE_STATUS.ALLOCATED, NODE_STATUS.RESERVED]:
+            node.release()
+            node.save()
+        else:
+            raise NodeStateViolation(
+                "Node cannot be released in its current state.")
+        return node
+
 
 def create_node(request):
     form = NodeWithMACAddressesForm(request.data)

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2012-02-24 17:37:01 +0000
+++ src/maasserver/exceptions.py	2012-03-05 07:43:20 +0000
@@ -14,6 +14,7 @@
     "MaaSAPIBadRequest",
     "MaaSAPIException",
     "MaaSAPINotFound",
+    "NodeStateViolation",
     "PermissionDenied",
     ]
 
@@ -62,6 +63,11 @@
     api_error = httplib.UNAUTHORIZED
 
 
-class NodesNotAvailable(MaaSAPIException):
+class NodeStateViolation(MaaSAPIException):
+    """Operation on node not possible given node's current state."""
+    api_error = httplib.CONFLICT
+
+
+class NodesNotAvailable(NodeStateViolation):
     """Requested node(s) are not available to be acquired."""
     api_error = httplib.CONFLICT

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-03-02 17:52:27 +0000
+++ src/maasserver/models.py	2012-03-05 07:43:20 +0000
@@ -402,6 +402,16 @@
         self.status = NODE_STATUS.ALLOCATED
         self.owner = by_user
 
+    def release(self):
+        """Mark allocated or reserved node as available again."""
+        assert self.status in [
+            NODE_STATUS.READY,
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.RESERVED,
+            ]
+        self.status = NODE_STATUS.READY
+        self.owner = None
+
 
 mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-03-01 15:40:36 +0000
+++ src/maasserver/tests/test_api.py	2012-03-05 07:43:20 +0000
@@ -28,6 +28,7 @@
     LoggedInTestCase,
     TestCase,
     )
+from maasserver.testing.enum import map_enum
 from maasserver.testing.factory import factory
 from maasserver.testing.oauthclient import OAuthAuthenticatedClient
 from metadataserver.models import (
@@ -37,6 +38,44 @@
 from metadataserver.nodeinituser import get_node_init_user
 
 
+def reload_object(model_object):
+    """Reload `obj` from the database.
+
+    Use this when a test needs to inspect changes to model objects made by
+    the API.
+
+    If the object has been deleted, this will raise the `DoesNotExist`
+    exception for its model class.
+
+    :param model_object: Model object to reload.
+    :type model_object: Concrete `Model` subtype.
+    :return: Freshly-loaded instance of `model_object`.
+    :rtype: Same as `model_object`.
+    """
+    return model_object.__class__.objects.get(id=model_object.id)
+
+
+def reload_objects(model_class, model_objects):
+    """Reload `model_objects` of type `model_class` from the database.
+
+    Use this when a test needs to inspect changes to model objects made by
+    the API.
+
+    If any of the objects have been deleted, they will not be included in
+    the result.
+
+    :param model_class: `Model` class to reload from.
+    :type model_class: Class.
+    :param model_objects: Objects to reload from the database.
+    :type model_objects: Sequence of `model_class` objects.
+    :return: Reloaded objects, in no particular order.
+    :rtype: Sequence of `model_class` objects.
+    """
+    assert all(isinstance(obj, model_class) for obj in model_objects)
+    return model_class.objects.filter(
+        id__in=[obj.id for obj in model_objects])
+
+
 class APIv10TestMixin:
 
     def get_uri(self, path):
@@ -297,7 +336,7 @@
         response = self.client.post(self.get_node_uri(node), {'op': 'start'})
         self.assertEqual(httplib.OK, response.status_code)
 
-    def test_POST_stores_user_data(self):
+    def test_POST_start_stores_user_data(self):
         node = factory.make_node(owner=self.logged_in_user)
         user_data = (
             b'\xff\x00\xff\xfe\xff\xff\xfe' +
@@ -310,6 +349,80 @@
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
 
+    def test_POST_release_releases_owned_node(self):
+        owned_statuses = [
+            NODE_STATUS.RESERVED,
+            NODE_STATUS.ALLOCATED,
+            ]
+        owned_nodes = [
+            factory.make_node(owner=self.logged_in_user, status=status)
+            for status in owned_statuses]
+        responses = [
+            self.client.post(self.get_node_uri(node), {'op': 'release'})
+            for node in owned_nodes]
+        self.assertEqual(
+            [httplib.OK] * len(owned_nodes),
+            [response.status_code for response in responses])
+        self.assertItemsEqual(
+            [NODE_STATUS.READY] * len(owned_nodes),
+            [node.status for node in reload_objects(Node, owned_nodes)])
+
+    def test_POST_release_does_nothing_for_unowned_node(self):
+        node = factory.make_node(status=NODE_STATUS.READY)
+        response = self.client.post(
+            self.get_node_uri(node), {'op': 'release'})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+
+    def test_POST_release_fails_for_other_node_states(self):
+        releasable_statuses = [
+            NODE_STATUS.RESERVED,
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.READY,
+            ]
+        unreleasable_statuses = [
+            status
+            for status in map_enum(NODE_STATUS).values()
+                if status not in releasable_statuses]
+        nodes = [
+            factory.make_node(status=status, owner=self.logged_in_user)
+            for status in unreleasable_statuses]
+        responses = [
+            self.client.post(self.get_node_uri(node), {'op': 'release'})
+            for node in nodes]
+        self.assertEqual(
+            [httplib.CONFLICT] * len(unreleasable_statuses),
+            [response.status_code for response in responses])
+        self.assertEqual(
+            unreleasable_statuses,
+            [node.status for node in reload_objects(Node, nodes)])
+
+    def test_POST_release_rejects_request_from_unauthorized_user(self):
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        response = self.client.post(
+            self.get_node_uri(node), {'op': 'release'})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertEqual(NODE_STATUS.ALLOCATED, reload_object(node).status)
+
+    def test_POST_release_allows_admin_to_release_anyones_node(self):
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        self.become_admin()
+        response = self.client.post(
+            self.get_node_uri(node), {'op': 'release'})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+
+    def test_POST_release_combines_with_acquire(self):
+        node = factory.make_node(status=NODE_STATUS.READY)
+        response = self.client.post(
+            self.get_uri('nodes/'), {'op': 'acquire'})
+        node_uri = json.loads(response.content)['resource_uri']
+        response = self.client.post(node_uri, {'op': 'release'})
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(NODE_STATUS.READY, reload_object(node).status)
+
     def test_PUT_updates_node(self):
         # The api allows to update a Node.
         node = factory.make_node(hostname='diane')

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-03-01 15:40:36 +0000
+++ src/maasserver/tests/test_models.py	2012-03-05 07:43:20 +0000
@@ -85,6 +85,12 @@
         self.assertEqual(user, node.owner)
         self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
 
+    def test_release(self):
+        node = factory.make_node(
+            status=NODE_STATUS.ALLOCATED, owner=factory.make_user())
+        node.release()
+        self.assertEqual((NODE_STATUS.READY, None), (node.status, node.owner))
+
 
 class NodeManagerTest(TestCase):