Remove true/false return from action.execute()

In watcher/applier/workflow_engine/default.py, we are checking the
return value of action.execute(). As the "TODO" above indicates it
(line 118), we should get rid of this and only flag an action as
failed if an exception was raised during its execute(). We will
need to update the related unit tests.

Change-Id: Ia8ff7abd9994c3504e733ccd1d629cafe9d4b839
Closes-Bug: #1548383

watcher/applier/action_plan/default.py

@@ -53,16 +53,15 @@ class DefaultActionPlanHandler(base.BaseActionPlanHandler):
                         event_types.EventTypes.LAUNCH_ACTION_PLAN,
                         ap_objects.State.ONGOING)
             applier = default.DefaultApplier(self.ctx, self.applier_manager)
-            result = applier.execute(self.action_plan_uuid)
+            applier.execute(self.action_plan_uuid)
+            state = ap_objects.State.SUCCEEDED
+
         except Exception as e:
             LOG.exception(e)
-            result = False
+            state = ap_objects.State.FAILED
+
         finally:
-            if result is True:
-                status = ap_objects.State.SUCCEEDED
-            else:
-                status = ap_objects.State.FAILED
             # update state
             self.notify(self.action_plan_uuid,
                         event_types.EventTypes.LAUNCH_ACTION_PLAN,
-                        status)
+                        state)

watcher/applier/workflow_engine/default.py

@@ -22,6 +22,7 @@ from taskflow import task
 
 from watcher._i18n import _LE, _LW, _LC
 from watcher.applier.workflow_engine import base
+from watcher.common import exception
 from watcher.objects import action as obj_action
 
 LOG = log.getLogger(__name__)
@@ -77,10 +78,9 @@ class DefaultWorkFlowEngine(base.BaseWorkFlowEngine):
 
             e = engines.load(flow)
             e.run()
-            return True
+
         except Exception as e:
-            LOG.exception(e)
+            raise exception.WorkflowExecutionException(error=e)
-            return False
 
 
 class TaskFlowActionContainer(task.Task):
@@ -121,14 +121,9 @@ class TaskFlowActionContainer(task.Task):
         try:
             LOG.debug("Running action %s", self.name)
 
