Only allow for deleting completed executions

A force parameter was added in case deleting a running execution is
needed but that will spew all kinds of errors afterwards
The default is force = False

Change-Id: I19d822800a1ee056682daeaa7ddf0f400392525d
Closes-bug: 1598135
Depends-On: Id935265fcbe0a3072ba2d693edf54b892173fbdf

mistral/api/controllers/v2/execution.py

@@ -268,16 +268,29 @@ class ExecutionsController(rest.RestController):
         return resources.Execution.from_dict(result)
 
     @rest_utils.wrap_wsme_controller_exception
-    @wsme_pecan.wsexpose(None, wtypes.text, status_code=204)
-    def delete(self, id):
+    @wsme_pecan.wsexpose(None, wtypes.text, bool, status_code=204)
+    def delete(self, id, force=False):
         """Delete the specified Execution.
 
         :param id: UUID of execution to delete.
+        :param force: Optional. Force the deletion of unfinished executions.
+                      Default: false. While the api is backward compatible
+                      the behaviour is not the same. The new default is the
+                      safer option
         """
         acl.enforce('executions:delete', context.ctx())
 
         LOG.debug("Delete execution [id=%s]", id)
 
+        if not force:
+            wf_ex = db_api.get_workflow_execution(id)
+
+            if not states.is_completed(wf_ex.state):
+                raise exc.NotAllowedException(
+                    "Only completed executions can be deleted."
+                    " Execution {} is in {} state".format(id, wf_ex.state)
+                )
+
         return rest_utils.rest_retry_on_db_error(
             db_api.delete_workflow_execution
         )(id)

mistral/tests/unit/api/v2/test_executions.py

@@ -144,6 +144,14 @@ MOCK_EMPTY = mock.MagicMock(return_value=[])
 MOCK_NOT_FOUND = mock.MagicMock(side_effect=exc.DBEntityNotFoundError())
 MOCK_ACTION_EXC = mock.MagicMock(side_effect=exc.ActionException())
 
+ERROR_WF_EX = copy.deepcopy(WF_EX)
+ERROR_WF_EX['state'] = states.ERROR
+MOCK_ERROR_WF_EX = mock.MagicMock(return_value=ERROR_WF_EX)
+
+SUCCESS_WF_EX = copy.deepcopy(WF_EX)
+SUCCESS_WF_EX['state'] = states.SUCCESS
+MOCK_SUCCESS_WF_EX = mock.MagicMock(return_value=SUCCESS_WF_EX)
+
 
 @mock.patch.object(rpc_base, '_IMPL_CLIENT', mock.Mock())
 class TestExecutionsController(base.APITest):
@@ -693,8 +701,39 @@ class TestExecutionsController(base.APITest):
 
         self.assertIn('Bad response: 400', context.args[0])
 
-    @mock.patch.object(db_api, 'delete_workflow_execution', MOCK_DELETE)
-    def test_delete(self):
+    @mock.patch.object(
+        db_api,
+        'get_workflow_execution',
+        MOCK_WF_EX
+    )
+    def test_delete_unfished_execution(self):
+        resp = self.app.delete('/v2/executions/123', expect_errors=True)
+
+        self.assertEqual(403, resp.status_int)
+        self.assertIn(
+            "Only completed executions can be deleted. "
+            "Execution 123 is in RUNNING state",
+            resp.body.decode()
+        )
+
+    @mock.patch.object(db_api,
+                       'get_workflow_execution',
+                       MOCK_ERROR_WF_EX)
+    @mock.patch.object(db_api,
+                       'delete_workflow_execution',
+                       MOCK_DELETE)
+    def test_delete_error_exec(self):
+        resp = self.app.delete('/v2/executions/123')
+
+        self.assertEqual(204, resp.status_int)
+
+    @mock.patch.object(db_api,
+                       'get_workflow_execution',
+                       MOCK_SUCCESS_WF_EX)
+    @mock.patch.object(db_api,
+                       'delete_workflow_execution',
+                       MOCK_DELETE)
+    def test_delete_success_exec(self):
         resp = self.app.delete('/v2/executions/123')
 
         self.assertEqual(204, resp.status_int)

releasenotes/notes/force-stop-executions-00cd67dbbc9b5483.yaml

@@ -0,0 +1,9 @@
+---
+features:
+  - Use of the parameter force to forcefully delete executions. Note using this parameter
+    on unfinished executions might cause a cascade of errors.
+issues:
+  - Deleting unfinished executions might cause a cascade of errors, so the standard behaviour
+    has been changed to delete only safe to delete executions and a new parameter force
+    was added to forceful delete ignoring the state the execution is in.
+