launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-987629 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #987629 in MAAS: "No way to move out of FAILED_TESTS state"
  https://bugs.launchpad.net/maas/+bug/987629

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

There was nobody around to pre-imp this with, so I just went ahead and did the absolute minimum that bug 987629 required: if a node is in Failed Tests state, allow an admin to retry the commissioning process (e.g. after swapping out bad hardware or remembering to plug the machine into the network).

In order of diff:

In the sampledata, node “moon” was in state Failed Tests but had an owner.  This is unrealistic.  A node in this state does not have an owner.  (And thus, it can be deleted which makes a lot of sense!)

In NODE_ACTIONS, I added the retry button (as well as a missing comma at the end of the last entry in the preceding dict).  All it does is send the node straight back into commissioning.  There's no need to send it back into Declared state first, which would only have added unnecessary code paths.

Why is there no need?  As you see in NODE_TRANSITIONS, all I had to do was allow the node transition from Failed Tests back to Commissioning.  In fact we didn't have any transitions from that state defined, so I defined what seemed sensible — a superset of what we actually use.

I did *not* add a “Retire” button for failed nodes, although I did allow the transition.  The reason is that we don't seem to have that option anywhere else in the UI, and we might as well add that, wherever appropriate, in one fell swoop.  Including it in this branch would water down its purpose, and doing it only for failed nodes would give a false impression that the job was done.

Oh, and finally of course there's a test.  There's no negative test to prove that non-admins can't do this; that's purely data-driven, so essentially a configuration matter.  There's little point in a test duplicating that information.

I toyed with making the Retry button require only View permission on the node, since after all at this stage there's no longer any objection to wiping its disks.  The only scenario I could think of where it mattered was if an admin owned a node that failed commissioning, and then lost admin privileges but remained a valid user of the MAAS; they could then retain responsibility for dealing with the broken node.  —And then I realized that a Failed node does not have an owner.  Never mind; Admin is nice and simple.


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

=== modified file 'src/maasserver/fixtures/dev_fixture.yaml'
--- src/maasserver/fixtures/dev_fixture.yaml	2012-03-12 07:25:31 +0000
+++ src/maasserver/fixtures/dev_fixture.yaml	2012-04-25 07:21:18 +0000
@@ -7,7 +7,7 @@
   pk: 15
 - fields: {
     created: 2012-01-24, hostname: moon, architecture: i386,
-    owner: 106, status: 2,
+    owner: null, status: 2,
     system_id: node-29d7ad70-4671-11e1-93b8-00225f89f211,
     updated: !!timestamp '2012-01-24 10:52:44.507777'}
   model: maasserver.node

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-04-25 02:23:38 +0000
+++ src/maasserver/forms.py	2012-04-25 07:21:18 +0000
@@ -287,7 +287,15 @@
             'display': "Accept & commission",
             'permission': NODE_PERMISSION.ADMIN,
             'execute': start_commissioning_node,
-            'message': "Node commissioning started."
+            'message': "Node commissioning started.",
+        },
+    ],
+    NODE_STATUS.FAILED_TESTS: [
+        {
+            'display': "Retry commissioning",
+            'permission': NODE_PERMISSION.ADMIN,
+            'execute': start_commissioning_node,
+            'message': "Started a new attempt to commission this node.",
         },
     ],
     NODE_STATUS.READY: [

=== modified file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py	2012-04-24 16:06:16 +0000
+++ src/maasserver/models/__init__.py	2012-04-25 07:21:18 +0000
@@ -159,6 +159,11 @@
         NODE_STATUS.RETIRED,
         NODE_STATUS.MISSING,
         ],
+    NODE_STATUS.FAILED_TESTS: [
+        NODE_STATUS.COMMISSIONING,
+        NODE_STATUS.MISSING,
+        NODE_STATUS.RETIRED,
+        ],
     NODE_STATUS.READY: [
         NODE_STATUS.ALLOCATED,
         NODE_STATUS.RESERVED,

=== modified file 'src/maasserver/tests/test_views_nodes.py'
--- src/maasserver/tests/test_views_nodes.py	2012-04-25 02:23:38 +0000
+++ src/maasserver/tests/test_views_nodes.py	2012-04-25 07:21:18 +0000
@@ -265,6 +265,17 @@
         self.assertEqual(
             NODE_STATUS.COMMISSIONING, reload_object(node).status)
 
+    def test_view_node_POST_admin_can_retry_failed_commissioning(self):
+        self.become_admin()
+        node = factory.make_node(status=NODE_STATUS.FAILED_TESTS)
+        node_link = reverse('node-view', args=[node.system_id])
+        response = self.client.post(
+            node_link,
+            data={NodeActionForm.input_name: "Retry commissioning"})
+        self.assertEqual(httplib.FOUND, response.status_code)
+        self.assertEqual(
+            NODE_STATUS.COMMISSIONING, reload_object(node).status)
+
     def perform_action_and_get_node_page(self, node, action_name):
         """POST to perform a node action, then load the resulting page."""
         node_link = reverse('node-view', args=[node.system_id])