launchpad-reviewers team mailing list archive

Thread
Date

[Merge] ~cjwatson/launchpad:build-status-gathering into launchpad:master

To: mp+447783@xxxxxxxxxxxxxxxxxx
From: Colin Watson <mp+447783@xxxxxxxxxxxxxxxxxx>
Date: Wed, 26 Jul 2023 13:20:20 -0000
Reply-to: mp+447783@xxxxxxxxxxxxxxxxxx
Sender: noreply@xxxxxxxxxxxxx

Colin Watson has proposed merging ~cjwatson/launchpad:build-status-gathering into launchpad:master.

Commit message:
Introduce a new GATHERING build status

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

If a build has large output files, it can take buildd-manager a long time to download those from the builder.  This happens in a single long scan cycle for the builder in question, and while it's in progress there's almost no visual indication of that fact; the only way to tell is that the build is in the BUILDING state with a build log (not just a log tail) attached to it.

More importantly, if somebody decides that a build is hung when it's in that state and tries to cancel it, the cancel attempt is largely ignored (there's currently no way to interrupt the downloads in progress) but when the download completes buildd-manager will raise `AssertionError: Can't change build status from CANCELLING to UPLOADING.`

To avoid this problem, introduce a new status called `GATHERING`, which builds pass through on their way from `BUILDING` to `UPLOADING`.  Builds in this state won't show up as cancellable, and it also makes the true state a little more visible to users.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:build-status-gathering into launchpad:master.

diff --git a/lib/lp/app/browser/tales.py b/lib/lp/app/browser/tales.py
index b8584c9..eb5cad2 100644
--- a/lib/lp/app/browser/tales.py
+++ b/lib/lp/app/browser/tales.py
@@ -1132,6 +1132,7 @@ class BuildImageDisplayAPI(ObjectImageDisplayAPI):
             BuildStatus.SUPERSEDED: {"src": "/@@/build-superseded"},
             BuildStatus.BUILDING: {"src": "/@@/processing"},
             BuildStatus.FAILEDTOUPLOAD: {"src": "/@@/build-failedtoupload"},
+            BuildStatus.GATHERING: {"src": "/@@/processing"},
             BuildStatus.UPLOADING: {"src": "/@@/processing"},
             BuildStatus.CANCELLING: {"src": "/@@/processing"},
             BuildStatus.CANCELLED: {"src": "/@@/build-failed"},
diff --git a/lib/lp/archiveuploader/tests/test_uploadprocessor.py b/lib/lp/archiveuploader/tests/test_uploadprocessor.py
index 665bc0b..22255bf 100644
--- a/lib/lp/archiveuploader/tests/test_uploadprocessor.py
+++ b/lib/lp/archiveuploader/tests/test_uploadprocessor.py
@@ -3105,6 +3105,20 @@ class TestUploadHandler(TestUploadProcessorBase):
         self.assertEqual(BuildStatus.BUILDING, build.status)
         self.assertLogContains("Build status is BUILDING. Ignoring.")
 
+    def testBuildStillGathering(self):
+        # Builds that are still GATHERING should be left alone.  The
+        # upload directory may already be in place, but buildd-manager
+        # will set the status to UPLOADING when it's handed off.
+        build, leaf_name = self.processUploadWithBuildStatus(
+            BuildStatus.GATHERING
+        )
+        # The build status is not changed
+        self.assertTrue(
+            os.path.exists(os.path.join(self.incoming_folder, leaf_name))
+        )
+        self.assertEqual(BuildStatus.GATHERING, build.status)
+        self.assertLogContains("Build status is GATHERING. Ignoring.")
+
     def testBuildWithInvalidStatus(self):
         # Builds with an invalid (not UPLOADING or BUILDING) status
         # should trigger a failure. We've probably raced with
@@ -3121,8 +3135,8 @@ class TestUploadHandler(TestUploadProcessorBase):
         )
         self.assertEqual(BuildStatus.NEEDSBUILD, build.status)
         self.assertLogContains(
-            "Expected build status to be UPLOADING or BUILDING, was "
-            "NEEDSBUILD."
+            "Expected build status to be BUILDING, GATHERING, or UPLOADING; "
+            "was NEEDSBUILD."
         )
 
     def testOrderFilenames(self):
diff --git a/lib/lp/archiveuploader/uploadprocessor.py b/lib/lp/archiveuploader/uploadprocessor.py
index bb7c326..f7cd1ae 100644
--- a/lib/lp/archiveuploader/uploadprocessor.py
+++ b/lib/lp/archiveuploader/uploadprocessor.py
@@ -792,19 +792,25 @@ class BuildUploadHandler(UploadHandler):
             )
             self.moveUpload(UploadStatusEnum.FAILED, logger)
             return
