Merge ~cjwatson/launchpad:refactor-updateBuild into launchpad:master

[Colin Watson]

Push more of BuilderInteractor.updateBuild down to behaviours

For upcoming work on CI jobs, we want to be able to do some build-type-specific work on non-terminal build states, namely incrementally fetching output files from builders. This requires `updateBuild` to call something build-type-specific in the `BUILDING` state. Since there's a small amount of code in common between `BUILDING`/`ABORTING` and `WAITING` already (the `updateStatus` call and the following commit), push this all down to `BuildFarmJobBehaviour.handleStatus`, which now handles everything except fetching the logtail.

No functional change is intended from this refactoring, although a number of tests need to change because `handleStatus` now requires a more complete version of launchpad-buildd's `status` response; previously some tests could get away with omitting some fields.

[Ioana Lasc (ilasc)]

LGTM

review: Approve

diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index e8a1d7f..679d5c6 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __all__ = [
@@ -526,19 +526,6 @@ class BuilderInteractor:
         return candidate
 
     @staticmethod
-    def extractBuildStatus(worker_status):
-        """Read build status name.
-
-        :param worker_status: build status dict from BuilderWorker.status.
-        :return: the unqualified status name, e.g. "OK".
-        """
-        status_string = worker_status['build_status']
-        lead_string = 'BuildStatus.'
-        assert status_string.startswith(lead_string), (
-            "Malformed status string: '%s'" % status_string)
-        return status_string[len(lead_string):]
-
-    @staticmethod
     def extractLogTail(worker_status):
         """Extract the log tail from a builder status response.
 
@@ -580,24 +567,20 @@ class BuilderInteractor:
         # matches the DB, and this method isn't called unless the DB
         # says there's a job.
         builder_status = worker_status['builder_status']
+        if builder_status not in (
+                'BuilderStatus.BUILDING', 'BuilderStatus.ABORTING',
+                'BuilderStatus.WAITING'):
+            raise AssertionError("Unknown status %s" % builder_status)
+        builder = builder_factory[vitals.name]
+        behaviour = behaviour_factory(vitals.build_queue, builder, worker)
         if builder_status in (
                 'BuilderStatus.BUILDING', 'BuilderStatus.ABORTING'):
             logtail = cls.extractLogTail(worker_status)
             if logtail is not None:
                 manager.addLogTail(vitals.build_queue.id, logtail)
-            vitals.build_queue.specific_build.updateStatus(
-                vitals.build_queue.specific_build.status,
-                worker_status=worker_status)
-            transaction.commit()
-        elif builder_status == 'BuilderStatus.WAITING':
-            # Build has finished. Delegate handling to the build itself.
-            builder = builder_factory[vitals.name]
-            behaviour = behaviour_factory(vitals.build_queue, builder, worker)
-            yield behaviour.handleStatus(
-                vitals.build_queue, cls.extractBuildStatus(worker_status),
-                worker_status)
-        else:
-            raise AssertionError("Unknown status %s" % builder_status)
+        # Delegate the remaining handling to the build behaviour, which will
+        # commit the transaction.
+        yield behaviour.handleStatus(vitals.build_queue, worker_status)
 
     @staticmethod
     def _getWorkerScannerLogger():
diff --git a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
index 2d0eed4..c529327 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for build farm job behaviours."""
@@ -88,10 +88,9 @@ class IBuildFarmJobBehaviour(Interface):
     def verifySuccessfulBuild():
         """Check that we are allowed to collect this successful build."""
 
-    def handleStatus(bq, status, worker_status):
-        """Update the build from a WAITING worker result.
+    def handleStatus(bq, worker_status):
+        """Update the build from a worker's status response.
 
         :param bq: The `BuildQueue` currently being processed.
-        :param status: The tail of the BuildStatus (eg. OK or PACKAGEFAIL).
         :param worker_status: Worker status dict from `BuilderWorker.status`.
         """
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 0315b52..a7537d3 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Base and idle BuildFarmJobBehaviour classes."""
@@ -33,6 +33,7 @@ from lp.services.config import config
 from lp.services.helpers import filenameToContentType
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.utils import copy_and_close
+from lp.services.propertycache import cachedproperty
 from lp.services.statsd.interfaces.statsd_client import IStatsdClient
 from lp.services.utils import sanitise_urls
 from lp.services.webapp import canonical_url
@@ -53,7 +54,10 @@ class BuildFarmJobBehaviourBase:
         """Store a reference to the job_type with which we were created."""
         self.build = build
         self._builder = None
-        self._authserver = xmlrpc.Proxy(
+
+    @cachedproperty
+    def _authserver(self):
+        return xmlrpc.Proxy(
             config.builddmaster.authentication_endpoint.encode('UTF-8'),
             connectTimeout=config.builddmaster.authentication_timeout)
 
@@ -255,6 +259,19 @@ class BuildFarmJobBehaviourBase:
                 "%s (%s) can not be built for pocket %s in %s: illegal status"
                 % (build.title, build.id, build.pocket.name, build.archive))
 
+    @staticmethod
+    def extractBuildStatus(worker_status):
+        """Read build status name.
+
+        :param worker_status: build status dict from BuilderWorker.status.
+        :return: the unqualified status name, e.g. "OK".
+        """
+        status_string = worker_status['build_status']
+        lead_string = 'BuildStatus.'
+        assert status_string.startswith(lead_string), (
+            "Malformed status string: '%s'" % status_string)
+        return status_string[len(lead_string):]
+
     # The list of build status values for which email notifications are
     # allowed to be sent. It is up to each callback as to whether it will
     # consider sending a notification but it won't do so if the status is not
@@ -262,57 +279,65 @@ class BuildFarmJobBehaviourBase:
     ALLOWED_STATUS_NOTIFICATIONS = ['PACKAGEFAIL', 'CHROOTFAIL']
 
     @defer.inlineCallbacks
-    def handleStatus(self, bq, status, worker_status):
+    def handleStatus(self, bq, worker_status):
         """See `IBuildFarmJobBehaviour`."""
         if bq != self.build.buildqueue_record:
             raise AssertionError(
                 "%r != %r" % (bq, self.build.buildqueue_record))
         from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME
         logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
-        notify = status in self.ALLOWED_STATUS_NOTIFICATIONS
-        fail_status_map = {
-            'PACKAGEFAIL': BuildStatus.FAILEDTOBUILD,
-            'DEPFAIL': BuildStatus.MANUALDEPWAIT,
-            'CHROOTFAIL': BuildStatus.CHROOTWAIT,
-            }
-        if self.build.status == BuildStatus.CANCELLING:
-            fail_status_map['ABORTED'] = BuildStatus.CANCELLED
-
-        logger.info(
-            'Processing finished job %s (%s) from builder %s: %s'
-            % (self.build.build_cookie, self.build.title,
-               self.build.buildqueue_record.builder.name, status))
-        build_status = None
-        if status == 'OK':
-            yield self.storeLogFromWorker()
-            # handleSuccess will sometimes perform write operations
-            # outside the database transaction, so a failure between
-            # here and the commit can cause duplicated results. For
-            # example, a BinaryPackageBuild will end up in the upload
-            # queue twice if notify() crashes.
-            build_status = yield self.handleSuccess(worker_status, logger)
-        elif status in fail_status_map:
-            # XXX wgrant: The builder should be set long before here, but
-            # currently isn't.
-            yield self.storeLogFromWorker()
-            build_status = fail_status_map[status]
+        builder_status = worker_status["builder_status"]
+
+        if builder_status == "BuilderStatus.WAITING":
+            # Build has finished.
+            status = self.extractBuildStatus(worker_status)
+            notify = status in self.ALLOWED_STATUS_NOTIFICATIONS
+            fail_status_map = {
+                'PACKAGEFAIL': BuildStatus.FAILEDTOBUILD,
+                'DEPFAIL': BuildStatus.MANUALDEPWAIT,
+                'CHROOTFAIL': BuildStatus.CHROOTWAIT,
+                }
+            if self.build.status == BuildStatus.CANCELLING:
+                fail_status_map['ABORTED'] = BuildStatus.CANCELLED
+
+            logger.info(
+                'Processing finished job %s (%s) from builder %s: %s'
+                % (self.build.build_cookie, self.build.title,
+                   self.build.buildqueue_record.builder.name, status))
+            build_status = None
+            if status == 'OK':
+                yield self.storeLogFromWorker()
+                # handleSuccess will sometimes perform write operations
+                # outside the database transaction, so a failure between
+                # here and the commit can cause duplicated results. For
+                # example, a BinaryPackageBuild will end up in the upload
+                # queue twice if notify() crashes.
+                build_status = yield self.handleSuccess(worker_status, logger)
+            elif status in fail_status_map:
+                yield self.storeLogFromWorker()
+                build_status = fail_status_map[status]
+            else:
+                raise BuildDaemonError(
+                    "Build returned unexpected status: %r" % status)
         else:
-            raise BuildDaemonError(
-                "Build returned unexpected status: %r" % status)
+            # The build status remains unchanged.
+            build_status = bq.specific_build.status
 
-        # Set the status and dequeue the build atomically. Setting the
-        # status to UPLOADING constitutes handoff to process-upload, so
-        # doing that before we've removed the BuildQueue causes races.
+        # Set the status and (if the build has finished) dequeue the build
+        # atomically.  Setting the status to UPLOADING constitutes handoff to
+        # process-upload, so doing that before we've removed the BuildQueue
+        # causes races.
 
         # XXX wgrant: The builder should be set long before here, but
         # currently isn't.
         self.build.updateStatus(
-            build_status,
-            builder=self.build.buildqueue_record.builder,
-            worker_status=worker_status)
-        if notify:
-            self.build.notify()
-        self.build.buildqueue_record.destroySelf()
+            build_status, builder=bq.builder, worker_status=worker_status)
+
+        if builder_status == "BuilderStatus.WAITING":
+            if notify:
+                self.build.notify()
+            self.build.buildqueue_record.destroySelf()
+
         transaction.commit()
 
     @defer.inlineCallbacks
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index ad05908..f36750e 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for BuildFarmJobBehaviourBase."""
@@ -30,6 +30,7 @@ from lp.buildmaster.interfaces.builder import BuildDaemonError
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
+from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
@@ -154,6 +155,22 @@ class TestBuildFarmJobBehaviourBase(TestCaseWithFactory):
         behaviour.setBuilder(self.factory.makeBuilder(virtualized=False), None)
         self.assertIs(False, behaviour.extraBuildArgs()["fast_cleanup"])
 
+    def test_extractBuildStatus_baseline(self):
+        # extractBuildStatus picks the name of the build status out of a
+        # dict describing the worker's status.
+        worker_status = {"build_status": "BuildStatus.BUILDING"}
+        self.assertEqual(
+            "BUILDING",
+            BuildFarmJobBehaviourBase.extractBuildStatus(worker_status))
+
+    def test_extractBuildStatus_malformed(self):
+        # extractBuildStatus errors out when the status string is not
+        # of the form it expects.
+        worker_status = {"build_status": "BUILDING"}
+        self.assertRaises(
+            AssertionError, BuildFarmJobBehaviourBase.extractBuildStatus,
+            worker_status)
+
 
 class TestDispatchBuildToWorker(StatsMixin, TestCase):
 
@@ -383,19 +400,44 @@ class TestHandleStatusMixin:
             1, len(os.listdir(os.path.join(self.upload_root, result))))
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_normal_file(self):
+    def test_handleStatus_BUILDING(self):
+        # If the builder is BUILDING (or any status other than WAITING),
+        # then the behaviour calls updateStatus but doesn't do anything
+        # else.
+        initial_status = self.build.status
+        bq_id = self.build.buildqueue_record.id
+        worker_status = {"builder_status": "BuilderStatus.BUILDING"}
+        removeSecurityProxy(self.build).updateStatus = FakeMethod()
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, worker_status)
+        self.assertEqual(None, self.build.log)
+        self.assertEqual(0, len(os.listdir(self.upload_root)))
+        self.assertEqual(
+            [((initial_status,),
+              {"builder": self.builder, "worker_status": worker_status})],
+            removeSecurityProxy(self.build).updateStatus.calls)
+        self.assertEqual(0, len(pop_notifications()), "Notifications received")
+        self.assertEqual(
+            self.build.buildqueue_record,
+            getUtility(IBuildQueueSet).get(bq_id))
+
+    @defer.inlineCallbacks
+    def test_handleStatus_WAITING_OK_normal_file(self):
         # A filemap with plain filenames should not cause a problem.
         # The call to handleStatus will attempt to get the file from
         # the worker resulting in a URL error in this test case.
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': {'myfile.py': 'test_file_hash'}})
+                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_OK_absolute_filepath(self):
+    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.
         with ExpectedException(
@@ -403,11 +445,13 @@ class TestHandleStatusMixin:
                 "Build returned a file named '/tmp/myfile.py'."):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, 'OK',
-                    {'filemap': {'/tmp/myfile.py': 'test_file_hash'}})
+                    self.build.buildqueue_record,
+                    {'builder_status': 'BuilderStatus.WAITING',
+                     'build_status': 'BuildStatus.OK',
+                     'filemap': {'/tmp/myfile.py': 'test_file_hash'}})
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_relative_filepath(self):
+    def test_handleStatus_WAITING_OK_relative_filepath(self):
         # A filemap that tries to write to files outside of
         # the upload directory will not be collected.
         with ExpectedException(
@@ -415,21 +459,25 @@ class TestHandleStatusMixin:
                 "Build returned a file named '../myfile.py'."):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, 'OK',
-                    {'filemap': {'../myfile.py': 'test_file_hash'}})
+                    self.build.buildqueue_record,
+                    {'builder_status': 'BuilderStatus.WAITING',
+                     'build_status': 'BuildStatus.OK',
+                     'filemap': {'../myfile.py': 'test_file_hash'}})
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_sets_build_log(self):
+    def test_handleStatus_WAITING_OK_sets_build_log(self):
         # The build log is set during handleStatus.
         self.assertEqual(None, self.build.log)
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': {'myfile.py': 'test_file_hash'}})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': {'myfile.py': 'test_file_hash'}})
         self.assertNotEqual(None, self.build.log)
 
     @defer.inlineCallbacks
-    def _test_handleStatus_notifies(self, status):
+    def _test_handleStatus_WAITING_notifies(self, status):
         # An email notification is sent for a given build status if
         # notifications are allowed for that status.
         expected_notification = (
@@ -437,7 +485,9 @@ class TestHandleStatusMixin:
 
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, status, {})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.%s' % status})
 
         if expected_notification:
             self.assertNotEqual(
@@ -446,26 +496,28 @@ class TestHandleStatusMixin:
             self.assertEqual(
                 0, len(pop_notifications()), "Notifications received")
 
-    def test_handleStatus_DEPFAIL_notifies(self):
-        return self._test_handleStatus_notifies("DEPFAIL")
+    def test_handleStatus_WAITING_DEPFAIL_notifies(self):
+        return self._test_handleStatus_WAITING_notifies("DEPFAIL")
 
-    def test_handleStatus_CHROOTFAIL_notifies(self):
-        return self._test_handleStatus_notifies("CHROOTFAIL")
+    def test_handleStatus_WAITING_CHROOTFAIL_notifies(self):
+        return self._test_handleStatus_WAITING_notifies("CHROOTFAIL")
 
-    def test_handleStatus_PACKAGEFAIL_notifies(self):
-        return self._test_handleStatus_notifies("PACKAGEFAIL")
+    def test_handleStatus_WAITING_PACKAGEFAIL_notifies(self):
+        return self._test_handleStatus_WAITING_notifies("PACKAGEFAIL")
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_cancels_cancelling(self):
+    def test_handleStatus_WAITING_ABORTED_cancels_cancelling(self):
         with dbuser(config.builddmaster.dbuser):
             self.build.updateStatus(BuildStatus.CANCELLING)
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, "ABORTED", {})
+                self.build.buildqueue_record,
+                {"builder_status": "BuilderStatus.WAITING",
+                 "build_status": "BuildStatus.ABORTED"})
         self.assertEqual(0, len(pop_notifications()), "Notifications received")
         self.assertEqual(BuildStatus.CANCELLED, self.build.status)
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_illegal_when_building(self):
+    def test_handleStatus_WAITING_ABORTED_illegal_when_building(self):
         self.builder.vm_host = "fake_vm_host"
         self.behaviour = self.interactor.getBuildBehaviour(
             self.build.buildqueue_record, self.builder, self.worker)
@@ -475,16 +527,20 @@ class TestHandleStatusMixin:
                     BuildDaemonError,
                     "Build returned unexpected status: %r" % 'ABORTED'):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "ABORTED", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.ABORTED"})
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_cancelling_sets_build_log(self):
+    def test_handleStatus_WAITING_ABORTED_cancelling_sets_build_log(self):
         # If a build is intentionally cancelled, the build log is set.
         self.assertEqual(None, self.build.log)
         with dbuser(config.builddmaster.dbuser):
             self.build.updateStatus(BuildStatus.CANCELLING)
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, "ABORTED", {})
+                self.build.buildqueue_record,
+                {"builder_status": "BuilderStatus.WAITING",
+                 "build_status": "BuildStatus.ABORTED"})
         self.assertNotEqual(None, self.build.log)
 
     @defer.inlineCallbacks
@@ -493,8 +549,10 @@ class TestHandleStatusMixin:
         self.assertEqual(None, self.build.date_finished)
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': {'myfile.py': 'test_file_hash'}})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': {'myfile.py': 'test_file_hash'}})
         self.assertNotEqual(None, self.build.date_finished)
 
     @defer.inlineCallbacks
@@ -504,7 +562,9 @@ class TestHandleStatusMixin:
                 "Build returned unexpected status: %r" % 'GIVENBACK'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "GIVENBACK", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.GIVENBACK"})
 
     @defer.inlineCallbacks
     def test_builderfail_collection(self):
@@ -513,7 +573,9 @@ class TestHandleStatusMixin:
                 "Build returned unexpected status: %r" % 'BUILDERFAIL'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "BUILDERFAIL", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.BUILDERFAIL"})
 
     @defer.inlineCallbacks
     def test_invalid_status_collection(self):
@@ -522,4 +584,6 @@ class TestHandleStatusMixin:
                 "Build returned unexpected status: %r" % 'BORKED'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "BORKED", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.BORKED"})
diff --git a/lib/lp/buildmaster/tests/test_interactor.py b/lib/lp/buildmaster/tests/test_interactor.py
index 6e4a790..9aa025c 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test BuilderInteractor features."""
@@ -121,21 +121,6 @@ class TestBuilderInteractor(TestCase):
         super().setUp()
         self.addCleanup(shut_down_default_process_pool)
 
