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

@@ -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:

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):
+        if args.get(rpc_api.PARAM_EXISTING, False):
-            common_params.setdefault(rpc_api.STACK_TAGS,
+            if rpc_api.STACK_TAGS not in common_params:
-                                     current_stack.tags)
+                common_params[rpc_api.STACK_TAGS] = current_stack.tags
         current_kwargs.update(common_params)
         updated_stack = parser.Stack(cnxt, stack_name, tmpl,
                                      **current_kwargs)

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(
+            if self.id is not None:
-                self.context, self.id)
+                tags = stack_tag_object.StackTagList.get(self.context, self.id)
-            if tags:
+                self._tags = [t.tag for t in tags] if tags else []
-                self._tags = [t.tag for t in tags]
+            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:
+        self._store_tags()
-            stack_tag_object.StackTagList.set(self.context, self.id, self.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 is already store()d in IN_PROGRESS state, so write tags now
-                stack_tag_object.StackTagList.set(self.context, self.id,
+            # otherwise new set won't appear until COMPLETE/FAILED.
-                                                  newstack.tags)
+            self._store_tags()
-            else:
-                stack_tag_object.StackTagList.delete(self.context, self.id)
 
             check_message = functools.partial(self._check_for_message,
                                               msg_queue)

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

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'])

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',