-        elif self.build.status == BuildStatus.BUILDING:
+        elif self.build.status in (
+            BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
+        ):
             # Handoff from buildd-manager isn't complete until the
             # status is UPLOADING.
-            self.processor.log.info("Build status is BUILDING. Ignoring.")
+            self.processor.log.info(
+                "Build status is %s. Ignoring.", self.build.status.name
+            )
             return
         elif self.build.status != BuildStatus.UPLOADING:
-            # buildd-manager should only have given us a directory
-            # during BUILDING or UPLOADING. Anything else indicates a
+            # buildd-manager should only have given us a directory during
+            # BUILDING, GATHERING, or UPLOADING. Anything else indicates a
             # bug, so get the upload out of the queue before the status
             # changes to something that might accidentally let it be
             # accepted.
             self.processor.log.warning(
-                "Expected build status to be UPLOADING or BUILDING, was %s.",
+                "Expected build status to be BUILDING, GATHERING, or "
+                "UPLOADING; was %s.",
                 self.build.status.name,
             )
             self.moveUpload(UploadStatusEnum.FAILED, logger)
diff --git a/lib/lp/buildmaster/enums.py b/lib/lp/buildmaster/enums.py
index ee1d90c..672bc80 100644
--- a/lib/lp/buildmaster/enums.py
+++ b/lib/lp/buildmaster/enums.py
@@ -123,8 +123,8 @@ class BuildStatus(DBEnumeratedType):
         """
         Uploading build
 
-        The build has completed and is waiting to be processed by the
-        upload processor.
+        The build has completed, its output files have been gathered, and it
+        is waiting to be processed by the upload processor.
         """,
     )
 
@@ -148,6 +148,18 @@ class BuildStatus(DBEnumeratedType):
         """,
     )
 
+    # It would be clearer for this to be before UPLOADING, but unfortunately
+    # the mapping of this enumeration to integers is too dense.
+    GATHERING = DBItem(
+        11,
+        """
+        Gathering build output
+
+        The build has completed and buildd-manager is in the process of
+        downloading output files from the builder.
+        """,
+    )
+
 
 class BuildFarmJobType(DBEnumeratedType):
     """Soyuz build farm job type.
diff --git a/lib/lp/buildmaster/model/buildfarmjob.py b/lib/lp/buildmaster/model/buildfarmjob.py
index e69651f..a152c14 100644
--- a/lib/lp/buildmaster/model/buildfarmjob.py
+++ b/lib/lp/buildmaster/model/buildfarmjob.py
@@ -41,6 +41,11 @@ VALID_STATUS_TRANSITIONS = {
     BuildStatus.SUPERSEDED: (),
     BuildStatus.BUILDING: tuple(BuildStatus.items),
     BuildStatus.FAILEDTOUPLOAD: (BuildStatus.NEEDSBUILD,),
+    BuildStatus.GATHERING: (
+        BuildStatus.NEEDSBUILD,
+        BuildStatus.FAILEDTOBUILD,
+        BuildStatus.UPLOADING,
+    ),
     BuildStatus.UPLOADING: (
         BuildStatus.FULLYBUILT,
         BuildStatus.FAILEDTOUPLOAD,
@@ -174,6 +179,7 @@ class BuildFarmJobMixin:
             BuildStatus.BUILDING,
             BuildStatus.CANCELLED,
             BuildStatus.CANCELLING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.SUPERSEDED,
         ]
@@ -251,6 +257,7 @@ class BuildFarmJobMixin:
             not in (
                 BuildStatus.NEEDSBUILD,
                 BuildStatus.BUILDING,
+                BuildStatus.GATHERING,
                 BuildStatus.CANCELLING,
             )
         ):
@@ -435,6 +442,7 @@ class BuildFarmJobSet:
         unfinished_states = [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.SUPERSEDED,
         ]
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 450e375..ae81888 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -441,6 +441,10 @@ class BuildFarmJobBehaviourBase:
         )
         os.makedirs(upload_path)
 
