Merge lp:~julian-edwards/maas/bulk_action_error_catch into lp:~maas-committers/maas/trunk

[Julian Edwards]

Ensure that BulkActionForm catches the new NodeActionError raised inside StartNode. Previously it resulted in an internal server error traceback.

[Jeroen T. Vermeulen (jtv)]

This is stretching the definition of "not actionable." You'll want to update the docstring for BulkNodeActionForm.save: it still says that this outcome is for disallowed state transitions.

Exception safety w.r.t. NodeActionError is important here. You assume that the transaction will still be valid when this happens, which is not what you'd expect for your average exception coming out of the action. You're making this part of the contract for NodeAction.execute, so document it in the docstring.

review: Approve

[Julian Edwards (julian-edwards)]

Thank you for the review.

Yes it is indeed stretching it. I was looking for a quick fix, otherwise I'd be doing a large internal API change here which I really didn't fancy.

WRT to valid transactions; I did file another bug today about this mess. The notion of what is and isn't atomic is a little frustrating. Things that are not atomic (like this) simply fail to report any useful info, and *that* is the part of the API that needs to change.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-06-17 01:31:17 +0000
+++ src/maasserver/forms.py	2014-06-17 08:56:05 +0000
@@ -1484,8 +1484,12 @@
                     if action_instance.is_permitted():
                         # Do not let execute() raise a redirect exception
                         # because this action is part of a bulk operation.
-                        action_instance.execute(allow_redirect=False)
-                        done += 1
+                        try:
+                            action_instance.execute(allow_redirect=False)
+                        except NodeActionError:
+                            not_actionable += 1
+                        else:
+                            done += 1
                     else:
                         not_permitted += 1
             else:
@@ -1521,6 +1525,12 @@
         transition was not allowed and the number of nodes for which the
         action could not be performed because the user does not have the
         required permission.
+
+        Currently, in the event of a NodeActionError this is thrown into the
+        "not actionable" bucket in lieu of an overhaul of this form to
+        properly report errors for part-failing actions.  In this case
+        the transaction will still be valid for the actions that did complete
+        successfully.
         """
         action_name = self.cleaned_data['action']
         system_ids = self.cleaned_data['system_id']

=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py	2014-06-17 08:17:59 +0000
+++ src/maasserver/tests/test_forms.py	2014-06-17 08:56:05 +0000
@@ -1680,6 +1680,25 @@
             [[], node3.system_id],
             [existing_nodes, node3_system_id])
 
+    def test_perform_action_catches_start_action_errors(self):
+        error_text = factory.getRandomString(prefix="NodeActionError")
+        exc = NodeActionError(error_text)
+        self.patch(StartNode, "execute").side_effect = exc
+        user = factory.make_user()
+        factory.make_sshkey(user)
+        node = factory.make_node(status=NODE_STATUS.READY, owner=user)
+        form = BulkNodeActionForm(
+            user=user,
+            data=dict(
+                action=StartNode.name,
+                system_id=[node.system_id]))
+
+        self.assertTrue(form.is_valid(), form._errors)
+        done, not_actionable, not_permitted = form.save()
+        self.assertEqual(
+            [0, 1, 0],
+            [done, not_actionable, not_permitted])
+
     def test_first_action_is_empty(self):
         form = BulkNodeActionForm(user=factory.make_admin())
         action = form.fields['action']