From 78b7a471c2167326cb611f498eb875a04a1eef72 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 9 Oct 2019 16:36:43 -0400 Subject: [PATCH] Make tags handling more robust Avoid loading the tags from the DB and then re-saving every time the stack is stored when the stack has tags. Avoid attempting to lazy-load the tags from the DB multiple times when there are no tags. Avoid lazy-loading the existing tags on an update when we intend to overwrite them anyway. Avoid writing the same set of tags multiple times. e.g. in a legacy update, previously we rewrote the current set of tags when changing the state to IN_PROGRESS, then wrote the new set of tags regardless of whether they had changed, then wrote the new set of tags again when the update completed. In a convergence update we also did three writes but in a different order, so that the new tags were written every time. With this change we write the new set of tags only once. This could also have prevented stacks with tags from being updated from legacy to convergence, because the (unchanged) tag list would get rewritten from inside a DB transaction. This is not expected so the stack_tags_set() API does not pass subtransactions=True when creating a transaction, which would cause a DB error. Change-Id: Ia52818cfc9479d5fa6e3b236988694f47998acda Task: 37001 --- heat/engine/api.py | 2 +- heat/engine/service.py | 8 ++-- heat/engine/stack.py | 40 +++++++++++-------- .../tests/engine/service/test_stack_update.py | 2 +- heat/tests/test_stack.py | 5 ++- heat/tests/test_stack_update.py | 2 +- 6 files changed, 33 insertions(+), 26 deletions(-) diff --git a/heat/engine/api.py b/heat/engine/api.py index cb064eab4e..4cc6a6d9b9 100644 --- a/heat/engine/api.py +++ b/heat/engine/api.py @@ -221,7 +221,7 @@ def format_stack(stack, preview=False, resolve_outputs=True): rpc_api.STACK_OWNER: stack.username, rpc_api.STACK_PARENT: stack.owner_id, rpc_api.STACK_USER_PROJECT_ID: stack.stack_user_project_id, - rpc_api.STACK_TAGS: stack.tags, + rpc_api.STACK_TAGS: stack.tags or None, } if not preview: diff --git a/heat/engine/service.py b/heat/engine/service.py index 62af50a958..36e725a8fd 100644 --- a/heat/engine/service.py +++ b/heat/engine/service.py @@ -896,7 +896,7 @@ class EngineService(service.ServiceBase): # stack definition. If PARAM_EXISTING is specified, we merge # any environment provided into the existing one and attempt # to use the existing stack template, if one is not provided. - if args.get(rpc_api.PARAM_EXISTING): + if args.get(rpc_api.PARAM_EXISTING, False): assert template_id is None, \ "Cannot specify template_id with PARAM_EXISTING" @@ -976,9 +976,9 @@ class EngineService(service.ServiceBase): common_params.setdefault(rpc_api.PARAM_CONVERGE, current_stack.converge) - if args.get(rpc_api.PARAM_EXISTING): - common_params.setdefault(rpc_api.STACK_TAGS, - current_stack.tags) + if args.get(rpc_api.PARAM_EXISTING, False): + if rpc_api.STACK_TAGS not in common_params: + common_params[rpc_api.STACK_TAGS] = current_stack.tags current_kwargs.update(common_params) updated_stack = parser.Stack(cnxt, stack_name, tmpl, **current_kwargs) diff --git a/heat/engine/stack.py b/heat/engine/stack.py index f0887bbd61..f91f8d09fb 100644 --- a/heat/engine/stack.py +++ b/heat/engine/stack.py @@ -173,6 +173,7 @@ class Stack(collections.Mapping): self._access_allowed_handlers = {} self._db_resources = None self._tags = tags + self._tags_stored = False self.adopt_stack_data = adopt_stack_data self.stack_user_project_id = stack_user_project_id self.created_time = created_time @@ -182,7 +183,6 @@ class Stack(collections.Mapping): self.nested_depth = nested_depth self.convergence = convergence self.current_traversal = current_traversal - self.tags = tags self.prev_raw_template_id = prev_raw_template_id self.current_deps = current_deps self._worker_client = None @@ -224,14 +224,17 @@ class Stack(collections.Mapping): @property def tags(self): if self._tags is None: - tags = stack_tag_object.StackTagList.get( - self.context, self.id) - if tags: - self._tags = [t.tag for t in tags] + if self.id is not None: + tags = stack_tag_object.StackTagList.get(self.context, self.id) + self._tags = [t.tag for t in tags] if tags else [] + else: + self._tags = [] + self._tags_stored = True return self._tags @tags.setter def tags(self, value): + self._tags_stored = (value == self._tags) self._tags = value @property @@ -721,13 +724,23 @@ class Stack(collections.Mapping): self.id = new_s.id self.created_time = new_s.created_at - if self.tags: - stack_tag_object.StackTagList.set(self.context, self.id, self.tags) + self._store_tags() self._set_param_stackid() return self.id + def _store_tags(self): + if (self._tags is not None and + not self._tags_stored and + self.id is not None): + tags = self._tags + if tags: + stack_tag_object.StackTagList.set(self.context, self.id, tags) + else: + stack_tag_object.StackTagList.delete(self.context, self.id) + self._tags_stored = True + def _backup_name(self): return '%s*' % self.name @@ -1362,11 +1375,6 @@ class Stack(collections.Mapping): self._set_param_stackid() self.tags = new_stack.tags - if new_stack.tags: - stack_tag_object.StackTagList.set(self.context, self.id, - new_stack.tags) - else: - stack_tag_object.StackTagList.delete(self.context, self.id) self.action = action self.status = self.IN_PROGRESS @@ -1661,11 +1669,9 @@ class Stack(collections.Mapping): self._set_param_stackid() self.tags = newstack.tags - if newstack.tags: - stack_tag_object.StackTagList.set(self.context, self.id, - newstack.tags) - else: - stack_tag_object.StackTagList.delete(self.context, self.id) + # Stack is already store()d in IN_PROGRESS state, so write tags now + # otherwise new set won't appear until COMPLETE/FAILED. + self._store_tags() check_message = functools.partial(self._check_for_message, msg_queue) diff --git a/heat/tests/engine/service/test_stack_update.py b/heat/tests/engine/service/test_stack_update.py index ff4e7943f4..f45adc35cf 100644 --- a/heat/tests/engine/service/test_stack_update.py +++ b/heat/tests/engine/service/test_stack_update.py @@ -396,7 +396,7 @@ resources: del api_args[rpc_api.STACK_TAGS] _, _, updated_stack = self.man._prepare_stack_updates( self.ctx, stk, t, {}, None, None, None, api_args, None) - self.assertIsNone(updated_stack.tags) + self.assertEqual([], updated_stack.tags) def test_stack_update_existing_registry(self): # Use a template with existing flag and ensure the diff --git a/heat/tests/test_stack.py b/heat/tests/test_stack.py index 47d407df74..cca559dc61 100644 --- a/heat/tests/test_stack.py +++ b/heat/tests/test_stack.py @@ -1407,7 +1407,8 @@ class StackTest(common.HeatTestCase): self.stack.store() stack_id = self.stack.id test_stack = stack.Stack.load(self.ctx, stack_id=stack_id) - self.assertIsNone(test_stack.tags) + self.assertIsNone(test_stack._tags) + self.assertEqual([], test_stack.tags) self.stack = stack.Stack(self.ctx, 'stack_name', self.tmpl) self.stack.tags = ['tag1', 'tag2'] @@ -1424,7 +1425,7 @@ class StackTest(common.HeatTestCase): self.stack.store() stack_id = self.stack.id test_stack = stack.Stack.load(self.ctx, stack_id=stack_id) - self.assertIsNone(test_stack.tags) + self.assertEqual([], test_stack.tags) self.stack = stack.Stack(self.ctx, 'stack_name', self.tmpl, tags=['tag1', 'tag2']) diff --git a/heat/tests/test_stack_update.py b/heat/tests/test_stack_update.py index 7a96a3f95d..2fe3243c15 100644 --- a/heat/tests/test_stack_update.py +++ b/heat/tests/test_stack_update.py @@ -234,7 +234,7 @@ class StackUpdateTest(common.HeatTestCase): self.stack.update(updated_stack) self.assertEqual((stack.Stack.UPDATE, stack.Stack.COMPLETE), self.stack.state) - self.assertIsNone(self.stack.tags) + self.assertEqual([], self.stack.tags) def test_update_modify_ok_replace(self): tmpl = {'HeatTemplateFormatVersion': '2012-12-12',