Colin Watson has proposed merging ~cjwatson/launchpad:refactor-updateBuild into launchpad:master with ~cjwatson/launchpad:rename-builder-interactor-slave as a prerequisite.
Commit message: Push more of BuilderInteractor.updateBuild down to behaviours Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414070 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. -- Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:refactor-updateBuild into launchpad:master.
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 0e446ec..93f9325 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)
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp