Merge lp:~jtv/maas/bug-987585 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Make “Start node” action acquire the node, if it was Ready.

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

[Gavin Panella (allenap)]

Looks good :)

[1]

+def start_commissioning_node(node, user, request=None):
+ """Start the commissioning process for a node."""
+ Node.start_commissioning(node, user)

node.start_commissioning(user) ?

[2]

+ notices = '\n'.join([
+ message.message for message in response.context['messages']])

No need for the [].

review: Approve

[Jeroen T. Vermeulen (jtv)]

You came back in the middle of the night to unblock me! That's very much appreciated.

> [1]
>
> +def start_commissioning_node(node, user, request=None):
> + """Start the commissioning process for a node."""
> + Node.start_commissioning(node, user)
>
> node.start_commissioning(user) ?

Whoops. Definitely. This'll work, but it's pointless to do it that way. And it wasn't covered by tests, so I added a form test for the action.

> [2]
>
> + notices = '\n'.join([
> + message.message for message in response.context['messages']])
>
> No need for the [].

Right you are.

[Francis J. Lacoste (flacoste)]

Can we get this backported on the released branch also? I think this is a good thing to have as part of the SRU.

[Francis J. Lacoste (flacoste)]

> 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).

"Start node" shouldn't be available when the node is allocated. Although from an API possible, it's possible to acquire a node without starting it, in practice, there is no way for the user to achieve that (except by making API calls).

If you use juju bootstrap or juju deploy, the nodes will show up as 'Allocated' even though they were acquired _and_ started.

We should probably couple acquire and start, or use a separate status for nodes that were started.

[Julian Edwards (julian-edwards)]

On Wednesday 25 April 2012 19:55:21 you wrote:
> We should probably couple acquire and start, or use a separate status for
> nodes that were started.

+1 for a separate status. It would be good to track this, as we might want to
power off stuff we started at some point.

[Julian Edwards (julian-edwards)]

On Wednesday 25 April 2012 16:29:02 you wrote:
> Can we get this backported on the released branch also? I think this is a
> good thing to have as part of the SRU.

Provided you don't mean that this branch is going to be part of 12.04.1? I
want to cut a new branch for that as we're making *extensive* changes. If
those changes are too much for SRU then we have a problem and need to talk
ASAP.

[Jeroen T. Vermeulen (jtv)]

This one's not quite so easy to backport. It changes the node-action signature, and a significant portion of the changes are in code that is in different files in trunk than it was in 1.0.

[Jeroen T. Vermeulen (jtv)]

ISTM the new status should be not “started” but “deploying” — which the node would be in until it had installed its new operating system and disabled netboot. The allocating user would already be the owner of the node at this point, so that their SSH keys get set up as part of deployment, but we won't say “this node is ready for use” until we've had some kind of confirmation from the node that it's all ready.

That does mean extra machinery, of course. We may even need feedback signaling or a timeout on deployment like we do with commissioning!

[Francis J. Lacoste (flacoste)]

On 12-04-25 10:38 PM, Julian Edwards wrote:
> On Wednesday 25 April 2012 16:29:02 you wrote:
>> Can we get this backported on the released branch also? I think this is a
>> good thing to have as part of the SRU.
>
> Provided you don't mean that this branch is going to be part of 12.04.1? I
> want to cut a new branch for that as we're making *extensive* changes. If
> those changes are too much for SRU then we have a problem and need to talk
> ASAP.

What will be in 12.04.01 is still in the air. My initial understanding
was that it would be based on trunk, but it seems the Ubuntu release
team wants to stick to the SRU process and thus it might be just the bug
fixes and other small fixes based on the branch you already cut.

Don't worry about this, if they want to stick to the bug-fix only
branch, it just means that 12.04.01 will be closer to 12.04 than
originally anticipated. We should stick to trunk development, but
backport small fixes that aren't too much of a problem.

What will be 12.04.01 will be settled next week.

--
Francis J. Lacoste
<email address hidden>

[Julian Edwards (julian-edwards)]

On Thursday 26 April 2012 14:16:18 you wrote:
> What will be in 12.04.01 is still in the air. My initial understanding
> was that it would be based on trunk, but it seems the Ubuntu release
> team wants to stick to the SRU process and thus it might be just the bug
> fixes and other small fixes based on the branch you already cut.
>
> Don't worry about this, if they want to stick to the bug-fix only
> branch, it just means that 12.04.01 will be closer to 12.04 than
> originally anticipated. We should stick to trunk development, but
> backport small fixes that aren't too much of a problem.
>
> What will be 12.04.01 will be settled next week.

IMO the 1.0 branch should only contain
 * security fixes
 * serious bug fixes

Most other things are a pain to backport already (jtv wasted a lot of time
backporting something yesterday). If the Ubuntu guys won't take trunk in an
SRU this will be problematic for continuing features being added in 12.04.*

Cheers.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-24 17:07:03 +0000
+++ src/maasserver/forms.py	2012-04-25 02:26:22 +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(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:07:03 +0000
+++ src/maasserver/tests/test_forms.py	2012-04-25 02:26:22 +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(
@@ -359,6 +365,14 @@
         power_status = get_provisioning_api_proxy().power_status
         self.assertEqual('start', power_status.get(node.system_id))
 
+    def test_accept_and_commission_starts_commissioning(self):
+        admin = factory.make_admin()
+        node = factory.make_node(status=NODE_STATUS.DECLARED)
+        form = get_action_form(admin)(
+            node, {NodeActionForm.input_name: "Accept & commission"})
+        form.save()
+        self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+
 
 class TestHostnameFormField(TestCase):
 

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-24 17:07:03 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-25 02:26:22 +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:07:03 +0000
+++ src/maasserver/views/nodes.py	2012-04-25 02:26:22 +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])