Merge lp:~andreserl/maas/fix_lp1441408_lp1386504 into lp:~maas-committers/maas/trunk

[Andres Rodriguez]

If a machine is already powered ON when executing the 'commissioning' action (both via UI and CLI), power cycle the machine instead of failing to start commissioning.

[Blake Rouse (blake-rouse)]

Looks good. One test looks like it needs an improvement.

review: Approve

[Andres Rodriguez (andreserl)]

Replied to comment inline.

[Blake Rouse (blake-rouse)]

This looks good. My follow up branch will add another tests.

review: Approve

=== modified file 'src/maasserver/forms_commission.py'
--- src/maasserver/forms_commission.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/forms_commission.py	2016-11-24 14:26:12 +0000
@@ -9,7 +9,6 @@
 
 from django import forms
 from django.core.exceptions import ValidationError
-from maasserver.enum import POWER_STATE
 from maasserver.node_action import compile_node_actions
 
 
@@ -36,10 +35,6 @@
             raise ValidationError(
                 "Commission is not available because of the current state "
                 "of the node.")
-        if self.instance.power_state == POWER_STATE.ON:
-            raise ValidationError(
-                "Commission is not available because of the node is currently "
-                "powered on.")
         return cleaned_data
 
     def save(self):

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-11-11 15:50:38 +0000
+++ src/maasserver/models/node.py	2016-11-24 14:26:12 +0000
@@ -3134,7 +3134,10 @@
 
         # Request that the node be powered on post-commit.
         d = post_commit()
-        d = self._power_control_node(d, power_on_node, power_info)
+        if self.power_state == POWER_STATE.ON:
+            d = self._power_control_node(d, power_cycle, power_info)
+        else:
+            d = self._power_control_node(d, power_on_node, power_info)
 
         # Set the deployment timeout so the node is marked failed after
         # a period of time.

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-11-11 17:21:57 +0000
+++ src/maasserver/models/tests/test_node.py	2016-11-24 14:26:12 +0000
@@ -2304,6 +2304,27 @@
         post_commit_hooks.reset()  # Ignore these for now.
         self.assertEqual('hwe-v', node.min_hwe_kernel)
 
+    def test_start_commissioning_starts_node_if_already_on(self):
+        node = factory.make_Node(
+            interface=True, status=NODE_STATUS.NEW, power_type='manual',
+            power_state=POWER_STATE.ON)
+        node_start = self.patch(node, '_start')
+        # Return a post-commit hook from Node.start().
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
+        admin = factory.make_admin()
+        node.start_commissioning(admin)
+        post_commit_hooks.reset()  # Ignore these for now.
+        node = reload_object(node)
+        expected_attrs = {
+            'status': NODE_STATUS.COMMISSIONING,
+            'owner': admin,
+        }
+        self.assertAttributes(node, expected_attrs)
+        self.expectThat(node.owner, Equals(admin))
+        self.assertThat(node_start, MockCalledOnceWith(
+            admin, ANY, NODE_STATUS.NEW))
+
     def test_start_commissioning_clears_node_commissioning_results(self):
         node = factory.make_Node(status=NODE_STATUS.NEW)
         node_start = self.patch(node, '_start')

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2016-08-16 09:31:16 +0000
+++ src/maasserver/node_action.py	2016-11-24 14:26:12 +0000
@@ -230,10 +230,6 @@
             self, enable_ssh=False, skip_networking=False,
             skip_storage=False):
         """See `NodeAction.execute`."""
-        if self.node.power_state == POWER_STATE.ON:
-            raise NodeActionError(
-                "Unable to be commissioned because the power is currently on.")
-
         try:
             self.node.start_commissioning(
                 self.user,

=== modified file 'src/maasserver/tests/test_forms_commission.py'
--- src/maasserver/tests/test_forms_commission.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/tests/test_forms_commission.py	2016-11-24 14:26:12 +0000
@@ -36,17 +36,20 @@
                 "of the node."],
             }, form.errors)
 
-    def test__not_allowed_if_on(self):
+    def test__calls_start_commissioning_if_already_on(self):
         node = factory.make_Node(
             status=NODE_STATUS.READY, power_state=POWER_STATE.ON)
         user = factory.make_admin()
+        mock_start_commissioning = self.patch_autospec(
+            node, "start_commissioning")
         form = CommissionForm(instance=node, user=user, data={})
-        self.assertFalse(form.is_valid(), form.errors)
-        self.assertEqual({
-            '__all__': [
-                "Commission is not available because of the node is currently "
-                "powered on."],
-            }, form.errors)
+        self.assertTrue(form.is_valid(), form.errors)
+        node = form.save()
+        self.assertThat(
+            mock_start_commissioning,
+            MockCalledOnceWith(
+                user, enable_ssh=False, skip_networking=False,
+                skip_storage=False))
 
     def test__calls_start_commissioning_with_options(self):
         node = factory.make_Node(

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2016-11-11 15:50:38 +0000
+++ src/maasserver/tests/test_node_action.py	2016-11-24 14:26:12 +0000
@@ -281,13 +281,20 @@
         ("READY", {"status": NODE_STATUS.READY}),
     )
 
-    def test_raise_NodeActionError_if_on(self):
+    def test_Commission_starts_commissioning_if_already_on(self):
         node = factory.make_Node(
-            status=self.status, power_state=POWER_STATE.ON)
-        user = factory.make_admin()
-        action = Commission(node, user)
-        self.assertTrue(action.is_permitted())
-        self.assertRaises(NodeActionError, action.execute)
+            interface=True, status=self.status,
+            power_type='manual', power_state=POWER_STATE.ON)
+        node_start = self.patch(node, '_start')
+        node_start.side_effect = (
+            lambda user, user_data, old_status: post_commit())
+        admin = factory.make_admin()
+        action = Commission(node, admin)
+        with post_commit_hooks:
+            action.execute()
+        self.assertEqual(NODE_STATUS.COMMISSIONING, node.status)
+        self.assertThat(
+            node_start, MockCalledOnceWith(admin, ANY, ANY))
 
     def test_Commission_starts_commissioning(self):
         node = factory.make_Node(