From 4349c9103158d667de02756bbd6dda13b384b49b Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Mon, 24 Feb 2020 08:41:32 +0530 Subject: [PATCH] Handle OS::Mistral::Workflow resource replacement properly OS::Mistral::Workflow resource creates a mistral workflow with a unique name (resource_id). We replace FAILED resources by default and replace wont work in this case as it will try to use the same workflow name for the replacement resouce, if the 'name' property is provided. If the workflow does not exist/deleted using mistral api directly, it would create a replacement resource, but it would delete the workflow when cleaning up the old resource. So we would endup with a replacement resource without any backing workflow. This adds a new resource attribute ``always_replace_on_check_failed`` and overrides needs_replace_failed() for OS::Mistral::Workflow. Note: This also removes the mocking of mistra_base, that was removed from stable/rocky with 714d9eea4c871cfc979664cc8fc846cc02fba798. Task: 38855 Change-Id: Ia0812b88cae363dfa25ccd907ecbe8b86f5b1a23 (cherry picked from commit 9e80518b900a1f14a80ee05ddeef4b250433febd) --- heat/engine/resource.py | 7 +- .../resources/openstack/mistral/workflow.py | 15 ++++ heat/tests/openstack/mistral/test_workflow.py | 88 ++++++++++++++----- 3 files changed, 87 insertions(+), 23 deletions(-) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index 2e59066843..4275887a58 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -158,6 +158,9 @@ class Resource(status.ResourceStatus): # a signal to this resource signal_needs_metadata_updates = True + # Whether the resource is always replaced when CHECK_FAILED + always_replace_on_check_failed = True + def __new__(cls, name, definition, stack): """Create a new Resource of the appropriate class for its type.""" @@ -1392,7 +1395,9 @@ class Resource(status.ResourceStatus): prev_resource, check_init_complete=True): if self.status == self.FAILED: # always replace when a resource is in CHECK_FAILED - if self.action == self.CHECK or self.needs_replace_failed(): + if ((self.action == self.CHECK and + self.always_replace_on_check_failed) or ( + self.needs_replace_failed())): raise UpdateReplace(self) if self.state == (self.DELETE, self.COMPLETE): diff --git a/heat/engine/resources/openstack/mistral/workflow.py b/heat/engine/resources/openstack/mistral/workflow.py index 3c9ea23bc5..8794b23d72 100644 --- a/heat/engine/resources/openstack/mistral/workflow.py +++ b/heat/engine/resources/openstack/mistral/workflow.py @@ -46,6 +46,8 @@ class Workflow(signal_responder.SignalResponder, entity = 'workflows' + always_replace_on_check_failed = False + PROPERTIES = ( NAME, TYPE, DESCRIPTION, INPUT, OUTPUT, TASKS, PARAMS, TASK_DEFAULTS, USE_REQUEST_BODY_AS_INPUT, TAGS @@ -596,6 +598,19 @@ class Workflow(signal_responder.SignalResponder, executions.extend(self.data().get(self.EXECUTIONS).split(',')) self.data_set(self.EXECUTIONS, ','.join(executions)) + def needs_replace_failed(self): + if self.resource_id is None: + return True + + if self.properties[self.NAME] is None: + return True + + with self.client_plugin().ignore_not_found: + self.client().workflows.get(self.resource_id) + return False + self.resource_id_set(None) + return True + def handle_update(self, json_snippet, tmpl_diff, prop_diff): if prop_diff: props = json_snippet.properties(self.properties_schema, diff --git a/heat/tests/openstack/mistral/test_workflow.py b/heat/tests/openstack/mistral/test_workflow.py index 04336cccac..1b06c97ca7 100644 --- a/heat/tests/openstack/mistral/test_workflow.py +++ b/heat/tests/openstack/mistral/test_workflow.py @@ -15,6 +15,7 @@ import mock import six import yaml +from mistralclient.api import base as mistral_base from mistralclient.api.v2 import executions from oslo_serialization import jsonutils @@ -302,6 +303,21 @@ resources: result: <% $.hello %> """ +workflow_template_update_replace_failed = """ +heat_template_version: 2013-05-23 +resources: + workflow: + type: OS::Mistral::Workflow + properties: + name: hello_action + type: direct + tasks: + - name: hello + action: std.echo output='Good Morning!' + publish: + result: <% $.hello %> +""" + workflow_template_update = """ heat_template_version: 2013-05-23 resources: @@ -373,12 +389,6 @@ class TestMistralWorkflow(common.HeatTestCase): def setUp(self): super(TestMistralWorkflow, self).setUp() self.ctx = utils.dummy_context() - tmpl = template_format.parse(workflow_template) - self.stack = utils.parse_stack(tmpl, stack_name='test_stack') - - resource_defns = self.stack.t.resource_definitions(self.stack) - self.rsrc_defn = resource_defns['workflow'] - self.mistral = mock.Mock() self.patchobject(workflow.Workflow, 'client', return_value=self.mistral) @@ -388,8 +398,6 @@ class TestMistralWorkflow(common.HeatTestCase): '_create_user')) self.patches.append(mock.patch.object(signal_responder.SignalResponder, '_create_keypair')) - self.patches.append(mock.patch.object(client, - 'mistral_base')) self.patches.append(mock.patch.object(client.MistralClientPlugin, '_create')) for patch in self.patches: @@ -402,15 +410,20 @@ class TestMistralWorkflow(common.HeatTestCase): for patch in self.patches: patch.stop() - def _create_resource(self, name, snippet, stack): - wf = workflow.Workflow(name, snippet, stack) + def _create_resource(self, name, template=workflow_template): + tmpl = template_format.parse(template) + self.stack = utils.parse_stack(tmpl, stack_name='test_stack') + + resource_defns = self.stack.t.resource_definitions(self.stack) + rsrc_defn = resource_defns['workflow'] + wf = workflow.Workflow(name, rsrc_defn, self.stack) self.mistral.workflows.create.return_value = [ FakeWorkflow('test_stack-workflow-b5fiekfci3yc')] scheduler.TaskRunner(wf.create)() return wf def test_create(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') expected_state = (wf.CREATE, wf.COMPLETE) self.assertEqual(expected_state, wf.state) self.assertEqual('test_stack-workflow-b5fiekfci3yc', wf.resource_id) @@ -462,7 +475,7 @@ class TestMistralWorkflow(common.HeatTestCase): break def test_attributes(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') self.mistral.workflows.get.return_value = ( FakeWorkflow('test_stack-workflow-b5fiekfci3yc')) self.assertEqual({'name': 'test_stack-workflow-b5fiekfci3yc', @@ -523,7 +536,7 @@ class TestMistralWorkflow(common.HeatTestCase): six.text_type(exc)) def test_update_replace(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') t = template_format.parse(workflow_template_update_replace) rsrc_defns = template.Template(t).resource_definitions(self.stack) @@ -540,8 +553,7 @@ class TestMistralWorkflow(common.HeatTestCase): self.assertEqual(msg, six.text_type(err)) def test_update(self): - wf = self._create_resource('workflow', self.rsrc_defn, - self.stack) + wf = self._create_resource('workflow') t = template_format.parse(workflow_template_update) rsrc_defns = template.Template(t).resource_definitions(self.stack) new_wf = rsrc_defns['workflow'] @@ -552,8 +564,7 @@ class TestMistralWorkflow(common.HeatTestCase): self.assertEqual((wf.UPDATE, wf.COMPLETE), wf.state) def test_update_input(self): - wf = self._create_resource('workflow', self.rsrc_defn, - self.stack) + wf = self._create_resource('workflow') t = template_format.parse(workflow_template) t['resources']['workflow']['properties']['input'] = {'foo': 'bar'} rsrc_defns = template.Template(t).resource_definitions(self.stack) @@ -565,8 +576,7 @@ class TestMistralWorkflow(common.HeatTestCase): self.assertEqual((wf.UPDATE, wf.COMPLETE), wf.state) def test_update_failed(self): - wf = self._create_resource('workflow', self.rsrc_defn, - self.stack) + wf = self._create_resource('workflow') t = template_format.parse(workflow_template_update) rsrc_defns = template.Template(t).resource_definitions(self.stack) new_wf = rsrc_defns['workflow'] @@ -575,8 +585,42 @@ class TestMistralWorkflow(common.HeatTestCase): scheduler.TaskRunner(wf.update, new_wf)) self.assertEqual((wf.UPDATE, wf.FAILED), wf.state) + def test_update_failed_no_replace(self): + wf = self._create_resource('workflow', + workflow_template_update_replace) + t = template_format.parse(workflow_template_update_replace_failed) + rsrc_defns = template.Template(t).resource_definitions(self.stack) + new_wf = rsrc_defns['workflow'] + self.mistral.workflows.get.return_value = ( + FakeWorkflow('test_stack-workflow-b5fiekfci3yc')) + self.mistral.workflows.update.side_effect = [ + Exception('boom!'), + [FakeWorkflow('test_stack-workflow-b5fiekfci3yc')]] + self.assertRaises(exception.ResourceFailure, + scheduler.TaskRunner(wf.update, new_wf)) + self.assertEqual((wf.UPDATE, wf.FAILED), wf.state) + scheduler.TaskRunner(wf.update, new_wf)() + self.assertTrue(self.mistral.workflows.update.called) + self.assertEqual((wf.UPDATE, wf.COMPLETE), wf.state) + + def test_update_failed_replace_not_found(self): + wf = self._create_resource('workflow', + workflow_template_update_replace) + t = template_format.parse(workflow_template_update_replace_failed) + rsrc_defns = template.Template(t).resource_definitions(self.stack) + new_wf = rsrc_defns['workflow'] + self.mistral.workflows.update.side_effect = Exception('boom!') + self.assertRaises(exception.ResourceFailure, + scheduler.TaskRunner(wf.update, new_wf)) + self.assertEqual((wf.UPDATE, wf.FAILED), wf.state) + self.mistral.workflows.get.side_effect = [ + mistral_base.APIException(error_code=404)] + self.assertRaises(resource.UpdateReplace, + scheduler.TaskRunner(wf.update, + new_wf)) + def test_delete_super_call_successful(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') scheduler.TaskRunner(wf.delete)() self.assertEqual((wf.DELETE, wf.COMPLETE), wf.state) @@ -584,7 +628,7 @@ class TestMistralWorkflow(common.HeatTestCase): self.assertEqual(1, self.mistral.workflows.delete.call_count) def test_delete_executions_successful(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') self.mistral.executuions.delete.return_value = None wf._data = {'executions': '1234,5678'} @@ -596,7 +640,7 @@ class TestMistralWorkflow(common.HeatTestCase): data_delete.assert_called_once_with('executions') def test_delete_executions_not_found(self): - wf = self._create_resource('workflow', self.rsrc_defn, self.stack) + wf = self._create_resource('workflow') self.mistral.executuions.delete.side_effect = [ self.mistral.mistral_base.APIException(error_code=404),