From d7f4f4277282173a534b4e4ac99dd9b99e739f9a Mon Sep 17 00:00:00 2001 From: Tin Lam Date: Wed, 24 Feb 2016 23:23:30 -0600 Subject: [PATCH] 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 | 13 +-- watcher/applier/workflow_engine/default.py | 17 +-- watcher/common/exception.py | 3 + watcher/locale/watcher.pot | 43 +++---- .../test_default_workflow_engine.py | 105 +++++++++++------- 5 files changed, 105 insertions(+), 76 deletions(-) diff --git a/watcher/applier/action_plan/default.py b/watcher/applier/action_plan/default.py index 12f9f8d19..e369cb76b 100644 --- a/watcher/applier/action_plan/default.py +++ b/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) diff --git a/watcher/applier/workflow_engine/default.py b/watcher/applier/workflow_engine/default.py index 65f879d60..a91e2723d 100644 --- a/watcher/applier/workflow_engine/default.py +++ b/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) - return False + raise exception.WorkflowExecutionException(error=e) 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 - result = self.action.execute() - if result is not True: - self.engine.notify(self._db_action, - obj_action.State.FAILED) - else: - self.engine.notify(self._db_action, - obj_action.State.SUCCEEDED) + self.action.execute() + 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 ' diff --git a/watcher/common/exception.py b/watcher/common/exception.py index 7f65906ef..a0d325171 100644 --- a/watcher/common/exception.py +++ b/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') diff --git a/watcher/locale/watcher.pot b/watcher/locale/watcher.pot index b1b9553b9..cf3cfcfcd 100644 --- a/watcher/locale/watcher.pot +++ b/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 \n" "Language-Team: LANGUAGE \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 "" diff --git a/watcher/tests/applier/workflow_engine/test_default_workflow_engine.py b/watcher/tests/applier/workflow_engine/test_default_workflow_engine.py index 0b45effab..a571691bd 100644 --- a/watcher/tests/applier/workflow_engine/test_default_workflow_engine.py +++ b/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) - self.assertEqual(True, result) + try: + 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) - self.assertEqual(True, result) + try: + 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) - self.assertEqual(True, result) - self.check_actions_state(actions, objects.action.State.SUCCEEDED) + try: + self.engine.execute(actions) + 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) - first = self.create_action("nop", {'message': 'test'}, next.id) + second = self.create_action("sleep", {'duration': 0.0}, None) + first = self.create_action("nop", {'message': 'test'}, second.id) actions.append(first) - actions.append(next) + actions.append(second) - result = self.engine.execute(actions) - self.assertEqual(True, result) - self.check_actions_state(actions, objects.action.State.SUCCEEDED) + try: + self.engine.execute(actions) + 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) - first = self.create_action("nop", {'message': 'hello'}, next.id) + + third = self.create_action("nop", {'message': 'next'}, None) + 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(next2, objects.action.State.PENDING) + self.check_action_state(second, objects.action.State.PENDING) + self.check_action_state(third, objects.action.State.PENDING) actions.append(first) - actions.append(next) - actions.append(next2) + actions.append(second) + actions.append(third) - result = self.engine.execute(actions) - self.assertEqual(True, result) - self.check_actions_state(actions, objects.action.State.SUCCEEDED) + try: + self.engine.execute(actions) + 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) - first = self.create_action("nop", {'message': 'hello'}, next.id) + + third = self.create_action("no_exist", {'message': 'next'}, None) + 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(next2, objects.action.State.PENDING) - actions.append(first) - actions.append(next) - actions.append(next2) + self.check_action_state(second, objects.action.State.PENDING) + self.check_action_state(third, objects.action.State.PENDING) + + actions.append(first) + 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(next2, objects.action.State.FAILED) + self.check_action_state(second, objects.action.State.SUCCEEDED) + 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)