launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-987585 into lp:maas with lp:~jtv/maas/pre-987585-cosmetics-and-test-helper as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #987585 in MAAS: "Start Node button does not allocate node"
  https://bugs.launchpad.net/maas/+bug/987585

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-987585/+merge/103332

The need for this was discussed with Julian, if memory serves.  When applied to a node in Ready state, the Start Node action is meant not just to start it up but to acquire it first.  This is of course not desired if it's already in Allocated state.  (These are the two states for which the action is offered).  The obvious solution, given how node actions are defined, is to define slightly different actions for the two original node states.

Acquiring a node involves grabbing the OAuth key that was used to make the request.  We might get away with grabbing any old key for the user, but it seemed arbitrary; what if it's an obsolete one?  So instead, I extended the actions methods to take the UI request as an argument.

You'll note that I changed the feedback messages.  I wanted to make the difference between “I allocated this node for you and started it up” and “you have just told this node to start” explicitly clear; I hope the former conveys a proportionate sense of assumed responsibility.  The text “Node started” text was also a bit optimistic in two respects: first, it seemed to imply that startup had already completed and second, it appears to exclude any possibility that the node might not comply.  And so I changed the wording to say that the node has been _asked_ to start up.

Since there are conceivable (if hopefully improbable) circumstances under which a node action might fail, I also defined a new rule (which I also discussed with Raphaël): as a special favour, a node action may raise a MAASAPIException on failure.  Normally such an exception in a non-API request would appear to the client as a server error, but the view responsible for node actions will catch it, extract its status code, and return it to the client in at least a somewhat friendly way.


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

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-24 17:49:21 +0000
+++ src/maasserver/forms.py	2012-04-24 17:49:21 +0000
@@ -232,11 +232,25 @@
         return node
 
 
-def start_node(node, user):
+def start_node(node, user, request=None):
     """Start a node from the UI.  It will have no meta_data."""
     Node.objects.start_nodes([node.system_id], user)
 
 
+def acquire_and_start_node(node, user, request=None):
+    """Acquire and start a node from the UI.  It will have no meta_data."""
+    # Avoid circular imports.
+    from maasserver.api import get_oauth_token
+
+    node.acquire(get_oauth_token(request))
+    start_node(node=node, user=user, request=request)
+
+
+def start_commissioning_node(node, user, request=None):
+    """Start the commissioning process for a node."""
+    Node.start_commissioning(node, user)
+
+
 # Node actions per status.
 #
 # This maps each NODE_STATUS to a list of actions applicable to a node
@@ -258,7 +272,11 @@
 #     'display': "Paint node",
 #     # Permission required to perform action.
 #     'permission': NODE_PERMISSION.EDIT,
-#     # Callable that performs action.  Takes parameters (node, user).
+#     # Callable that performs action.
+#     # Takes parameters (node, user, request).
+#     # Even though this is not the API, the action may raise a
+#     # MAASAPIException if it wants to convey failure with a specific
+#     # http status code.
 #     'execute': lambda node, user: paint_node(
 #                    node, favourite_colour(user)),
 # }
@@ -268,16 +286,18 @@
         {
             'display': "Accept & commission",
             'permission': NODE_PERMISSION.ADMIN,
-            'execute': lambda node, user: Node.start_commissioning(node, user),
+            'execute': start_commissioning_node,
             'message': "Node commissioning started."
         },
     ],
     NODE_STATUS.READY: [
         {
             'display': "Start node",
-            'permission': NODE_PERMISSION.EDIT,
-            'execute': start_node,
-            'message': "Node started."
+            'permission': NODE_PERMISSION.VIEW,
+            'execute': acquire_and_start_node,
+            'message': (
+                "This node is now allocated to you.  "
+                "It has been asked to start up."),
         },
     ],
     NODE_STATUS.ALLOCATED: [
@@ -285,7 +305,7 @@
             'display': "Start node",
             'permission': NODE_PERMISSION.EDIT,
             'execute': start_node,
-            'message': "Node started."
+            'message': "The node has been asked to start up.",
         },
     ],
 }
@@ -346,7 +366,7 @@
             raise PermissionDenied()
         if not self.user.has_perm(permission, self.node):
             raise PermissionDenied()
-        execute(self.node, self.user)
+        execute(node=self.node, user=self.user, request=self.request)
         self.display_message(message)
 
 

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2012-04-24 17:49:21 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-24 17:49:21 +0000
@@ -18,6 +18,7 @@
     ValidationError,
     )
 from django.http import QueryDict