-    def test_extractBuildStatus_baseline(self):
-        # extractBuildStatus picks the name of the build status out of a
-        # dict describing the worker's status.
-        worker_status = {'build_status': 'BuildStatus.BUILDING'}
-        self.assertEqual(
-            'BUILDING', BuilderInteractor.extractBuildStatus(worker_status))
-
-    def test_extractBuildStatus_malformed(self):
-        # extractBuildStatus errors out when the status string is not
-        # of the form it expects.
-        worker_status = {'build_status': 'BUILDING'}
-        self.assertRaises(
-            AssertionError, BuilderInteractor.extractBuildStatus,
-            worker_status)
-
     def resumeWorkerHost(self, builder):
         vitals = extract_vitals_from_db(builder)
         return BuilderInteractor.resumeWorkerHost(
diff --git a/lib/lp/code/model/tests/test_recipebuilder.py b/lib/lp/code/model/tests/test_recipebuilder.py
index 9077a08..9c75d68 100644
--- a/lib/lp/code/model/tests/test_recipebuilder.py
+++ b/lib/lp/code/model/tests/test_recipebuilder.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test RecipeBuildBehaviour."""
@@ -418,7 +418,11 @@ class TestBuildNotifications(TestCaseWithFactory):
             self.queue_record, self.queue_record.builder, worker)
 
     def assertDeferredNotifyCount(self, status, behaviour, expected_count):
-        d = behaviour.handleStatus(self.queue_record, status, {'filemap': {}})
+        d = behaviour.handleStatus(
+            self.queue_record,
+            {'builder_status': 'BuilderStatus.WAITING',
+             'build_status': 'BuildStatus.%s' % status,
+             'filemap': {}})
 
         def cb(result):
             self.assertEqual(expected_count, len(pop_notifications()))
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index 7804583..cb5438f 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `OCIRecipeBuildBehaviour`."""
@@ -55,6 +55,7 @@ from lp.buildmaster.interfaces.builder import (
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
+from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.tests.builderproxy import (
     InProcessProxyAuthAPIFixture,
@@ -773,15 +774,40 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
             1, len(os.listdir(os.path.join(self.upload_root, result))))
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_normal_image(self):
+    def test_handleStatus_BUILDING(self):
+        # If the builder is BUILDING (or any status other than WAITING),
+        # then the behaviour calls updateStatus but doesn't do anything
+        # else.
+        initial_status = self.build.status
+        bq_id = self.build.buildqueue_record.id
+        worker_status = {"builder_status": "BuilderStatus.BUILDING"}
+        removeSecurityProxy(self.build).updateStatus = FakeMethod()
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, worker_status)
+        self.assertEqual(None, self.build.log)
+        self.assertEqual(0, len(os.listdir(self.upload_root)))
+        self.assertEqual(
+            [((initial_status,),
+              {"builder": self.builder, "worker_status": worker_status})],
+            removeSecurityProxy(self.build).updateStatus.calls)
+        self.assertEqual(0, len(pop_notifications()), "Notifications received")
+        self.assertEqual(
+            self.build.buildqueue_record,
+            getUtility(IBuildQueueSet).get(bq_id))
+
+    @defer.inlineCallbacks
+    def test_handleStatus_WAITING_OK_normal_image(self):
         now = datetime.now()
         mock_datetime = self.useFixture(MockPatch(
             'lp.buildmaster.model.buildfarmjobbehaviour.datetime')).mock
         mock_datetime.now = lambda: now
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': self.filemap})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': self.filemap})
         self.assertEqual(
             ['buildlog', 'manifest_hash', 'digests_hash', 'config_1_hash',
              'layer_2_hash'],
@@ -805,7 +831,7 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
             self.assertEqual(contents, b'retrieved from librarian')
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_reuse_from_other_build(self):
+    def test_handleStatus_WAITING_OK_reuse_from_other_build(self):
         """We should be able to reuse a layer file from a separate build."""
         oci_file = self.factory.makeOCIFile(
             layer_file_digest='digest_2',
@@ -820,8 +846,10 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
         mock_oci_datetime.now = lambda tzinfo=None: now
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': self.filemap})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': self.filemap})
         self.assertEqual(
             ['buildlog', 'manifest_hash', 'digests_hash', 'config_1_hash'],
             self.worker._got_file_record)