-            # todo(jed) remove return (true or false) raise an Exception
+            self.action.execute()
-            result = self.action.execute()
+            self.engine.notify(self._db_action,
-            if result is not True:
+                               obj_action.State.SUCCEEDED)
-                self.engine.notify(self._db_action,
-                                   obj_action.State.FAILED)
-            else:
-                self.engine.notify(self._db_action,
-                                   obj_action.State.SUCCEEDED)
         except Exception as e:
             LOG.exception(e)
             LOG.error(_LE('The WorkFlow Engine has failed '

watcher/common/exception.py

@@ -234,6 +234,9 @@ class PatchError(Invalid):
 
 # decision engine
 
+class WorkflowExecutionException(WatcherException):
+    msg_fmt = _('Workflow execution error: %(error)s')
+
 
 class IllegalArgumentException(WatcherException):
     msg_fmt = _('Illegal argument')

watcher/locale/watcher.pot

@@ -7,9 +7,9 @@
 #, fuzzy
 msgid ""
 msgstr ""
-"Project-Id-Version: python-watcher 0.24.1.dev4\n"
+"Project-Id-Version: python-watcher 0.24.1.dev12\n"
 "Report-Msgid-Bugs-To: EMAIL@ADDRESS\n"
-"POT-Creation-Date: 2016-03-14 15:29+0100\n"
+"POT-Creation-Date: 2016-03-16 18:18-0500\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -109,17 +109,17 @@ msgstr ""
 msgid "Migration of type %(migration_type)s is not supported."
 msgstr ""
 
-#: watcher/applier/workflow_engine/default.py:134
+#: watcher/applier/workflow_engine/default.py:129
 #, python-format
 msgid "The WorkFlow Engine has failed to execute the action %s"
 msgstr ""
 
-#: watcher/applier/workflow_engine/default.py:152
+#: watcher/applier/workflow_engine/default.py:147
 #, python-format
 msgid "Revert action %s"
 msgstr ""
 
-#: watcher/applier/workflow_engine/default.py:158
+#: watcher/applier/workflow_engine/default.py:153
 msgid "Oops! We need disaster recover plan"
 msgstr ""
 
@@ -292,64 +292,69 @@ msgstr ""
 msgid "Couldn't apply patch '%(patch)s'. Reason: %(reason)s"
 msgstr ""
 
-#: watcher/common/exception.py:239
+#: watcher/common/exception.py:238
+#, python-format
+msgid "Workflow execution error: %(error)s"
+msgstr ""
+
+#: watcher/common/exception.py:242
 msgid "Illegal argument"
 msgstr ""
 
-#: watcher/common/exception.py:243
+#: watcher/common/exception.py:246
 msgid "No such metric"
 msgstr ""
 
-#: watcher/common/exception.py:247
+#: watcher/common/exception.py:250
 msgid "No rows were returned"
 msgstr ""
 
-#: watcher/common/exception.py:251
+#: watcher/common/exception.py:254
 #, python-format
 msgid "%(client)s connection failed. Reason: %(reason)s"
 msgstr ""
 
-#: watcher/common/exception.py:255
+#: watcher/common/exception.py:258
 msgid "'Keystone API endpoint is missing''"
 msgstr ""
 
-#: watcher/common/exception.py:259
+#: watcher/common/exception.py:262
 msgid "The list of hypervisor(s) in the cluster is empty"
 msgstr ""
 
-#: watcher/common/exception.py:263
+#: watcher/common/exception.py:266
 msgid "The metrics resource collector is not defined"
 msgstr ""
 
-#: watcher/common/exception.py:267
+#: watcher/common/exception.py:270
 msgid "the cluster state is not defined"
 msgstr ""
 
-#: watcher/common/exception.py:273
+#: watcher/common/exception.py:276
 #, python-format
 msgid "The instance '%(name)s' is not found"
 msgstr ""
 
-#: watcher/common/exception.py:277
+#: watcher/common/exception.py:280
 msgid "The hypervisor is not found"
 msgstr ""
 
-#: watcher/common/exception.py:281
+#: watcher/common/exception.py:284
 #, python-format
 msgid "Error loading plugin '%(name)s'"
 msgstr ""
 
-#: watcher/common/exception.py:285
+#: watcher/common/exception.py:288
 #, python-format
 msgid "The identifier '%(name)s' is a reserved word"
 msgstr ""
 
-#: watcher/common/exception.py:289
+#: watcher/common/exception.py:292
 #, python-format
 msgid "The %(name)s resource %(id)s is not soft deleted"
 msgstr ""
 
-#: watcher/common/exception.py:293
+#: watcher/common/exception.py:296
 msgid "Limit should be positive"
 msgstr ""
 

watcher/tests/applier/workflow_engine/test_default_workflow_engine.py

@@ -25,6 +25,7 @@ from stevedore import extension
 
 from watcher.applier.actions import base as abase
 from watcher.applier.workflow_engine import default as tflow
+from watcher.common import exception
 from watcher.common import utils
 from watcher import objects
 from watcher.tests.db import base
@@ -63,10 +64,15 @@ class TestDefaultWorkFlowEngine(base.DbTestCase):
             context=self.context,
             applier_manager=mock.MagicMock())
 
-    def test_execute(self):
+    @mock.patch('taskflow.engines.load')
+    @mock.patch('taskflow.patterns.graph_flow.Flow.link')
+    def test_execute(self, graph_flow, engines):
         actions = mock.MagicMock()
-        result = self.engine.execute(actions)
+        try:
-        self.assertEqual(True, result)
+            self.engine.execute(actions)
+            self.assertTrue(engines.called)
+        except Exception as exc:
+            self.fail(exc)
 
     def create_action(self, action_type, parameters, next):
         action = {
@@ -91,64 +97,84 @@ class TestDefaultWorkFlowEngine(base.DbTestCase):
         for a in actions:
             self.check_action_state(a, expected_state)
 
-    def test_execute_with_no_actions(self):
+    @mock.patch('taskflow.engines.load')
+    @mock.patch('taskflow.patterns.graph_flow.Flow.link')
+    def test_execute_with_no_actions(self, graph_flow, engines):
         actions = []
-        result = self.engine.execute(actions)
+        try:
-        self.assertEqual(True, result)
+            self.engine.execute(actions)
+            self.assertFalse(graph_flow.called)
+            self.assertTrue(engines.called)
+        except Exception as exc:
+            self.fail(exc)
 
     def test_execute_with_one_action(self):
         actions = [self.create_action("nop", {'message': 'test'}, None)]
-        result = self.engine.execute(actions)
+        try:
-        self.assertEqual(True, result)
+            self.engine.execute(actions)
-        self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+            self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+
+        except Exception as exc:
+            self.fail(exc)
 
     def test_execute_with_two_actions(self):
         actions = []
-        next = self.create_action("sleep", {'duration': 0.0}, None)
+        second = self.create_action("sleep", {'duration': 0.0}, None)
-        first = self.create_action("nop", {'message': 'test'}, next.id)
+        first = self.create_action("nop", {'message': 'test'}, second.id)
 
         actions.append(first)
-        actions.append(next)
+        actions.append(second)
 
-        result = self.engine.execute(actions)
+        try:
-        self.assertEqual(True, result)
+            self.engine.execute(actions)
-        self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+            self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+
+        except Exception as exc:
+            self.fail(exc)
 
     def test_execute_with_three_actions(self):
         actions = []
-        next2 = self.create_action("nop", {'message': 'next'}, None)
+
-        next = self.create_action("sleep", {'duration': 0.0}, next2.id)
+        third = self.create_action("nop", {'message': 'next'}, None)
-        first = self.create_action("nop", {'message': 'hello'}, next.id)
+        second = self.create_action("sleep", {'duration': 0.0}, third.id)
+        first = self.create_action("nop", {'message': 'hello'}, second.id)
+
         self.check_action_state(first, objects.action.State.PENDING)
-        self.check_action_state(next, objects.action.State.PENDING)
+        self.check_action_state(second, objects.action.State.PENDING)
-        self.check_action_state(next2, objects.action.State.PENDING)
+        self.check_action_state(third, objects.action.State.PENDING)
 
         actions.append(first)
-        actions.append(next)
+        actions.append(second)
-        actions.append(next2)
+        actions.append(third)
 
-        result = self.engine.execute(actions)
+        try:
-        self.assertEqual(True, result)
+            self.engine.execute(actions)
-        self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+            self.check_actions_state(actions, objects.action.State.SUCCEEDED)
+
+        except Exception as exc:
+            self.fail(exc)
 
     def test_execute_with_exception(self):
         actions = []
-        next2 = self.create_action("no_exist", {'message': 'next'}, None)
+
-        next = self.create_action("sleep", {'duration': 0.0}, next2.id)
+        third = self.create_action("no_exist", {'message': 'next'}, None)
-        first = self.create_action("nop", {'message': 'hello'}, next.id)
+        second = self.create_action("sleep", {'duration': 0.0}, third.id)
+        first = self.create_action("nop", {'message': 'hello'}, second.id)
 
         self.check_action_state(first, objects.action.State.PENDING)
-        self.check_action_state(next, objects.action.State.PENDING)
+        self.check_action_state(second, objects.action.State.PENDING)
-        self.check_action_state(next2, objects.action.State.PENDING)
+        self.check_action_state(third, objects.action.State.PENDING)
-        actions.append(first)
+
-        actions.append(next)
+        actions.append(first)
-        actions.append(next2)
+        actions.append(second)
+        actions.append(third)
+
+        self.assertRaises(exception.WorkflowExecutionException,
+                          self.engine.execute, actions)
 
-        result = self.engine.execute(actions)
-        self.assertEqual(False, result)
         self.check_action_state(first, objects.action.State.SUCCEEDED)
-        self.check_action_state(next, objects.action.State.SUCCEEDED)
+        self.check_action_state(second, objects.action.State.SUCCEEDED)
-        self.check_action_state(next2, objects.action.State.FAILED)
+        self.check_action_state(third, objects.action.State.FAILED)
 
     @mock.patch("watcher.common.loader.default.DriverManager")
     def test_execute_with_action_exception(self, m_driver):
@@ -161,6 +187,7 @@ class TestDefaultWorkFlowEngine(base.DbTestCase):
                                           obj=None),
             namespace=FakeAction.namespace())
         actions = [self.create_action("dontcare", {}, None)]
-        result = self.engine.execute(actions)
+
-        self.assertEqual(False, result)
+        self.assertRaises(exception.WorkflowExecutionException,
+                          self.engine.execute, actions)
         self.check_action_state(actions[0], objects.action.State.FAILED)