Merge lp:~gmb/maas/bug-1274912 into lp:~maas-committers/maas/trunk

[Graham Binns]

Trying to stop a node which has no power type configured will no longer result in an unhandled ValueError.

This branch fixes bug 1274912 by correctly handling ValueErrors from get_effective_power_type() in stop_nodes(). If a ValueError is encountered, stop_nodes() will simply skip creating a power event for the node.

[Raphaël Badin (rvb)]

[0]

11 + except ValueError:
12 + continue

It's worth a warning. (Let's also add one in start_nodes please.)

review: Approve

[Jeroen T. Vermeulen (jtv)]

As discussed on IRC: let's try not to catch such generic errors coming out of functions that may raise them for other reasons! And if we have to do it in multiple places that call get_effective_power_type(), that suggests that the function could do more to simplify this.

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Raphaël Badin (rvb)]

No warning for me I guess :/

[Graham Binns (gmb)]

On 31 January 2014 14:39, Raphaël Badin <email address hidden> wrote:
> No warning for me I guess :/

Crap, I didn't see your comment, only Jeroen's sorry. E_NOT_LOOKING.
You're right, and I'll add a warning now.

[Jeroen T. Vermeulen (jtv)]

Just one more thing from me: changing the exception type does not break any tests? Looks like that behaviour of get_effective_power_type() is untested! Could you rectify?

With apologies for the anality of it all. :)

review: Approve

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-01-29 10:08:12 +0000
+++ src/maasserver/models/node.py	2014-01-31 14:22:50 +0000
@@ -150,6 +150,10 @@
     }
 
 
+class UnknownPowerType(Exception):
+    """Raised when a node has an unknown power type."""
+
+
 class NodeManager(Manager):
     """A utility to manage the collection of Nodes."""
 
@@ -297,7 +301,12 @@
         processed_nodes = []
         for node in nodes:
             power_params = node.get_effective_power_parameters()
-            node_power_type = node.get_effective_power_type()
+            try:
+                node_power_type = node.get_effective_power_type()
+            except UnknownPowerType:
+                # Skip the rest of the loop to avoid creating a power
+                # event for a node that we can't power down.
+                continue
             # WAKE_ON_LAN does not support poweroff.
             if node_power_type != 'ether_wake':
                 power_off.apply_async(
@@ -337,7 +346,9 @@
             power_params = node.get_effective_power_parameters()
             try:
                 node_power_type = node.get_effective_power_type()
-            except ValueError:
+            except UnknownPowerType:
+                # Skip the rest of the loop to avoid creating a power
+                # event for a node that we can't power up.
                 continue
             if node_power_type == 'ether_wake':
                 mac = power_params.get('mac_address')
@@ -686,10 +697,11 @@
     def get_effective_power_type(self):
         """Get power-type to use for this node.
 
-        If no power type has been set for the node, raise an error.
+        If no power type has been set for the node, raise
+        UnknownPowerType.
         """
         if self.power_type == '':
-            raise ValueError("Node power type is unconfigured")
+            raise UnknownPowerType("Node power type is unconfigured")
         return self.power_type
 
     def get_primary_mac(self):

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-01-21 06:27:45 +0000
+++ src/maasserver/models/tests/test_node.py	2014-01-31 14:22:50 +0000
@@ -230,7 +230,8 @@
 
     def test_get_effective_power_type_raises_if_not_set(self):
         node = factory.make_node(power_type='')
-        self.assertRaises(ValueError, node.get_effective_power_type)
+        self.assertRaises(
+            node_module.UnknownPowerType, node.get_effective_power_type)
 
     def test_get_effective_power_type_reads_node_field(self):
         power_types = list(get_power_types().keys())  # Python3 proof.
@@ -1017,6 +1018,18 @@
             [stoppable_node],
             Node.objects.stop_nodes(ids, stoppable_node.owner))
 
+    def test_stop_nodes_does_not_attempt_power_task_if_no_power_type(self):
+        # If the node has a power_type set to UNKNOWN_POWER_TYPE
+        # NodeManager.stop_node(this_node) won't create a power event
+        # for it.
+        user = factory.make_user()
+        node, unused = self.make_node_with_mac(
+            user, power_type='')
+        output = Node.objects.stop_nodes([node.system_id], user)
+
+        self.assertItemsEqual([], output)
+        self.assertEqual(0, len(self.celery.tasks))
+
     def test_start_nodes_starts_nodes(self):
         user = factory.make_user()
         node, mac = self.make_node_with_mac(