@@ -844,7 +872,7 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
         self.assertEqual(now, oci_file.date_last_used)
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_absolute_filepath(self):
+    def test_handleStatus_WAITING_OK_absolute_filepath(self):
 
         self._createTestFile(
             'manifest.json',
@@ -862,11 +890,13 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                 "'/notvalid/config_file_1.json'."):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, 'OK',
-                    {'filemap': self.filemap})
+                    self.build.buildqueue_record,
+                    {'builder_status': 'BuilderStatus.WAITING',
+                     'build_status': 'BuildStatus.OK',
+                     'filemap': self.filemap})
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_relative_filepath(self):
+    def test_handleStatus_WAITING_OK_relative_filepath(self):
 
         self._createTestFile(
             'manifest.json',
@@ -882,30 +912,36 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                 "Build returned a file named '../config_file_1.json'."):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, 'OK',
-                    {'filemap': self.filemap})
+                    self.build.buildqueue_record,
+                    {'builder_status': 'BuilderStatus.WAITING',
+                     'build_status': 'BuildStatus.OK',
+                     'filemap': self.filemap})
 
     @defer.inlineCallbacks
-    def test_handleStatus_OK_sets_build_log(self):
+    def test_handleStatus_WAITING_OK_sets_build_log(self):
         # The build log is set during handleStatus.
         self.assertEqual(None, self.build.log)
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': self.filemap})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': self.filemap})
         self.assertNotEqual(None, self.build.log)
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_cancels_cancelling(self):
+    def test_handleStatus_WAITING_ABORTED_cancels_cancelling(self):
         with dbuser(config.builddmaster.dbuser):
             self.build.updateStatus(BuildStatus.CANCELLING)
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, "ABORTED", {})
+                self.build.buildqueue_record,
+                {"builder_status": "BuilderStatus.WAITING",
+                 "build_status": "BuildStatus.ABORTED"})
         self.assertEqual(0, len(pop_notifications()), "Notifications received")
         self.assertEqual(BuildStatus.CANCELLED, self.build.status)
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_illegal_when_building(self):
+    def test_handleStatus_WAITING_ABORTED_illegal_when_building(self):
         self.builder.vm_host = "fake_vm_host"
         self.behaviour = self.interactor.getBuildBehaviour(
             self.build.buildqueue_record, self.builder, self.worker)
