Merge "Eliminate client race condition in convergence delete" into stable/pike

6 files changed, 37 insertions, 14 deletions

diff --git a/heat/engine/service.py b/heat/engine/service.py
index 52d42ea59..615c52897 100644
--- a/heat/engine/service.py
+++ b/heat/engine/service.py
@@ -1418,14 +1418,19 @@ class EngineService(service.ServiceBase):
         self.resource_enforcer.enforce_stack(stack)
 
         if stack.convergence and cfg.CONF.convergence_engine:
-            def convergence_delete():
-                stack.thread_group_mgr = self.thread_group_mgr
+            stack.thread_group_mgr = self.thread_group_mgr
+            template = templatem.Template.create_empty_template(
+                from_template=stack.t)
+
+            # stop existing traversal; mark stack as FAILED
+            if stack.status == stack.IN_PROGRESS:
+                self.worker_service.stop_traversal(stack)
+
+            def stop_workers():
                 self.worker_service.stop_all_workers(stack)
-                template = templatem.Template.create_empty_template(
-                    from_template=stack.t)
-                stack.converge_stack(template=template, action=stack.DELETE)
 
-            self.thread_group_mgr.start(stack.id, convergence_delete)
+            stack.converge_stack(template=template, action=stack.DELETE,
+                                 pre_converge=stop_workers)
             return
 
         lock = stack_lock.StackLock(cnxt, stack.id, self.engine_id)
diff --git a/heat/engine/stack.py b/heat/engine/stack.py
index b82b04fb1..8a029564c 100644
--- a/heat/engine/stack.py
+++ b/heat/engine/stack.py
@@ -1253,7 +1253,8 @@ class Stack(collections.Mapping):
         updater()
 
     @profiler.trace('Stack.converge_stack', hide_args=False)
-    def converge_stack(self, template, action=UPDATE, new_stack=None):
+    def converge_stack(self, template, action=UPDATE, new_stack=None,
+                       pre_converge=None):
         """Update the stack template and trigger convergence for resources."""
         if action not in [self.CREATE, self.ADOPT]:
             # no back-up template for create action
@@ -1307,9 +1308,10 @@ class Stack(collections.Mapping):
 
         # TODO(later): lifecycle_plugin_utils.do_pre_ops
 
-        self.thread_group_mgr.start(self.id, self._converge_create_or_update)
+        self.thread_group_mgr.start(self.id, self._converge_create_or_update,
+                                    pre_converge=pre_converge)
 
-    def _converge_create_or_update(self):
+    def _converge_create_or_update(self, pre_converge=None):
         current_resources = self._update_or_store_resources()
         self._compute_convg_dependencies(self.ext_rsrcs_db, self.dependencies,
                                          current_resources)
@@ -1326,6 +1328,8 @@ class Stack(collections.Mapping):
                          'action': self.action})
             return
 
+        if callable(pre_converge):
+            pre_converge()
         if self.action == self.DELETE:
             try:
                 self.delete_all_snapshots()
diff --git a/heat/engine/worker.py b/heat/engine/worker.py
index d625d1b08..84dc404b2 100644
--- a/heat/engine/worker.py
+++ b/heat/engine/worker.py
@@ -125,11 +125,11 @@ class WorkerService(object):
                 _stop_traversal(child)
 
     def stop_all_workers(self, stack):
-        # stop the traversal
-        if stack.status == stack.IN_PROGRESS:
-            self.stop_traversal(stack)
+        """Cancel all existing worker threads for the stack.
 
-        # cancel existing workers
+        Threads will stop running at their next yield point, whether or not the
+        resource operations are complete.
+        """
         cancelled = _cancel_workers(stack, self.thread_group_mgr,
                                     self.engine_id, self._rpc_client)
         if not cancelled:
diff --git a/heat/tests/convergence/framework/worker_wrapper.py b/heat/tests/convergence/framework/worker_wrapper.py
index 042bc9e07..7ca39ada1 100644
--- a/heat/tests/convergence/framework/worker_wrapper.py
+++ b/heat/tests/convergence/framework/worker_wrapper.py
@@ -37,5 +37,8 @@ class Worker(message_processor.MessageProcessor):
                                  adopt_stack_data,
                                  converge)
 
+    def stop_traversal(self, current_stack):
+        pass
+
     def stop_all_workers(self, current_stack):
         pass
diff --git a/heat/tests/engine/test_engine_worker.py b/heat/tests/engine/test_engine_worker.py
index eeae9e326..affb51186 100644
--- a/heat/tests/engine/test_engine_worker.py
+++ b/heat/tests/engine/test_engine_worker.py
@@ -209,7 +209,7 @@ class WorkerServiceTest(common.HeatTestCase):
         stack.id = 'stack_id'
         stack.rollback = mock.MagicMock()
         _worker.stop_all_workers(stack)
-        mock_st.assert_called_once_with(stack)
+        mock_st.assert_not_called()
         mock_cw.assert_called_once_with(stack, mock_tgm, 'engine-001',
                                         _worker._rpc_client)
         self.assertFalse(stack.rollback.called)
diff --git a/releasenotes/notes/convergence-delete-race-5b821bbd4c5ba5dc.yaml b/releasenotes/notes/convergence-delete-race-5b821bbd4c5ba5dc.yaml
new file mode 100644
index 000000000..f06a90619
--- /dev/null
+++ b/releasenotes/notes/convergence-delete-race-5b821bbd4c5ba5dc.yaml
@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Previously, when deleting a convergence stack, the API call would return
+    immediately, so that it was possible for a client immediately querying the
+    status of the stack to see the state of the previous operation in progress
+    or having failed, and confuse that with a current status. (This included
+    Heat itself when acting as a client for a nested stack.) Convergence stacks
+    are now guaranteed to have moved to the ``DELETE_IN_PROGRESS`` state before
+    the delete API call returns, so any subsequent polling will reflect
+    up-to-date information.