+import maasserver.api
 from maasserver.enum import (
     ARCHITECTURE,
     NODE_AFTER_COMMISSIONING_ACTION_CHOICES,
@@ -340,14 +341,19 @@
 
         self.assertRaises(PermissionDenied, form.save)
 
-    def test_start_action_starts_ready_node_for_admin(self):
+    def test_start_action_acquires_and_starts_ready_node_for_user(self):
         node = factory.make_node(status=NODE_STATUS.READY)
-        form = get_action_form(factory.make_admin())(
+        user = factory.make_user()
+        consumer, token = user.get_profile().create_authorisation_token()
+        self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
+        form = get_action_form(user)(
             node, {NodeActionForm.input_name: "Start node"})
         form.save()
 
         power_status = get_provisioning_api_proxy().power_status
         self.assertEqual('start', power_status.get(node.system_id))
+        self.assertEqual(NODE_STATUS.ALLOCATED, node.status)
+        self.assertEqual(user, node.owner)
 
     def test_start_action_starts_allocated_node_for_owner(self):
         node = factory.make_node(

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-24 17:49:21 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-24 17:49:21 +0000
@@ -19,6 +19,7 @@
 from django.core.urlresolvers import reverse
 from lxml.html import fromstring
 from maasserver import messages
+import maasserver.api
 from maasserver.enum import (
     NODE_AFTER_COMMISSIONING_ACTION,
     NODE_STATUS,
@@ -280,7 +281,7 @@
         if redirect != node_link:
             self.fail(
                 "Odd: POST on %s redirected to %s." % (node_link, redirect))
-        return self.client.get(node_link)
+        return self.client.get(redirect)
 
     def test_start_commisioning_displays_message(self):
         self.become_admin()
@@ -292,23 +293,31 @@
             [message.message for message in response.context['messages']])
 
     def test_start_node_from_ready_displays_message(self):
-        node = factory.make_node(
-            status=NODE_STATUS.READY, owner=self.logged_in_user)
-        response = self.perform_action_and_get_node_page(
-            node, "Start node")
-        self.assertIn(
-            "Node started.",
-            [message.message for message in response.context['messages']])
+        profile = self.logged_in_user.get_profile()
+        consumer, token = profile.create_authorisation_token()
+        self.patch(maasserver.api, 'get_oauth_token', lambda request: token)
+        node = factory.make_node(status=NODE_STATUS.READY)
+        response = self.perform_action_and_get_node_page(node, "Start node")
+        notices = '\n'.join([
+            message.message for message in response.context['messages']])
+        self.assertIn("This node is now allocated to you.", notices)
+        self.assertIn("asked to start up.", notices)
 
     def test_start_node_from_allocated_displays_message(self):
         node = factory.make_node(
             status=NODE_STATUS.ALLOCATED, owner=self.logged_in_user)
-        response = self.perform_action_and_get_node_page(
-            node, "Start node")
+        response = self.perform_action_and_get_node_page(node, "Start node")
         self.assertEqual(
-            ["Node started."],
+            ["The node has been asked to start up."],
             [message.message for message in response.context['messages']])
 
+    def test_start_node_without_auth_returns_Unauthorized(self):
+        node = factory.make_node(status=NODE_STATUS.READY)
+        response = self.client.post(
+            reverse('node-view', args=[node.system_id]),
+            data={NodeActionForm.input_name: "Start node"})
+        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
 
 class AdminNodeViewsTest(AdminLoggedInTestCase):
 

=== modified file 'src/maasserver/views/nodes.py'
--- src/maasserver/views/nodes.py	2012-04-24 17:49:21 +0000
+++ src/maasserver/views/nodes.py	2012-04-24 17:49:21 +0000
@@ -22,6 +22,7 @@
 from django.contrib import messages
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
+from django.http import HttpResponse
 from django.utils.safestring import mark_safe
 from django.views.generic import (
     ListView,
@@ -31,7 +32,10 @@
     NODE_PERMISSION,
     NODE_STATUS,
     )
-from maasserver.exceptions import NoRabbit
+from maasserver.exceptions import (
+    MAASAPIException,
+    NoRabbit,
+    )
 from maasserver.forms import (
     get_action_form,
     UIAdminNodeEditForm,
@@ -114,6 +118,18 @@
             node.error if node.status != NODE_STATUS.FAILED_TESTS else None)
         return context
 
+    def dispatch(self, *args, **kwargs):
+        """Override from Django `View`: Handle MAAS exceptions.
+
+        Node actions may raise exceptions derived from
+        :class:`MAASAPIException`.  This type of exception contains an
+        http status code that we will forward to the client.
+        """
+        try:
+            return super(NodeView, self).dispatch(*args, **kwargs)
+        except MAASAPIException as e:
+            return HttpResponse(unicode(e), status=e.api_error)
+
     def get_success_url(self):
         return reverse('node-view', args=[self.get_object().system_id])