launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14959
[Merge] lp:~wgrant/launchpad/handleStatus-refactor-2 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/handleStatus-refactor-2 into lp:launchpad.
Commit message:
Encapsulate most build status operations into BuildFarmJob.updateStatus.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/handleStatus-refactor-2/+merge/144203
This branch encapsulates most build status operations into BuildFarmJob.updateStatus. It handles setting status, date_started, date_finished, builder, dependencies (if required), and will soon know how to update the search schema.
All model callsites should be fixed, but there's a thousand or so lines of test changes to come later. Once those are done, all attribute write permissions on BPB/SPRB can be revoked. TTB is another matter.
I left some XXXs around regarding the setting of BFJ.builder. At present it's expected to not be set until build completion, and that will have to remain the case until BFJO is further eliminated.
--
https://code.launchpad.net/~wgrant/launchpad/handleStatus-refactor-2/+merge/144203
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/handleStatus-refactor-2 into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py 2012-10-30 01:46:10 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py 2013-01-22 01:56:22 +0000
@@ -419,10 +419,7 @@
def checkBuild(self, build):
"""See PackageUploadFile."""
# The master verifies the status to confirm successful upload.
- build.status = BuildStatus.FULLYBUILT
- # If this upload is successful, any existing log is wrong and
- # unuseful.
- build.upload_log = None
+ build.updateStatus(BuildStatus.FULLYBUILT)
# Sanity check; raise an error if the build we've been
# told to link to makes no sense.
@@ -865,7 +862,7 @@
build = sourcepackagerelease.getBuildByArch(
dar, self.policy.archive)
if build is not None:
- build.status = BuildStatus.FULLYBUILT
+ build.updateStatus(BuildStatus.FULLYBUILT)
self.logger.debug("Updating build for %s: %s" % (
dar.architecturetag, build.id))
else:
@@ -886,14 +883,7 @@
"Upload to unknown architecture %s for distroseries %s" %
(self.archtag, self.policy.distroseries))
- # Ensure gathered binary is related to a FULLYBUILT build
- # record. It will be check in slave-scanner procedure to
- # certify that the build was processed correctly.
- build.status = BuildStatus.FULLYBUILT
- # Also purge any previous failed upload_log stored, so its
- # content can be garbage-collected since it's not useful
- # anymore.
- build.upload_log = None
+ build.updateStatus(BuildStatus.FULLYBUILT)
# Sanity check; raise an error if the build we've been
# told to link to makes no sense.
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py 2012-10-24 10:59:09 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py 2013-01-22 01:56:22 +0000
@@ -681,7 +681,7 @@
result = UploadStatusEnum.FAILED
if (result != UploadStatusEnum.ACCEPTED or
not self.build.verifySuccessfulUpload()):
- self.build.status = BuildStatus.FAILEDTOUPLOAD
+ self.build.updateStatus(BuildStatus.FAILEDTOUPLOAD)
if self.build.status != BuildStatus.FULLYBUILT:
if recipe_deleted:
# For a deleted recipe, no need to notify that uploading has
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py 2013-01-07 07:53:54 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2013-01-22 01:56:22 +0000
@@ -285,6 +285,19 @@
services job directly on the BuildFarmJob.
"""
+ def setLog(log):
+ """Set the `LibraryFileAlias` that contains the job log."""
+
+ def updateStatus(status, builder=None, slave_status=None,
+ date_started=None, date_finished=None):
+ """Update job metadata when the build status changes.
+
+ This automatically handles setting status, date_finished, builder,
+ dependencies. Later it will manage the denormalised search schema.
+
+ date_started and date_finished override the default (now).
+ """
+
def gotFailure():
"""Increment the failure_count for this job."""
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py 2012-01-11 08:52:22 +0000
+++ lib/lp/buildmaster/manager.py 2013-01-22 01:56:22 +0000
@@ -92,7 +92,7 @@
# on the build record so don't worry about that here.
builder.resetFailureCount()
build_job = current_job.specific_job.build
- build_job.status = BuildStatus.FAILEDTOBUILD
+ build_job.updateStatus(BuildStatus.FAILEDTOBUILD)
builder.currentjob.destroySelf()
# N.B. We could try and call _handleStatus_PACKAGEFAIL here
=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py 2013-01-07 07:53:54 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py 2013-01-22 01:56:22 +0000
@@ -10,6 +10,7 @@
'BuildFarmJobOldDerived',
]
+import datetime
import hashlib
from lazr.delegates import delegates
@@ -324,6 +325,44 @@
BuildStatus.UPLOADING,
BuildStatus.SUPERSEDED]
+ def setLog(self, log):
+ """See `IBuildFarmJob`."""
+ assert self.log is None
+ self.log = log
+
+ def updateStatus(self, status, builder=None, slave_status=None,
+ date_started=None, date_finished=None):
+ """See `IBuildFarmJob`."""
+ self.status = status
+
+ # If there's a builder provided, set it if we don't already have
+ # one, or otherwise crash if it's different from the one we
+ # expected.
+ if builder is not None:
+ if self.builder is None:
+ self.builder = builder
+ else:
+ assert self.builder == builder
+
+ # If we're starting to build, set date_started and
+ # date_first_dispatched if required.
+ if self.date_started is None and status == BuildStatus.BUILDING:
+ self.date_started = date_started or datetime.datetime.now(pytz.UTC)
+ if self.date_first_dispatched is None:
+ self.date_first_dispatched = self.date_started
+
+ # If we're in a final build state (or UPLOADING, which sort of
+ # is), set date_finished if date_started is.
+ if (self.date_started is not None and self.date_finished is None
+ and status not in (
+ BuildStatus.NEEDSBUILD, BuildStatus.BUILDING,
+ BuildStatus.CANCELLING)):
+ # XXX cprov 20060615 bug=120584: Currently buildduration includes
+ # the scanner latency, it should really be asking the slave for
+ # the duration spent building locally.
+ self.date_finished = (
+ date_finished or datetime.datetime.now(pytz.UTC))
+
def gotFailure(self):
"""See `IBuildFarmJob`."""
self.failure_count += 1
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-01-21 04:16:09 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-01-22 01:56:22 +0000
@@ -98,28 +98,12 @@
build.is_private)
return d
- @classmethod
@defer.inlineCallbacks
- def storeBuildInfo(cls, build, status, librarian, slave_status):
- """See `IPackageBuild`."""
- if status is not None:
- build.status = status
-
- lfa_id = yield cls.getLogFromSlave(build, build.buildqueue_record)
-
- # log, builder and date_finished are read-only, so we must
- # currently remove the security proxy to set them.
- naked_build = removeSecurityProxy(build)
- naked_build.log = lfa_id
- naked_build.builder = build.buildqueue_record.builder
- # XXX cprov 20060615 bug=120584: Currently buildduration includes
- # the scanner latency, it should really be asking the slave for
- # the duration spent building locally.
- naked_build.date_finished = datetime.datetime.now(pytz.UTC)
- if slave_status.get('dependencies') is not None:
- build.dependencies = unicode(slave_status.get('dependencies'))
- else:
- build.dependencies = None
+ def storeLogFromSlave(self):
+ """See `IBuildFarmJob`."""
+ lfa_id = yield self.getLogFromSlave(
+ self.build, self.build.buildqueue_record)
+ self.build.setLog(lfa_id)
def updateBuild(self, queueItem):
"""See `IBuildFarmJobBehavior`."""
@@ -286,7 +270,7 @@
if build.build_farm_job_type == BuildFarmJobType.PACKAGEBUILD:
build = build.buildqueue_record.specific_job.build
if not build.current_source_publication:
- build.status = BuildStatus.SUPERSEDED
+ build.updateStatus(BuildStatus.SUPERSEDED)
yield self.build.buildqueue_record.builder.cleanSlave()
self.build.buildqueue_record.destroySelf()
return
@@ -336,7 +320,12 @@
status = (
BuildStatus.UPLOADING if successful_copy_from_slave
else BuildStatus.FAILEDTOUPLOAD)
- yield self.storeBuildInfo(build, status, librarian, slave_status)
+ # XXX wgrant: The builder should be set long before here, but
+ # currently isn't.
+ build.updateStatus(
+ status, builder=build.buildqueue_record.builder,
+ slave_status=slave_status)
+ yield self.storeLogFromSlave()
# We only attempt the upload if we successfully copied all the
# files from the slave.
@@ -376,7 +365,12 @@
The build, not the builder, has failed. Set its status, store
available information, and remove the queue entry.
"""
- yield self.storeBuildInfo(self.build, status, librarian, slave_status)
+ # XXX wgrant: The builder should be set long before here, but
+ # currently isn't.
+ self.build.updateStatus(
+ status, builder=self.build.buildqueue_record.builder,
+ slave_status=slave_status)
+ yield self.storeLogFromSlave()
if send_notification:
self.build.notify()
yield self.build.buildqueue_record.builder.cleanSlave()
@@ -403,7 +397,6 @@
BuildStatus.CHROOTWAIT, librarian, slave_status, logger,
send_notification)
- @defer.inlineCallbacks
def _handleStatus_BUILDERFAIL(self, librarian, slave_status, logger,
send_notification):
"""Handle builder failures.
@@ -412,7 +405,6 @@
"""
self.build.buildqueue_record.builder.failBuilder(
"Builder returned BUILDERFAIL when asked for its status")
- yield self.storeBuildInfo(self.build, None, librarian, slave_status)
self.build.buildqueue_record.reset()
@defer.inlineCallbacks
@@ -424,7 +416,6 @@
later, the build records is delayed by reducing the lastscore to
ZERO.
"""
- yield self.storeBuildInfo(self.build, None, librarian, slave_status)
yield self.build.buildqueue_record.builder.cleanSlave()
self.build.buildqueue_record.reset()
=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py 2013-01-10 07:19:48 +0000
+++ lib/lp/buildmaster/model/packagebuild.py 2013-01-22 01:56:22 +0000
@@ -149,6 +149,18 @@
"""See `IPackageBuild`."""
raise NotImplementedError
+ def updateStatus(self, status, builder=None, slave_status=None,
+ date_started=None, date_finished=None):
+ super(PackageBuildMixin, self).updateStatus(
+ status, builder=builder, slave_status=slave_status,
+ date_started=date_started, date_finished=date_finished)
+
+ if (status == BuildStatus.MANUALDEPWAIT and slave_status is not None
+ and slave_status.get('dependencies') is not None):
+ self.dependencies = unicode(slave_status.get('dependencies'))
+ else:
+ self.dependencies = None
+
def verifySuccessfulUpload(self):
"""See `IPackageBuild`."""
raise NotImplementedError
=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjob.py'
--- lib/lp/buildmaster/tests/test_buildfarmjob.py 2013-01-07 07:53:54 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjob.py 2013-01-22 01:56:22 +0000
@@ -12,6 +12,7 @@
import pytz
from storm.store import Store
+from testtools.matchers import GreaterThan
from zope.component import getUtility
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
@@ -29,6 +30,7 @@
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.services.database.sqlbase import flush_database_updates
from lp.testing import (
+ admin_logged_in,
login,
TestCaseWithFactory,
)
@@ -165,6 +167,71 @@
self.failUnlessEqual(
BuildStatus.FULLYBUILT, self.build_farm_job.status)
+ def test_updateStatus_sets_status(self):
+ # updateStatus always sets status.
+ self.assertEqual(BuildStatus.NEEDSBUILD, self.build_farm_job.status)
+ self.build_farm_job.updateStatus(BuildStatus.FULLYBUILT)
+ self.assertEqual(BuildStatus.FULLYBUILT, self.build_farm_job.status)
+
+ def test_updateStatus_sets_builder(self):
+ # updateStatus sets builder if it's passed.
+ builder = self.factory.makeBuilder()
+ self.assertIs(None, self.build_farm_job.builder)
+ self.build_farm_job.updateStatus(
+ BuildStatus.FULLYBUILT, builder=builder)
+ self.assertEqual(builder, self.build_farm_job.builder)
+
+ def test_updateStatus_BUILDING_sets_date_started(self):
+ # updateStatus sets date_started on transition to BUILDING.
+ # date_first_dispatched is also set if it isn't already.
+ self.assertEqual(BuildStatus.NEEDSBUILD, self.build_farm_job.status)
+ self.assertIs(None, self.build_farm_job.date_started)
+ self.assertIs(None, self.build_farm_job.date_first_dispatched)
+
+ self.build_farm_job.updateStatus(BuildStatus.CANCELLED)
+ self.assertIs(None, self.build_farm_job.date_started)
+ self.assertIs(None, self.build_farm_job.date_first_dispatched)
+
+ # Setting it to BUILDING for the first time sets date_started
+ # and date_first_dispatched.
+ self.build_farm_job.updateStatus(BuildStatus.BUILDING)
+ self.assertIsNot(None, self.build_farm_job.date_started)
+ first = self.build_farm_job.date_started
+ self.assertEqual(first, self.build_farm_job.date_first_dispatched)
+
+ self.build_farm_job.updateStatus(BuildStatus.FAILEDTOBUILD)
+ with admin_logged_in():
+ self.build_farm_job.retry()
+ self.assertIs(None, self.build_farm_job.date_started)
+ self.assertEqual(first, self.build_farm_job.date_first_dispatched)
+
+ # But BUILDING a second time doesn't change
+ # date_first_dispatched.
+ self.build_farm_job.updateStatus(BuildStatus.BUILDING)
+ self.assertThat(self.build_farm_job.date_started, GreaterThan(first))
+ self.assertEqual(first, self.build_farm_job.date_first_dispatched)
+
+ def test_updateStatus_sets_date_finished(self):
+ # updateStatus sets date_finished if it's a final state and
+ # date_started is set.
+ # UPLOADING counts as the end of the job. date_finished doesn't
+ # include the upload time.
+ for status in (
+ BuildStatus.FULLYBUILT, BuildStatus.FAILEDTOBUILD,
+ BuildStatus.CHROOTWAIT, BuildStatus.MANUALDEPWAIT,
+ BuildStatus.UPLOADING, BuildStatus.FAILEDTOUPLOAD,
+ BuildStatus.CANCELLED, BuildStatus.SUPERSEDED):
+ build = self.factory.makeBinaryPackageBuild()
+ build.updateStatus(status)
+ self.assertIs(None, build.date_started)
+ self.assertIs(None, build.date_finished)
+ build.updateStatus(BuildStatus.BUILDING)
+ self.assertIsNot(None, build.date_started)
+ self.assertIs(None, build.date_finished)
+ build.updateStatus(status)
+ self.assertIsNot(None, build.date_started)
+ self.assertIsNot(None, build.date_finished)
+
class TestBuildFarmJobSet(TestBuildFarmJobBase, TestCaseWithFactory):
=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-01-21 02:51:25 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-01-22 01:56:22 +0000
@@ -377,5 +377,6 @@
def test_sets_date_finished(self):
# storeBuildInfo should set date_finished on the BuildFarmJob.
self.assertIs(None, self.build.date_finished)
- self.behavior.storeBuildInfo(self.build, None, None, {})
+ self.behavior.storeBuildInfo(
+ self.build, BuildStatus.FULLYBUILT, None, {})
self.assertIsNot(None, self.build.date_finished)
=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py 2013-01-10 05:12:54 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py 2013-01-22 01:56:22 +0000
@@ -111,6 +111,24 @@
# PackageBuild provides IPackageBuild
self.assertProvides(self.package_build, IPackageBuild)
+ def test_updateStatus_MANUALDEPWAIT_sets_dependencies(self):
+ # updateStatus sets dependencies for a MANUALDEPWAIT build.
+ self.package_build.updateStatus(
+ BuildStatus.MANUALDEPWAIT, slave_status={'dependencies': u'deps'})
+ self.assertEqual(u'deps', self.package_build.dependencies)
+ self.package_build.updateStatus(
+ BuildStatus.MANUALDEPWAIT, slave_status={})
+ self.assertEqual(None, self.package_build.dependencies)
+
+ def test_updateStatus_unsets_dependencies_for_other_statuses(self):
+ # updateStatus unsets existing dependencies when transitioning
+ # to another state.
+ self.package_build.updateStatus(
+ BuildStatus.MANUALDEPWAIT, slave_status={'dependencies': u'deps'})
+ self.assertEqual(u'deps', self.package_build.dependencies)
+ self.package_build.updateStatus(BuildStatus.FULLYBUILT)
+ self.assertEqual(None, self.package_build.dependencies)
+
def test_log_url(self):
# The url of the build log file is determined by the PackageBuild.
lfa = self.factory.makeLibraryFileAlias('mybuildlog.txt')
=== modified file 'lib/lp/soyuz/model/buildfarmbuildjob.py'
--- lib/lp/soyuz/model/buildfarmbuildjob.py 2011-12-30 06:14:56 +0000
+++ lib/lp/soyuz/model/buildfarmbuildjob.py 2013-01-22 01:56:22 +0000
@@ -11,7 +11,6 @@
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.model.buildfarmjob import BuildFarmJobOld
-from lp.services.database.constants import UTC_NOW
from lp.soyuz.interfaces.buildfarmbuildjob import IBuildFarmBuildJob
@@ -35,12 +34,9 @@
return self.build.title
def jobStarted(self):
- """See `IBuildFarmJob`."""
- self.build.status = BuildStatus.BUILDING
- # The build started, set the start time if not set already.
- self.build.date_started = UTC_NOW
- if self.build.date_first_dispatched is None:
- self.build.date_first_dispatched = UTC_NOW
+ """See `IBuildFarmJobOld`."""
+ # XXX wgrant: builder should be set here.
+ self.build.updateStatus(BuildStatus.BUILDING)
def jobReset(self):
"""See `IBuildFarmJob`."""
=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py 2012-10-02 04:39:27 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py 2013-01-22 01:56:22 +0000
@@ -249,7 +249,7 @@
logger.debug(
"Build %s FAILEDTOBUILD, queue item %s REMOVED"
% (build.id, job.id))
- build.status = BuildStatus.FAILEDTOBUILD
+ build.updateStatus(BuildStatus.FAILEDTOBUILD)
job.destroySelf()
return False
@@ -260,7 +260,7 @@
logger.debug(
"Build %s SUPERSEDED, queue item %s REMOVED"
% (build.id, job.id))
- build.status = BuildStatus.SUPERSEDED
+ build.updateStatus(BuildStatus.SUPERSEDED)
job.destroySelf()
return False
=== modified file 'lib/lp/soyuz/tests/test_buildpackagejob.py'
--- lib/lp/soyuz/tests/test_buildpackagejob.py 2013-01-03 00:27:37 +0000
+++ lib/lp/soyuz/tests/test_buildpackagejob.py 2013-01-22 01:56:22 +0000
@@ -237,8 +237,11 @@
build, bq = find_job(self, 'gcc', '386')
build_package_job = bq.specific_job
build_package_job.jobStarted()
- self.failUnlessEqual(
+ self.assertEqual(
BuildStatus.BUILDING, build_package_job.build.status)
+ self.assertIsNot(None, build_package_job.build.date_started)
+ self.assertIsNot(None, build_package_job.build.date_first_dispatched)
+ self.assertIs(None, build_package_job.build.date_finished)
class TestBuildPackageJobScore(TestCaseWithFactory):
