From 06d7629b7e1dc4d173d032a90cc1fbfa2635cddb Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Jun 15 2020 11:39:47 +0000
Subject: backend, frontend: cancel also "starting" builds


There's no point to _not_ deliver a cancel request when the build is in
"starting" state.  At least now.

Also, from the backend perspective, try to check if there's the cancel
request always before we start doing some expensive actions.

Fixes: #1391

---

diff --git a/backend/copr_backend/background_worker_build.py b/backend/copr_backend/background_worker_build.py
index 92d3c15..dc66be0 100644
--- a/backend/copr_backend/background_worker_build.py
+++ b/backend/copr_backend/background_worker_build.py
@@ -350,6 +350,18 @@ class BuildBackgroundWorker(BackgroundWorker):
         self.canceled = bool(self.redis_get_worker_flag("cancel_request"))
         return self.canceled
 
+    def _cancel_if_requested(self):
+        """
+        Raise BuildCanceled exception if there's already a request for
+        cancellation.  This is useful as "quick and cheap check" before starting
+        some expensive task that would have to be later canceled anyways.
+        We can call this multiple times, anytime we feel it is appropriate.
+        """
+        if self._cancel_task_check_request():
+            self.log.warning("Canceling the build early")
+            self._drop_host()
+            raise BuildCanceled
+
     def _cancel_vm_allocation(self):
         self.redis_set_worker_flag("canceling", 1)
         self._drop_host()
@@ -659,10 +671,12 @@ class BuildBackgroundWorker(BackgroundWorker):
         failed = True
 
         self._wait_for_repo()
+        self._cancel_if_requested()
         self._alloc_host()
         self._alloc_ssh_connection()
         self._check_vm()
         self._fill_build_info_file()
+        self._cancel_if_requested()
         self._mark_running(attempt)
         self._start_remote_build()
         transfer_failure = CancellableThreadTask(
diff --git a/backend/tests/test_background_worker_build.py b/backend/tests/test_background_worker_build.py
index 53208d3..797b05b 100644
--- a/backend/tests/test_background_worker_build.py
+++ b/backend/tests/test_background_worker_build.py
@@ -534,6 +534,42 @@ def test_cancel_build_on_tail_log_no_ssh(f_build_rpm_sign_on, caplog):
 
 @_patch_bwbuild_object("CANCEL_CHECK_PERIOD", 0.5)
 @mock.patch("copr_backend.sign.SIGN_BINARY", "tests/fake-bin-sign")
+def test_cancel_before_vm(f_build_rpm_sign_on, caplog):
+    config = f_build_rpm_sign_on
+    worker = config.bw
+    # set this early
+    worker.redis_set_worker_flag("cancel_request", 1)
+    worker.process()
+    assert_logs_exist([
+        "Build was canceled",
+        "Canceling the build early",
+        COMMON_MSGS["not finished"],
+        "Worker failed build",
+    ], caplog)
+    assert_logs_dont_exist(["Releasing VM back to pool"], caplog)
+
+@_patch_bwbuild_object("CANCEL_CHECK_PERIOD", 0.5)
+@mock.patch("copr_backend.sign.SIGN_BINARY", "tests/fake-bin-sign")
+def test_cancel_before_start(f_build_rpm_sign_on, caplog):
+    config = f_build_rpm_sign_on
+    worker = config.bw
+
+    # cancel request right before starting the build
+    worker._fill_build_info_file = mock.MagicMock()
+    worker._fill_build_info_file.side_effect = \
+        lambda: worker.redis_set_worker_flag("cancel_request", 1)
+
+    worker.process()
+    assert_logs_exist([
+        "Build was canceled",
+        "Releasing VM back to pool",
+        "Canceling the build early",
+        COMMON_MSGS["not finished"],
+        "Worker failed build",
+    ], caplog)
+
+@_patch_bwbuild_object("CANCEL_CHECK_PERIOD", 0.5)
+@mock.patch("copr_backend.sign.SIGN_BINARY", "tests/fake-bin-sign")
 def test_build_retry(f_build_rpm_sign_on):
     config = f_build_rpm_sign_on
     worker = config.bw
diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py
index a6bd826..8060617 100644
--- a/frontend/coprs_frontend/coprs/logic/builds_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py
@@ -913,13 +913,13 @@ class BuildsLogic(object):
             raise InsufficientRightsException(
                 "You are not allowed to cancel this build.")
         if not build.cancelable:
-            if build.status == StatusEnum("starting"):
-                # this is not intuitive, that's why we provide more specific message
-                err_msg = "Cannot cancel build {} in state 'starting'".format(build.id)
-            else:
-                err_msg = "Cannot cancel build {}".format(build.id)
+            err_msg = "Cannot cancel build {}".format(build.id)
             raise ConflictingRequest(err_msg)
 
+        # No matter the state, we tell backend to cancel this build.  Even when
+        # the build is in pending state (worker manager may be already handling
+        # this build ATM, and creating an asynchronous worker which needs to be
+        # canceled).
         ActionsLogic.send_cancel_build(build)
 
         build.canceled = True
diff --git a/frontend/coprs_frontend/coprs/models.py b/frontend/coprs_frontend/coprs/models.py
index c15323a..ee13c66 100644
--- a/frontend/coprs_frontend/coprs/models.py
+++ b/frontend/coprs_frontend/coprs/models.py
@@ -1148,7 +1148,7 @@ class Build(db.Model, helpers.Serializer):
         """
         Find out if this build is cancelable.
         """
-        return not self.finished and self.status != StatusEnum("starting")
+        return not self.finished
 
     @property
     def repeatable(self):