+        # Indicate that downloads are in progress.
+        build.updateStatus(BuildStatus.GATHERING, worker_status=worker_status)
+        transaction.commit()
+
         yield self._downloadFiles(worker_status, upload_path, logger)
 
         transaction.commit()
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjob.py b/lib/lp/buildmaster/tests/test_buildfarmjob.py
index 8b8739b..caf952f 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjob.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjob.py
@@ -218,7 +218,10 @@ class TestBuildFarmJobMixin(TestCaseWithFactory):
             build.updateStatus(BuildStatus.BUILDING)
             self.assertIsNot(None, build.date_started)
             self.assertIs(None, build.date_finished)
-            build.updateStatus(status)
+            build.updateStatus(BuildStatus.GATHERING)
+            self.assertIsNot(None, build.date_started)
+            self.assertIs(None, build.date_finished)
+            build.updateStatus(status, force_invalid_transition=True)
             self.assertIsNot(None, build.date_started)
             self.assertIsNot(None, build.date_finished)
 
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index 11c6a3e..4de9565 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,29 @@ class TestHandleStatusMixin:
         self.assertResultCount(1, "incoming")
 
     @defer.inlineCallbacks
+    def test_handleStatus_WAITING_OK_gathering_status(self):
+        # handleStatus sets the build's status to GATHERING while
+        # downloading files from the builder.
+        def mock_downloadFiles(*args, **kwargs):
+            self.assertEqual(BuildStatus.GATHERING, self.build.status)
+
+        self.useFixture(
+            MockPatchObject(
+                self.behaviour, "_downloadFiles", mock_downloadFiles
+            )
+        )
+        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)
+
+    @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.
diff --git a/lib/lp/charms/model/charmrecipe.py b/lib/lp/charms/model/charmrecipe.py
index 2195dab..73d2aea 100644
--- a/lib/lp/charms/model/charmrecipe.py
+++ b/lib/lp/charms/model/charmrecipe.py
@@ -749,6 +749,7 @@ class CharmRecipe(StormBase, WebhookTargetMixin):
         return [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.CANCELLING,
         ]
diff --git a/lib/lp/code/browser/sourcepackagerecipebuild.py b/lib/lp/code/browser/sourcepackagerecipebuild.py
index cb60777..9e374dd 100644
--- a/lib/lp/code/browser/sourcepackagerecipebuild.py
+++ b/lib/lp/code/browser/sourcepackagerecipebuild.py
@@ -73,6 +73,7 @@ class SourcePackageRecipeBuildView(LaunchpadView):
             return "No suitable builders"
         return {
             BuildStatus.NEEDSBUILD: "Pending build",
+            BuildStatus.GATHERING: "Gathering build output",
             BuildStatus.UPLOADING: "Build uploading",
             BuildStatus.FULLYBUILT: "Successful build",
             BuildStatus.MANUALDEPWAIT: (
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 068e286..aa15c37 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -733,6 +733,7 @@ class OCIRecipe(StormBase, WebhookTargetMixin):
         return [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.CANCELLING,
         ]
@@ -1085,7 +1086,9 @@ class OCIRecipeSet:
             BuildStatus.FAILEDTOUPLOAD,
         )
         needsbuild = collect_builds(BuildStatus.NEEDSBUILD)
-        building = collect_builds(BuildStatus.BUILDING, BuildStatus.UPLOADING)
+        building = collect_builds(
+            BuildStatus.BUILDING, BuildStatus.GATHERING, BuildStatus.UPLOADING
+        )
         successful = collect_builds(BuildStatus.FULLYBUILT)
         cancelled = collect_builds(
             BuildStatus.CANCELLING, BuildStatus.CANCELLED
diff --git a/lib/lp/registry/model/sourcepackage.py b/lib/lp/registry/model/sourcepackage.py
index 928996c..329e692 100644
--- a/lib/lp/registry/model/sourcepackage.py
+++ b/lib/lp/registry/model/sourcepackage.py
@@ -693,13 +693,14 @@ class SourcePackage(
         )
 
         # Ordering according status
-        # * NEEDSBUILD, BUILDING & UPLOADING by -lastscore
+        # * NEEDSBUILD, BUILDING, GATHERING & UPLOADING by -lastscore
         # * SUPERSEDED by -datecreated
         # * FULLYBUILT & FAILURES by -datebuilt
         # It should present the builds in a more natural order.
         if build_state in [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
         ]:
             orderBy = ["-BuildQueue.lastscore"]
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index c55f435..ddaa13d 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1272,6 +1272,7 @@ class Snap(StormBase, WebhookTargetMixin):
         return [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.CANCELLING,
         ]
diff --git a/lib/lp/soyuz/mail/binarypackagebuild.py b/lib/lp/soyuz/mail/binarypackagebuild.py
index 3124676..d6a6b63 100644
--- a/lib/lp/soyuz/mail/binarypackagebuild.py
+++ b/lib/lp/soyuz/mail/binarypackagebuild.py
@@ -223,7 +223,7 @@ class BinaryPackageBuildMailer(BaseMailer):
             buildduration = "not available"
             buildlog_url = "not available"
             builder_url = "not available"
-        elif build.status == BuildStatus.UPLOADING:
+        elif build.status in (BuildStatus.GATHERING, BuildStatus.UPLOADING):
             buildduration = "uploading"
             buildlog_url = "see builder page"
             builder_url = "not available"
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 72264d4..b098467 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -1534,6 +1534,7 @@ class Archive(SQLBase):
             ),
             "pending": [
                 BuildStatus.BUILDING,
+                BuildStatus.GATHERING,
                 BuildStatus.UPLOADING,
             ],
             "succeeded": (BuildStatus.FULLYBUILT,),
@@ -1546,6 +1547,7 @@ class Archive(SQLBase):
                 BuildStatus.FAILEDTOUPLOAD,
                 BuildStatus.MANUALDEPWAIT,
                 BuildStatus.BUILDING,
+                BuildStatus.GATHERING,
                 BuildStatus.UPLOADING,
                 BuildStatus.FULLYBUILT,
                 BuildStatus.SUPERSEDED,
diff --git a/lib/lp/soyuz/model/binarypackagebuild.py b/lib/lp/soyuz/model/binarypackagebuild.py
index b71a38d..4846830 100644
--- a/lib/lp/soyuz/model/binarypackagebuild.py
+++ b/lib/lp/soyuz/model/binarypackagebuild.py
@@ -332,6 +332,7 @@ class BinaryPackageBuild(PackageBuildMixin, SQLBase):
         return self.status not in [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.SUPERSEDED,
         ]
@@ -1091,7 +1092,7 @@ class BinaryPackageBuildSet(SpecificBuildFarmJobSourceMixin):
             )
 
         # Ordering according status