@@ -915,16 +951,20 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                     BuildDaemonError,
                     "Build returned unexpected status: %r" % 'ABORTED'):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "ABORTED", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.ABORTED"})
 
     @defer.inlineCallbacks
-    def test_handleStatus_ABORTED_cancelling_sets_build_log(self):
+    def test_handleStatus_WAITING_ABORTED_cancelling_sets_build_log(self):
         # If a build is intentionally cancelled, the build log is set.
         self.assertEqual(None, self.build.log)
         with dbuser(config.builddmaster.dbuser):
             self.build.updateStatus(BuildStatus.CANCELLING)
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, "ABORTED", {})
+                self.build.buildqueue_record,
+                {"builder_status": "BuilderStatus.WAITING",
+                 "build_status": "BuildStatus.ABORTED"})
         self.assertNotEqual(None, self.build.log)
 
     @defer.inlineCallbacks
@@ -933,8 +973,10 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
         self.assertEqual(None, self.build.date_finished)
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
-                self.build.buildqueue_record, 'OK',
-                {'filemap': self.filemap})
+                self.build.buildqueue_record,
+                {'builder_status': 'BuilderStatus.WAITING',
+                 'build_status': 'BuildStatus.OK',
+                 'filemap': self.filemap})
         self.assertNotEqual(None, self.build.date_finished)
 
     @defer.inlineCallbacks
