launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/backport-r1287 into lp:maas/1.2.

Commit message:
Implemented NodesHandler.release() API call to release multiple nodes at once. (backport r1287 frm trunk)

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1061870 in MAAS: "No API to mass release nodes"
  https://bugs.launchpad.net/maas/+bug/1061870

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/backport-r1287/+merge/137483
-- 
https://code.launchpad.net/~julian-edwards/maas/backport-r1287/+merge/137483
Your team MAAS Maintainers is requested to review the proposed merge of lp:~julian-edwards/maas/backport-r1287 into lp:maas/1.2.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-12-03 04:46:11 +0000
+++ src/maasserver/api.py	2012-12-03 06:17:27 +0000
@@ -839,6 +839,52 @@
         nodes = [node.accept_enlistment(request.user) for node in nodes]
         return filter(None, nodes)
 
+    @operation(idempotent=False)
+    def release(self, request):
+        """Release multiple nodes.
+
+        This places the nodes back into the pool, ready to be reallocated.
+
+        :param nodes: system_ids of the nodes which are to be released.
+           (An empty list is acceptable).
+        :return: The system_ids of any nodes that have their status
+            changed by this call. Thus, nodes that were already released
+            are excluded from the result.
+        """
+        system_ids = set(request.POST.getlist('nodes'))
+         # Check the existence of these nodes first.
+        self._check_system_ids_exist(system_ids)
+        # Make sure that the user has the required permission.
+        nodes = Node.objects.get_nodes(
+            request.user, perm=NODE_PERMISSION.EDIT, ids=system_ids)
+        if len(nodes) < len(system_ids):
+            permitted_ids = set(node.system_id for node in nodes)
+            raise PermissionDenied(
+                "You don't have the required permission to release the "
+                "following node(s): %s." % (
+                    ', '.join(system_ids - permitted_ids)))
+
+        released_ids = []
+        failed = []
+        for node in nodes:
+            if node.status == NODE_STATUS.READY:
+                # Nothing to do.
+                pass
+            elif node.status in [NODE_STATUS.ALLOCATED, NODE_STATUS.RESERVED]:
+                node.release()
+                released_ids.append(node.system_id)
+            else:
+                failed.append(
+                    "%s ('%s')"
+                    % (node.system_id, node.display_status()))
+
+        if any(failed):
+            raise NodeStateViolation(
+                "Node(s) cannot be released in their current state: %s."
+                % ', '.join(failed))
+        
+        return released_ids
+        
     @operation(idempotent=True)
     def list(self, request):
         """List Nodes visible to the user, optionally filtered by criteria.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-12-03 05:12:03 +0000
+++ src/maasserver/tests/test_api.py	2012-12-03 06:17:27 +0000
@@ -2325,6 +2325,114 @@
             [node.system_id for node in acceptable_nodes], accepted_ids)
         self.assertNotIn(accepted_node.system_id, accepted_ids)
 
+    def test_POST_quietly_releases_empty_set(self):
+        response = self.client.post(self.get_uri('nodes/'), {'op': 'release'})
+        self.assertEqual(
+            (httplib.OK, "[]"), (response.status_code, response.content))
+
+    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_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id],
+                })
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertEqual(NODE_STATUS.ALLOCATED, reload_object(node).status)
+
+    def test_POST_release_fails_if_nodes_do_not_exist(self):
+         # Make sure there is a node, it just isn't among the ones to release
+        factory.make_node()
+        node_ids = {factory.getRandomString() for i in xrange(5)}
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': node_ids
+                })
+        # Awkward parsing, but the order may vary and it's not JSON
+        s = response.content
+        returned_ids = s[s.find(':')+2:s.rfind('.')].split(', ')
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        self.assertIn("Unknown node(s): ", response.content)
+        self.assertItemsEqual(node_ids, returned_ids)
+
+    def test_POST_release_forbidden_if_user_cannot_edit_node(self):
+        # Create a bunch of nodes, owned by the logged in user
+        node_ids = {
+            factory.make_node(
+                status=NODE_STATUS.ALLOCATED,
+                owner=self.logged_in_user).system_id
+            for i in xrange(3)
+            }
+        # And one with no owner
+        another_node = factory.make_node(status=NODE_STATUS.RESERVED)
+        node_ids.add(another_node.system_id)
+        
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': node_ids
+                })
+        self.assertEqual(
+            (httplib.FORBIDDEN,
+                "You don't have the required permission to release the "
+                "following node(s): %s." % another_node.system_id),
+            (response.status_code, response.content))
+        
+    def test_POST_release_rejects_impossible_state_changes(self):
+        acceptable_states = {
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.RESERVED,
+            NODE_STATUS.READY,
+            }
+        unacceptable_states = (
+            set(map_enum(NODE_STATUS).values()) - acceptable_states)
+        owner = self.logged_in_user
+        nodes = [
+            factory.make_node(status=status, owner=owner)
+            for status in unacceptable_states]
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id for node in nodes],
+                })
+        # Awkward parsing again, because a string is returned, not JSON
+        expected = [
+            "%s ('%s')" % (node.system_id, node.display_status())
+            for node in nodes
+            if node.status not in acceptable_states]
+        s = response.content
+        returned = s[s.rfind(':')+2:s.rfind('.')].split(', ') 
+        self.assertEqual(httplib.CONFLICT, response.status_code)
+        self.assertIn(
+            "Node(s) cannot be released in their current state:",
+            response.content)
+        self.assertItemsEqual(expected, returned)
+
+    def test_POST_release_returns_modified_nodes(self):
+        owner = self.logged_in_user
+        acceptable_states = {
+            NODE_STATUS.READY,
+            NODE_STATUS.ALLOCATED,
+            NODE_STATUS.RESERVED,
+            }
+        nodes = [
+            factory.make_node(status=status, owner=owner)
+            for status in acceptable_states
+            ]
+        response = self.client.post(
+            self.get_uri('nodes/'), {
+                'op': 'release',
+                'nodes': [node.system_id for node in nodes],
+                })
+        parsed_result = json.loads(response.content)
+        self.assertEqual(httplib.OK, response.status_code)
+        # The first node is READY, so shouldn't be touched
+        self.assertItemsEqual(
+            [nodes[1].system_id, nodes[2].system_id],
+            parsed_result)
+        
 
 class MACAddressAPITest(APITestCase):