-        # * NEEDSBUILD, BUILDING & UPLOADING by -lastscore
+        # * NEEDSBUILD, BUILDING, GATHERING & UPLOADING by -lastscore
         # * SUPERSEDED & All by -BinaryPackageBuild.id
         #   (nearly equivalent to -datecreated, but much more
         #   efficient.)
@@ -1102,6 +1103,7 @@ class BinaryPackageBuildSet(SpecificBuildFarmJobSourceMixin):
         if status in [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
         ]:
             order_by = [Desc(BuildQueue.lastscore), BinaryPackageBuild.id]
@@ -1239,7 +1241,9 @@ class BinaryPackageBuildSet(SpecificBuildFarmJobSourceMixin):
             BuildStatus.FAILEDTOUPLOAD,
         )
         needsbuild = collect_builds(BuildStatus.NEEDSBUILD)
-        building = collect_builds(BuildStatus.BUILDING, BuildStatus.UPLOADING)
+        building = collect_builds(
+            BuildStatus.BUILDING, BuildStatus.GATHERING, BuildStatus.UPLOADING
+        )
         successful = collect_builds(BuildStatus.FULLYBUILT)
 
         # Note: the BuildStatus DBItems are used here to summarize the
diff --git a/lib/lp/soyuz/model/livefs.py b/lib/lp/soyuz/model/livefs.py
index 5da4a28..0412b0e 100644
--- a/lib/lp/soyuz/model/livefs.py
+++ b/lib/lp/soyuz/model/livefs.py
@@ -272,6 +272,7 @@ class LiveFS(StormBase, WebhookTargetMixin):
         return [
             BuildStatus.NEEDSBUILD,
             BuildStatus.BUILDING,
+            BuildStatus.GATHERING,
             BuildStatus.UPLOADING,
             BuildStatus.CANCELLING,
         ]
diff --git a/lib/lp/soyuz/tests/test_build_notify.py b/lib/lp/soyuz/tests/test_build_notify.py
index 7813c4f..8dd291e 100644
--- a/lib/lp/soyuz/tests/test_build_notify.py
+++ b/lib/lp/soyuz/tests/test_build_notify.py
@@ -163,7 +163,7 @@ class TestBuildNotify(TestCaseWithFactory):
             duration = "not available"
             build_log = "not available"
             builder = "not available"
-        elif build.status == BuildStatus.UPLOADING:
+        elif build.status in (BuildStatus.GATHERING, BuildStatus.UPLOADING):
             duration = "uploading"
             build_log = "see builder page"
             builder = "not available"