Merge lp:~blake-rouse/maas/no-power-action-when-off into lp:~maas-committers/maas/trunk

[Blake Rouse]

Stops performing the stop power action during the release action when the node is already off.

This stops a power action from being registered in the power registry when the node is already off and is transitioned directly to ready. Branch also includes a drive-by fix for a test that can random fail from a recent change that was made.

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2015-05-13 19:20:54 +0000
+++ src/maasserver/models/node.py	2015-05-21 19:35:09 +0000
@@ -1578,13 +1578,19 @@
         """Mark allocated or reserved node as available again and power off.
         """
         maaslog.info("%s: Releasing node", self.hostname)
-        try:
-            self.stop(self.owner)
-        except Exception as ex:
-            maaslog.error(
-                "%s: Unable to shut node down: %s", self.hostname,
-                unicode(ex))
-            raise
+
+        # Don't perform stop the node if its already off. Doing so will
+        # place an action in the power registry which is not needed and can
+        # block a following deploy action. See bug 1453954 for an example of
+        # the issue this will cause.
+        if self.power_state != POWER_STATE.OFF:
+            try:
+                self.stop(self.owner)
+            except Exception as ex:
+                maaslog.error(
+                    "%s: Unable to shut node down: %s", self.hostname,
+                    unicode(ex))
+                raise
 
         deallocate_ip_address = True
         if self.power_state == POWER_STATE.OFF:

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2015-05-16 06:21:20 +0000
+++ src/maasserver/models/tests/test_node.py	2015-05-21 19:35:09 +0000
@@ -939,9 +939,11 @@
         owner = factory.make_User()
         node = factory.make_Node(
             status=NODE_STATUS.ALLOCATED, owner=owner, agent_name=agent_name)
+        self.patch(node, 'stop')
         self.patch(node, 'start_transition_monitor')
         node.power_state = POWER_STATE.OFF
         node.release()
+        self.expectThat(node.stop, MockNotCalled())
         self.expectThat(node.start_transition_monitor, MockNotCalled())
         self.expectThat(node.status, Equals(NODE_STATUS.READY))
         self.expectThat(node.owner, Is(None))
@@ -1180,16 +1182,27 @@
         self.assertEqual("", node.osystem)
         self.assertEqual("", node.distro_series)
 
-    def test_release_powers_off_node(self):
+    def test_release_powers_off_node_when_on(self):
         user = factory.make_User()
         node = factory.make_Node(
-            status=NODE_STATUS.ALLOCATED, owner=user, power_type='virsh')
+            status=NODE_STATUS.ALLOCATED, owner=user, power_type='virsh',
+            power_state=POWER_STATE.ON)
         self.patch(node, 'start_transition_monitor')
         node_stop = self.patch(node, 'stop')
         node.release()
         self.assertThat(
             node_stop, MockCalledOnceWith(user))
 
+    def test_release_doesnt_power_off_node_when_off(self):
+        user = factory.make_User()
+        node = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=user, power_type='virsh',
+            power_state=POWER_STATE.OFF)
+        self.patch(node, 'start_transition_monitor')
+        node_stop = self.patch(node, 'stop')
+        node.release()
+        self.assertThat(node_stop, MockNotCalled())
+
     def test_release_deallocates_static_ip_when_node_is_off(self):
         user = factory.make_User()
         node = factory.make_Node_with_MACAddress_and_NodeGroupInterface(
@@ -1255,7 +1268,8 @@
         self.assertThat(dns_update_zones, MockCalledOnceWith([node.nodegroup]))
 
     def test_release_logs_and_raises_errors_in_stopping(self):
-        node = factory.make_Node(status=NODE_STATUS.DEPLOYED)
+        node = factory.make_Node(
+            status=NODE_STATUS.DEPLOYED, power_state=POWER_STATE.ON)
         maaslog = self.patch(node_module, 'maaslog')
         exception_class = factory.make_exception_type()
         exception = exception_class(factory.make_name())
@@ -1272,6 +1286,7 @@
         # will leave the node in its previous state (i.e. DEPLOYED).
         node = factory.make_Node(
             status=NODE_STATUS.DEPLOYED, power_type="virsh",
+            power_state=POWER_STATE.ON,
             owner=factory.make_User())
         node_stop = self.patch(node, 'stop')
         node_stop.side_effect = factory.make_exception()
@@ -1671,7 +1686,7 @@
         self.assertEqual(expected_hostname, node.fqdn)
 
     def test_boot_type_has_fastpath_set_by_default(self):
-        node = factory.make_Node()
+        node = Node()
         self.assertEqual(NODE_BOOT.FASTPATH, node.boot_type)
 
     def test_split_arch_returns_arch_as_tuple(self):

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2015-05-11 09:51:30 +0000
+++ src/maasserver/tests/test_node_action.py	2015-05-21 19:35:09 +0000
@@ -733,9 +733,10 @@
         self.patch_autospec(node, 'start_transition_monitor')
         self.patch_autospec(node, 'stop_transition_monitor')
 
-    def make_action(self, action_class, node_status):
+    def make_action(self, action_class, node_status, power_state=None):
         node = factory.make_Node(
-            mac=True, status=node_status, power_type='ether_wake')
+            mac=True, status=node_status, power_type='ether_wake',
+            power_state=power_state)
         admin = factory.make_admin()
         return action_class(node, admin)
 
@@ -781,7 +782,8 @@
             unicode(exception))
 
     def test_Release_handles_rpc_errors(self):
-        action = self.make_action(Release, NODE_STATUS.ALLOCATED)
+        action = self.make_action(
+            Release, NODE_STATUS.ALLOCATED, power_state=POWER_STATE.ON)
         self.patch_rpc_methods(action.node)
         exception = self.assertRaises(NodeActionError, action.execute)
         self.assertEqual(