From df5261bb349dd56ac88876e14779d244bdc0beb3 Mon Sep 17 00:00:00 2001
From: Raphael Glon <raphael.glon@ovh.net>
Date: Fri, 28 Dec 2018 16:15:32 +0100
Subject: [PATCH] Ansible module: fix clean error handling

It should not be up to the driver to handle the error. The error should
reach the manager. Moreover, handling the error in the driver and
returning nothing caused the manager to consider the step done and go to
the next one instead of interrupting the cleaning workflow

Change-Id: I3825838b5507bc735d983466aa3cac0edd4dfaca
Story: #2005357
Task: #30315
---
 ironic/drivers/modules/ansible/deploy.py          | 15 ++++-----------
 .../unit/drivers/modules/ansible/test_deploy.py   | 11 ++++-------
 .../notes/bug-30315-e46eafe5b575f3da.yaml         |  8 ++++++++
 3 files changed, 16 insertions(+), 18 deletions(-)
 create mode 100644 releasenotes/notes/bug-30315-e46eafe5b575f3da.yaml

diff --git a/ironic/drivers/modules/ansible/deploy.py b/ironic/drivers/modules/ansible/deploy.py
index 3e424206bb..cdd211cb8f 100644
--- a/ironic/drivers/modules/ansible/deploy.py
+++ b/ironic/drivers/modules/ansible/deploy.py
@@ -526,17 +526,10 @@ class AnsibleDeploy(agent_base.HeartbeatMixin, base.DeployInterface):
         LOG.debug('Starting cleaning step %(step)s on node %(node)s',
                   {'node': node.uuid, 'step': stepname})
         step_tags = step['args'].get('tags', [])
-        try:
-            _run_playbook(node, playbook, extra_vars, key, tags=step_tags)
-        except exception.InstanceDeployFailure as e:
-            LOG.error("Ansible failed cleaning step %(step)s "
-                      "on node %(node)s.",
-                      {'node': node.uuid, 'step': stepname})
-            manager_utils.cleaning_error_handler(task, six.text_type(e))
-        else:
-            LOG.info('Ansible completed cleaning step %(step)s '
-                     'on node %(node)s.',
-                     {'node': node.uuid, 'step': stepname})
+        _run_playbook(node, playbook, extra_vars, key, tags=step_tags)
+        LOG.info('Ansible completed cleaning step %(step)s '
+                 'on node %(node)s.',
+                 {'node': node.uuid, 'step': stepname})
 
     @METRICS.timer('AnsibleDeploy.prepare_cleaning')
     def prepare_cleaning(self, task):
diff --git a/ironic/tests/unit/drivers/modules/ansible/test_deploy.py b/ironic/tests/unit/drivers/modules/ansible/test_deploy.py
index 49050a7944..4c114326ef 100644
--- a/ironic/tests/unit/drivers/modules/ansible/test_deploy.py
+++ b/ironic/tests/unit/drivers/modules/ansible/test_deploy.py
@@ -713,11 +713,10 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
     @mock.patch.object(ansible_deploy, '_parse_ansible_driver_info',
                        return_value=('test_pl', 'test_u', 'test_k'),
                        autospec=True)
-    @mock.patch.object(utils, 'cleaning_error_handler', autospec=True)
     @mock.patch.object(ansible_deploy, '_run_playbook', autospec=True)
     @mock.patch.object(ansible_deploy, 'LOG', autospec=True)
     def test_execute_clean_step_no_success_log(
-            self, log_mock, run_mock, utils_mock, parse_driver_info_mock):
+            self, log_mock, run_mock, parse_driver_info_mock):
 
         run_mock.side_effect = exception.InstanceDeployFailure('Boom')
         step = {'priority': 10, 'interface': 'deploy',
@@ -727,11 +726,9 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase):
         self.node.driver_internal_info = di_info
         self.node.save()
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.driver.execute_clean_step(task, step)
-            log_mock.error.assert_called_once_with(
-                mock.ANY, {'node': task.node['uuid'],
-                           'step': 'erase_devices'})
-            utils_mock.assert_called_once_with(task, 'Boom')
+            self.assertRaises(exception.InstanceDeployFailure,
+                              self.driver.execute_clean_step,
+                              task, step)
             self.assertFalse(log_mock.info.called)
 
     @mock.patch.object(ansible_deploy, '_run_playbook', autospec=True)
diff --git a/releasenotes/notes/bug-30315-e46eafe5b575f3da.yaml b/releasenotes/notes/bug-30315-e46eafe5b575f3da.yaml
new file mode 100644
index 0000000000..52020517cb
--- /dev/null
+++ b/releasenotes/notes/bug-30315-e46eafe5b575f3da.yaml
@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+      Fixes an issue regarding the ``ansible deployment interface`` cleaning
+      workflow.
+      Handling the error in the driver and returning nothing caused the manager
+      to consider the step done and go to the next one instead of interrupting
+      the cleaning workflow.