launchpad-reviewers team mailing list archive

[Colin Watson <mp+447565@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:build-status-allow-cancelling-to-uploading into launchpad:master.

Commit message:
Allow build status transitions from CANCELLING to UPLOADING

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/447565

It turns out to be possible for this transition to happen in practice if a cancel request is sent for a build while buildd-manager is gathering its output files, which can be quite a large window since the entire download happens as part of a single scan cycle for a builder.  Ideally we'd probably restructure buildd-manager so that builds can be effectively cancelled during this process, but that's considerably more work.  In the meantime, I don't think it should actually cause a practical problem if we effectively ignore the cancel request in this case and allow the build to complete normally, and it will avoid some unnecessary large failures piling up on the buildd-manager unit.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:build-status-allow-cancelling-to-uploading into launchpad:master.

diff --git a/lib/lp/buildmaster/model/buildfarmjob.py b/lib/lp/buildmaster/model/buildfarmjob.py
index e69651f..e6c7663 100644
--- a/lib/lp/buildmaster/model/buildfarmjob.py
+++ b/lib/lp/buildmaster/model/buildfarmjob.py
@@ -46,7 +46,17 @@ VALID_STATUS_TRANSITIONS = {
         BuildStatus.FAILEDTOUPLOAD,
         BuildStatus.SUPERSEDED,
     ),
-    BuildStatus.CANCELLING: (BuildStatus.CANCELLED,),
+    BuildStatus.CANCELLING: (
+        # Gathering a build's output files is all done in the middle of a
+        # single iteration of its scan loop, so there's no opportunity for
+        # cancelling a build to interrupt that process.  As a result, it's
+        # possible and somewhat reasonable for a build to go from CANCELLING
+        # to UPLOADING if it was cancelled while gathering files, perhaps
+        # because the files were very large and so the build appeared to
+        # have hung.
+        BuildStatus.UPLOADING,
+        BuildStatus.CANCELLED,
+    ),
     BuildStatus.CANCELLED: (BuildStatus.NEEDSBUILD,),
 }
 
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index 11c6a3e..6fd202c 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -11,6 +11,7 @@ from collections import OrderedDict
 from datetime import datetime
 
 import six
+from fixtures import MockPatchObject
 from testtools import ExpectedException
 from testtools.twistedsupport import AsynchronousDeferredRunTest
 from twisted.internet import defer
@@ -489,6 +490,37 @@ class TestHandleStatusMixin:
         self.assertResultCount(1, "incoming")
 
     @defer.inlineCallbacks
+    def test_handleStatus_WAITING_OK_normal_file_cancel_while_gathering(self):
+        # Cancelling a build while it is in the process of gathering files
+        # does not (currently) interrupt that process; it leaves the build
+        # in the CANCELLING status until the scan cycle for that builder
+        # finishes (which in practice means until gathering has completed or
+        # failed).  While this is a bit odd, if gathering completes anyway
+        # then we might as well upload the results of the build.
+        original_handleSuccess = self.behaviour.handleSuccess
+
+        def cancel_then_handle_success(*args, **kwargs):
+            self.build.cancel()
+            return original_handleSuccess(*args, **kwargs)
+
+        self.useFixture(
+            MockPatchObject(
+                self.behaviour, "handleSuccess", cancel_then_handle_success
+            )
+        )
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record,
+                {
+                    "builder_status": "BuilderStatus.WAITING",
+                    "build_status": "BuildStatus.OK",
+                    "filemap": {"myfile.py": "test_file_hash"},
+                },
+            )
+        self.assertEqual(BuildStatus.UPLOADING, self.build.status)
+        self.assertResultCount(1, "incoming")
+
+    @defer.inlineCallbacks
     def test_handleStatus_WAITING_OK_absolute_filepath(self):
         # A filemap that tries to write to files outside of the upload
         # directory will not be collected.