Merge "Address comments on cleaning commit"

2 files changed, 42 insertions, 10 deletions

diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index e3adc03f8..d77e65d04 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -954,7 +954,7 @@ class ConductorManager(periodic_task.PeriodicTasks):
                 # Kill this worker, the async step will make an RPC call to
                 # continue_node_clean to continue cleaning
                 LOG.debug('Waiting for node %(node)s to call continue after '
-                          'async clean step %(step)s' %
+                          'async clean step %(step)s',
                           {'node': node.uuid, 'step': step})
                 return
             elif result is not None:
@@ -962,7 +962,7 @@ class ConductorManager(periodic_task.PeriodicTasks):
                          '%(node)s, step returned invalid value: %(val)s') %
                        {'step': step, 'node': node.uuid, 'val': result})
                 LOG.error(msg)
-                cleaning_error_handler(task, msg)
+                return cleaning_error_handler(task, msg)
             LOG.info(_LI('Node %(node)s finished clean step %(step)s'),
                      {'node': node.uuid, 'step': step})
 
@@ -2082,15 +2082,14 @@ def _get_cleaning_steps(task, enabled=False):
     :returns: A list of clean steps dictionaries, sorted with largest priority
         as the first item
     """
-
-    steps = list()
     # Iterate interfaces and get clean steps from each
-    for interface in CLEANING_INTERFACE_PRIORITY.keys():
-        driver_interface = getattr(task.driver, interface)
-        if driver_interface:
-            for step in driver_interface.get_clean_steps(task):
-                if not enabled or step['priority'] > 0:
-                    steps.append(step)
+    steps = list()
+    for interface in CLEANING_INTERFACE_PRIORITY:
+        interface = getattr(task.driver, interface)
+        if interface:
+            interface_steps = [x for x in interface.get_clean_steps(task)
+                               if not enabled or x['priority'] > 0]
+            steps.extend(interface_steps)
     # Sort the steps from higher priority to lower priority
     return sorted(steps, key=_step_key, reverse=True)
 
diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py
index 2e49090c5..9316adfea 100644
--- a/ironic/tests/conductor/test_manager.py
+++ b/ironic/tests/conductor/test_manager.py
@@ -1925,6 +1925,39 @@ class DoNodeCleanTestCase(_ServiceSetUpMixin, tests_db_base.DbTestCase):
         self.assertEqual({}, node.clean_step)
         mock_execute.assert_not_called()
 
+    @mock.patch('ironic.drivers.modules.fake.FakePower.execute_clean_step')
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.execute_clean_step')
+    def test__do_next_clean_step_bad_step_return_value(
+            self, deploy_exec_mock, power_exec_mock):
+        # When a clean step fails, go to CLEANFAIL
+        node = obj_utils.create_test_node(
+            self.context, driver='fake',
+            provision_state=states.CLEANING,
+            target_provision_state=states.AVAILABLE,
+            last_error=None,
+            clean_step={})
+        deploy_exec_mock.return_value = "foo"
+
+        self._start_service()
+
+        with task_manager.acquire(
+                self.context, node['id'], shared=False) as task:
+            self.service._do_next_clean_step(
+                task, self.clean_steps, node.clean_step)
+
+        self.service._worker_pool.waitall()
+        node.refresh()
+
+        # Make sure we go to CLEANFAIL, clear clean_steps
+        self.assertEqual(states.CLEANFAIL, node.provision_state)
+        self.assertEqual({}, node.clean_step)
+        self.assertIsNotNone(node.last_error)
+        self.assertTrue(node.maintenance)
+        deploy_exec_mock.assert_called_once_with(mock.ANY,
+                                                 self.clean_steps[0])
+        # Make sure we don't execute any other step and return
+        self.assertFalse(power_exec_mock.called)
+
     @mock.patch('ironic.conductor.manager._get_cleaning_steps')
     def test_set_node_cleaning_steps(self, mock_steps):
         mock_steps.return_value = self.clean_steps