From 8edccc98b1daf00d4fcd425b40d46beee63ffe1d Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 21 Jul 2017 21:32:46 -0400 Subject: [PATCH] Calculate the requires list in check_resource Obtain the list of required resources in one place in check_resource, instead of in both Resource.create_convergence() and Resource.update_convergence(). Since these require knowledge about how the dependencies are encoded in the RPC message, they never really belonged in the Resource class anyway. Change-Id: I030c6287acddcd91dfe5fecba72c276fec52829b --- heat/engine/check_resource.py | 10 ++-- heat/engine/resource.py | 11 ++-- heat/tests/engine/test_check_resource.py | 26 +++++++--- heat/tests/test_resource.py | 65 +++++++----------------- 4 files changed, 46 insertions(+), 66 deletions(-) diff --git a/heat/engine/check_resource.py b/heat/engine/check_resource.py index 74f615d897..b9a2c35b98 100644 --- a/heat/engine/check_resource.py +++ b/heat/engine/check_resource.py @@ -137,8 +137,10 @@ class CheckResource(object): is_update, rsrc, stack, adopt_stack_data): try: if is_update: + requires = set(d.primary_key for d in resource_data.values() + if d is not None) try: - check_resource_update(rsrc, tmpl.id, resource_data, + check_resource_update(rsrc, tmpl.id, requires, self.engine_id, stack, self.msg_queue) except resource.UpdateReplace: @@ -387,15 +389,15 @@ def _check_for_message(msg_queue): LOG.error('Unknown message "%s" received', message) -def check_resource_update(rsrc, template_id, resource_data, engine_id, +def check_resource_update(rsrc, template_id, requires, engine_id, stack, msg_queue): """Create or update the Resource if appropriate.""" check_message = functools.partial(_check_for_message, msg_queue) if rsrc.action == resource.Resource.INIT: - rsrc.create_convergence(template_id, resource_data, engine_id, + rsrc.create_convergence(template_id, requires, engine_id, stack.time_remaining(), check_message) else: - rsrc.update_convergence(template_id, resource_data, engine_id, + rsrc.update_convergence(template_id, requires, engine_id, stack.time_remaining(), stack, check_message) diff --git a/heat/engine/resource.py b/heat/engine/resource.py index cc59435f5a..b4c2950320 100644 --- a/heat/engine/resource.py +++ b/heat/engine/resource.py @@ -1154,14 +1154,11 @@ class Resource(status.ResourceStatus): """ return self - def create_convergence(self, template_id, resource_data, engine_id, + def create_convergence(self, template_id, requires, engine_id, timeout, progress_callback=None): """Creates the resource by invoking the scheduler TaskRunner.""" self._calling_engine_id = engine_id - self.requires = list( - set(data.primary_key for data in resource_data.values() - if data is not None) - ) + self.requires = list(requires) self.current_template_id = template_id if self.stack.adopt_stack_data is None: runner = scheduler.TaskRunner(self.create) @@ -1412,7 +1409,7 @@ class Resource(status.ResourceStatus): else: raise UpdateReplace(self.name) - def update_convergence(self, template_id, resource_data, engine_id, + def update_convergence(self, template_id, new_requires, engine_id, timeout, new_stack, progress_callback=None): """Update the resource synchronously. @@ -1422,8 +1419,6 @@ class Resource(status.ResourceStatus): resource by invoking the scheduler TaskRunner. """ self._calling_engine_id = engine_id - new_requires = set(data.primary_key for data in resource_data.values() - if data is not None) # Check that the resource type matches. If the type has changed by a # legitimate substitution, the load()ed resource will already be of diff --git a/heat/tests/engine/test_check_resource.py b/heat/tests/engine/test_check_resource.py index e0eacfda37..93691297f0 100644 --- a/heat/tests/engine/test_check_resource.py +++ b/heat/tests/engine/test_check_resource.py @@ -90,7 +90,7 @@ class CheckWorkflowUpdateTest(common.HeatTestCase): self.is_update, None) mock_cru.assert_called_once_with(self.resource, self.resource.stack.t.id, - {}, self.worker.engine_id, + set(), self.worker.engine_id, mock.ANY, mock.ANY) self.assertFalse(mock_crc.called) @@ -121,7 +121,7 @@ class CheckWorkflowUpdateTest(common.HeatTestCase): self.is_update, None) mock_cru.assert_called_once_with(self.resource, self.resource.stack.t.id, - {}, self.worker.engine_id, + set(), self.worker.engine_id, mock.ANY, mock.ANY) self.assertTrue(mock_mr.called) self.assertFalse(mock_crc.called) @@ -143,7 +143,7 @@ class CheckWorkflowUpdateTest(common.HeatTestCase): self.is_update, None) mock_cru.assert_called_once_with(self.resource, self.resource.stack.t.id, - {}, self.worker.engine_id, + set(), self.worker.engine_id, mock.ANY, mock.ANY) mock_ss.assert_called_once_with(self.resource.action, resource.Resource.FAILED, @@ -523,6 +523,20 @@ class CheckWorkflowUpdateTest(common.HeatTestCase): self.assertFalse(mock_pcr.called) self.assertFalse(mock_csc.called) + @mock.patch.object(resource.Resource, 'load') + def test_requires(self, mock_load, mock_cru, mock_crc, mock_pcr, mock_csc): + mock_load.return_value = self.resource, self.stack, self.stack + res_data = {(1, True): {u'id': 5, u'name': 'A', 'attrs': {}}, + (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} + self.worker.check_resource(self.ctx, self.resource.id, + self.stack.current_traversal, + sync_point.serialize_input_data(res_data), + self.is_update, {}) + mock_cru.assert_called_once_with( + self.resource, self.resource.stack.t.id, + {5, 3}, self.worker.engine_id, + self.stack, mock.ANY) + @mock.patch.object(check_resource, 'check_stack_complete') @mock.patch.object(check_resource, 'propagate_check_resource') @@ -677,7 +691,7 @@ class MiscMethodsTest(common.HeatTestCase): def test_check_resource_update_init_action(self, mock_update, mock_create): self.resource.action = 'INIT' check_resource.check_resource_update( - self.resource, self.resource.stack.t.id, {}, 'engine-id', + self.resource, self.resource.stack.t.id, set(), 'engine-id', self.stack, None) self.assertTrue(mock_create.called) self.assertFalse(mock_update.called) @@ -688,7 +702,7 @@ class MiscMethodsTest(common.HeatTestCase): self, mock_update, mock_create): self.resource.action = 'CREATE' check_resource.check_resource_update( - self.resource, self.resource.stack.t.id, {}, 'engine-id', + self.resource, self.resource.stack.t.id, set(), 'engine-id', self.stack, None) self.assertFalse(mock_create.called) self.assertTrue(mock_update.called) @@ -699,7 +713,7 @@ class MiscMethodsTest(common.HeatTestCase): self, mock_update, mock_create): self.resource.action = 'UPDATE' check_resource.check_resource_update( - self.resource, self.resource.stack.t.id, {}, 'engine-id', + self.resource, self.resource.stack.t.id, set(), 'engine-id', self.stack, None) self.assertFalse(mock_create.called) self.assertTrue(mock_update.called) diff --git a/heat/tests/test_resource.py b/heat/tests/test_resource.py index 72a423952f..4d71b1b05e 100644 --- a/heat/tests/test_resource.py +++ b/heat/tests/test_resource.py @@ -2089,13 +2089,10 @@ class ResourceTest(common.HeatTestCase): res.action = res.CREATE res.store() self._assert_resource_lock(res.id, None, None) - res_data = {(1, True): {u'id': 1, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) pcb = mock.Mock() with mock.patch.object(resource.Resource, 'create') as mock_create: - res.create_convergence(self.stack.t.id, res_data, 'engine-007', + res.create_convergence(self.stack.t.id, {1, 3}, 'engine-007', -1, pcb) self.assertTrue(mock_create.called) @@ -2107,13 +2104,10 @@ class ResourceTest(common.HeatTestCase): res = generic_rsrc.GenericResource('test_res', tmpl, self.stack) res.action = res.CREATE res.store() - res_data = {(1, True): {u'id': 1, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) pcb = mock.Mock() self.assertRaises(scheduler.Timeout, res.create_convergence, - self.stack.t.id, res_data, 'engine-007', -1, pcb) + self.stack.t.id, {1, 3}, 'engine-007', -1, pcb) def test_create_convergence_sets_requires_for_failure(self): """Ensure that requires are computed correctly. @@ -2127,11 +2121,8 @@ class ResourceTest(common.HeatTestCase): dummy_ex = exception.ResourceNotAvailable(resource_name=res.name) res.create = mock.Mock(side_effect=dummy_ex) self._assert_resource_lock(res.id, None, None) - res_data = {(1, True): {u'id': 5, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) self.assertRaises(exception.ResourceNotAvailable, - res.create_convergence, self.stack.t.id, res_data, + res.create_convergence, self.stack.t.id, {5, 3}, 'engine-007', self.dummy_timeout, self.dummy_event) self.assertItemsEqual([5, 3], res.requires) # The locking happens in create which we mocked out @@ -2146,11 +2137,8 @@ class ResourceTest(common.HeatTestCase): self.stack.adopt_stack_data = {'resources': {'test_res': { 'resource_id': 'fluffy'}}} self._assert_resource_lock(res.id, None, None) - res_data = {(1, True): {u'id': 5, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) tr = scheduler.TaskRunner(res.create_convergence, self.stack.t.id, - res_data, 'engine-007', self.dummy_timeout, + {5, 3}, 'engine-007', self.dummy_timeout, self.dummy_event) tr() mock_adopt.assert_called_once_with( @@ -2165,11 +2153,8 @@ class ResourceTest(common.HeatTestCase): res.store() self.stack.adopt_stack_data = {'resources': {}} self._assert_resource_lock(res.id, None, None) - res_data = {(1, True): {u'id': 5, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) tr = scheduler.TaskRunner(res.create_convergence, self.stack.t.id, - res_data, 'engine-007', self.dummy_timeout, + {5, 3}, 'engine-007', self.dummy_timeout, self.dummy_event) exc = self.assertRaises(exception.ResourceFailure, tr) self.assertIn('Resource ID was not provided', six.text_type(exc)) @@ -2203,11 +2188,8 @@ class ResourceTest(common.HeatTestCase): new_temp, stack_id=self.stack.id) res.stack.convergence = True - res_data = {(1, True): {u'id': 4, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) tr = scheduler.TaskRunner(res.update_convergence, new_temp.id, - res_data, 'engine-007', 120, new_stack) + {4, 3}, 'engine-007', 120, new_stack) tr() self.assertItemsEqual([3, 4], res.requires) @@ -2238,9 +2220,8 @@ class ResourceTest(common.HeatTestCase): new_stack = parser.Stack(utils.dummy_context(), 'test_stack', new_temp, stack_id=self.stack.id) - res_data = {} tr = scheduler.TaskRunner(res.update_convergence, new_temp.id, - res_data, 'engine-007', -1, new_stack, + set(), 'engine-007', -1, new_stack, self.dummy_event) self.assertRaises(scheduler.Timeout, tr) @@ -2260,9 +2241,8 @@ class ResourceTest(common.HeatTestCase): new_stack = parser.Stack(utils.dummy_context(), 'test_stack', new_temp, stack_id=self.stack.id) - res_data = {} self.assertRaises(resource.UpdateReplace, res.update_convergence, - new_temp.id, res_data, 'engine-007', + new_temp.id, set(), 'engine-007', -1, new_stack) def test_update_convergence_checks_resource_class(self): @@ -2282,9 +2262,8 @@ class ResourceTest(common.HeatTestCase): new_stack = parser.Stack(ctx, 'test_stack', new_temp, stack_id=self.stack.id) - res_data = {} tr = scheduler.TaskRunner(res.update_convergence, new_temp.id, - res_data, 'engine-007', -1, new_stack, + set(), 'engine-007', -1, new_stack, self.dummy_event) self.assertRaises(resource.UpdateReplace, tr) @@ -2301,9 +2280,6 @@ class ResourceTest(common.HeatTestCase): res.stack.convergence = True - res_data = {(1, True): {u'id': 4, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) tmpl = template.Template({ 'HeatTemplateFormatVersion': '2012-12-12', 'Resources': { @@ -2312,7 +2288,7 @@ class ResourceTest(common.HeatTestCase): new_stack = parser.Stack(utils.dummy_context(), 'test_stack', tmpl, stack_id=self.stack.id) tr = scheduler.TaskRunner(res.update_convergence, 'template_key', - res_data, 'engine-007', self.dummy_timeout, + {4, 3}, 'engine-007', self.dummy_timeout, new_stack) ex = self.assertRaises(exception.UpdateInProgress, tr) msg = ("The resource %s is already being updated." % @@ -2349,9 +2325,6 @@ class ResourceTest(common.HeatTestCase): }}, env=self.env) new_temp.store(stack.context) - res_data = {(1, True): {u'id': 4, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) new_stack = parser.Stack(utils.dummy_context(), 'test_stack', new_temp, stack_id=self.stack.id) @@ -2359,7 +2332,7 @@ class ResourceTest(common.HeatTestCase): res._calling_engine_id = 'engine-9' tr = scheduler.TaskRunner(res.update_convergence, new_temp.id, - res_data, 'engine-007', 120, new_stack, + {4, 3}, 'engine-007', 120, new_stack, self.dummy_event) self.assertRaises(exception.ResourceFailure, tr) @@ -2395,13 +2368,10 @@ class ResourceTest(common.HeatTestCase): res.stack.convergence = True - res_data = {(1, True): {u'id': 4, u'name': 'A', 'attrs': {}}, - (2, True): {u'id': 3, u'name': 'B', 'attrs': {}}} - res_data = node_data.load_resources_data(res_data) new_stack = parser.Stack(utils.dummy_context(), 'test_stack', new_temp, stack_id=self.stack.id) tr = scheduler.TaskRunner(res.update_convergence, new_temp.id, - res_data, 'engine-007', 120, new_stack, + {4, 3}, 'engine-007', 120, new_stack, self.dummy_event) self.assertRaises(resource.UpdateReplace, tr) @@ -2429,7 +2399,7 @@ class ResourceTest(common.HeatTestCase): self.stack.state_set(self.stack.ROLLBACK, self.stack.IN_PROGRESS, 'Simulate rollback') res.restore_prev_rsrc = mock.Mock() - tr = scheduler.TaskRunner(res.update_convergence, 'new_tmpl_id', {}, + tr = scheduler.TaskRunner(res.update_convergence, 'new_tmpl_id', set(), 'engine-007', self.dummy_timeout, new_stack, self.dummy_event) self.assertRaises(resource.UpdateReplace, tr) @@ -2453,7 +2423,7 @@ class ResourceTest(common.HeatTestCase): self.stack.state_set(self.stack.ROLLBACK, self.stack.IN_PROGRESS, 'Simulate rollback') res.restore_prev_rsrc = mock.Mock(side_effect=Exception) - tr = scheduler.TaskRunner(res.update_convergence, 'new_tmpl_id', {}, + tr = scheduler.TaskRunner(res.update_convergence, 'new_tmpl_id', set(), 'engine-007', self.dummy_timeout, new_stack, self.dummy_event) self.assertRaises(exception.ResourceFailure, tr) @@ -4330,11 +4300,10 @@ class ResourceUpdateRestrictionTest(common.HeatTestCase): self.stack = parser.Stack(utils.dummy_context(), 'test_stack', template.Template(self.tmpl, env=self.env), stack_id=str(uuid.uuid4())) - res_data = {} res = self.stack['bar'] pcb = mock.Mock() self.patchobject(res, 'lock') - res.create_convergence(self.stack.t.id, res_data, 'engine-007', + res.create_convergence(self.stack.t.id, set(), 'engine-007', self.dummy_timeout, pcb) return res @@ -4456,7 +4425,7 @@ class ResourceUpdateRestrictionTest(common.HeatTestCase): error = self.assertRaises(exception.ResourceFailure, scheduler.TaskRunner(res.update_convergence, self.stack.t.id, - {}, + set(), 'engine-007', self.dummy_timeout, self.new_stack, @@ -4488,7 +4457,7 @@ class ResourceUpdateRestrictionTest(common.HeatTestCase): error = self.assertRaises(resource.UpdateReplace, scheduler.TaskRunner(res.update_convergence, self.stack.t.id, - {}, + set(), 'engine-007', self.dummy_timeout, self.new_stack,