@@ -944,7 +986,9 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                 "Build returned unexpected status: %r" % 'GIVENBACK'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "GIVENBACK", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.GIVENBACK"})
 
     @defer.inlineCallbacks
     def test_builderfail_collection(self):
@@ -953,7 +997,9 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                 "Build returned unexpected status: %r" % 'BUILDERFAIL'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "BUILDERFAIL", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.BUILDERFAIL"})
 
     @defer.inlineCallbacks
     def test_invalid_status_collection(self):
@@ -962,7 +1008,9 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
                 "Build returned unexpected status: %r" % 'BORKED'):
             with dbuser(config.builddmaster.dbuser):
                 yield self.behaviour.handleStatus(
-                    self.build.buildqueue_record, "BORKED", {})
+                    self.build.buildqueue_record,
+                    {"builder_status": "BuilderStatus.WAITING",
+                     "build_status": "BuildStatus.BORKED"})
 
 
 class TestGetUploadMethodsForOCIRecipeBuild(
diff --git a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
index 634ed1a..5b532e5 100644
--- a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
+++ b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2022 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for TranslationTemplatesBuildBehaviour."""
@@ -14,7 +14,6 @@ from zope.component import getUtility
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interactor import BuilderInteractor
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
@@ -164,11 +163,7 @@ class TestTranslationTemplatesBuildBehaviour(
         d = worker.status()
 
         def got_status(status):
-            return (
-                behaviour.handleStatus(
-                    queue_item, BuilderInteractor.extractBuildStatus(status),
-                    status),
-                worker.call_log)
+            return behaviour.handleStatus(queue_item, status), worker.call_log
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behaviour.build.status)
@@ -193,10 +188,7 @@ class TestTranslationTemplatesBuildBehaviour(
 
         def got_status(status):
             del status['filemap']
-            return behaviour.handleStatus(
-                queue_item,
-                BuilderInteractor.extractBuildStatus(status),
-                status),
+            return behaviour.handleStatus(queue_item, status),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FAILEDTOBUILD, behaviour.build.status)
@@ -222,10 +214,7 @@ class TestTranslationTemplatesBuildBehaviour(
 
         def got_status(status):
             del status['filemap']
-            return behaviour.handleStatus(
-                queue_item,
-                BuilderInteractor.extractBuildStatus(status),
-                status),
+            return behaviour.handleStatus(queue_item, status),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behaviour.build.status)
@@ -257,10 +246,7 @@ class TestTranslationTemplatesBuildBehaviour(
         d = worker.status()
 
         def got_status(status):
-            return behaviour.handleStatus(
-                queue_item,
-                BuilderInteractor.extractBuildStatus(status),
-                status),
+            return behaviour.handleStatus(queue_item, status),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behaviour.build.status)