launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15913
[Merge] lp:~wgrant/launchpad/bfjb-no-interactor into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bfjb-no-interactor into lp:launchpad.
Commit message:
Stop BuildFarmJobBehaviors from using a BuilderInteractor.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bfjb-no-interactor/+merge/186477
Stop BuildFarmJobBehaviors from using a BuilderInteractor. BuilderInteractor stores more state than we can afford, and it's easy to avoid, so it will soon die.
As part of this I adjusted unexpected ABORTED handling to mark the failure against both the build and the builder, then clean the slave. This case is probably usually a build fault, but previously the failure was only recorded against the builder, so failure counting would never kill the build. And I needed to move BFJB away from BI.
--
https://code.launchpad.net/~wgrant/launchpad/bfjb-no-interactor/+merge/186477
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bfjb-no-interactor into lp:launchpad.
=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py 2013-09-17 04:31:16 +0000
+++ lib/lp/buildmaster/interactor.py 2013-09-19 07:47:32 +0000
@@ -278,7 +278,7 @@
elif currentjob != self._cached_currentjob:
self._cached_build_behavior = IBuildFarmJobBehavior(
currentjob.specific_job)
- self._cached_build_behavior.setBuilderInteractor(self)
+ self._cached_build_behavior.setBuilder(self.builder, self.slave)
self._cached_currentjob = currentjob
return self._cached_build_behavior
@@ -363,35 +363,15 @@
# will rescue the DB side instead), and we just have to wait
# out an ABORTING one.
if status == 'BuilderStatus.WAITING':
- yield self.cleanSlave()
+ yield self.slave.clean()
elif status == 'BuilderStatus.BUILDING':
- yield self.requestAbort()
+ yield self.slave.abort()
if logger:
logger.info(
"Builder '%s' rescued from '%s': '%s'" %
(self.vitals.name, slave_cookie, reason))
defer.returnValue(True)
- def cleanSlave(self):
- """Clean any temporary files from the slave.
-
- :return: A Deferred that fires when the dialog with the slave is
- finished. It does not have a return value.
- """
- return self.slave.clean()
-
- def requestAbort(self):
- """Ask that a build be aborted.
-
- This takes place asynchronously: Actually killing everything running
- can take some time so the slave status should be queried again to
- detect when the abort has taken effect. (Look for status ABORTED).
-
- :return: A Deferred that fires when the dialog with the slave is
- finished. It does not have a return value.
- """
- return self.slave.abort()
-
def resumeSlaveHost(self):
"""Resume the slave host to a known good condition.
@@ -567,7 +547,8 @@
queueItem.logtail = "Waiting for slave process to be terminated"
transaction.commit()
- def extractBuildStatus(self, status_dict):
+ @staticmethod
+ def extractBuildStatus(status_dict):
"""Read build status name.
:param status_dict: build status dict as passed to the
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2013-09-02 08:11:58 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2013-09-19 07:47:32 +0000
@@ -14,8 +14,8 @@
class IBuildFarmJobBehavior(Interface):
- def setBuilderInteractor(interactor):
- """Sets the associated `BuilderInteractor` for this instance."""
+ def setBuilder(builder, slave):
+ """Sets the associated builder and slave for this instance."""
def logStartBuild(logger):
"""Log the start of a specific build queue item.
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py 2013-09-17 04:31:16 +0000
+++ lib/lp/buildmaster/manager.py 2013-09-19 07:47:32 +0000
@@ -236,7 +236,7 @@
try:
if self.date_cancel is None:
self.logger.info("Cancelling build '%s'" % build.title)
- yield interactor.requestAbort()
+ yield interactor.slave.abort()
self.date_cancel = self._clock.seconds() + self.CANCEL_TIMEOUT
defer.returnValue(False)
else:
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-09-02 06:31:24 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-09-19 07:47:32 +0000
@@ -24,7 +24,6 @@
BuildFarmJobType,
BuildStatus,
)
-from lp.buildmaster.interfaces.builder import BuildSlaveFailure
from lp.services.config import config
from lp.services.helpers import filenameToContentType
from lp.services.librarian.interfaces import ILibraryFileAliasSet
@@ -49,10 +48,10 @@
def build(self):
return self.buildfarmjob.build
- def setBuilderInteractor(self, interactor):
+ def setBuilder(self, builder, slave):
"""The builder should be set once and not changed."""
- self._interactor = interactor
- self._builder = interactor.builder
+ self._builder = builder
+ self._slave = slave
def verifyBuildRequest(self, logger):
"""The default behavior is a no-op."""
@@ -119,7 +118,7 @@
return library_file.id
- d = self._interactor.slave.getFile(file_sha1, out_file)
+ d = self._slave.getFile(file_sha1, out_file)
d.addCallback(got_file, filename, out_file, out_file_name)
return d
@@ -185,7 +184,7 @@
if build.job_type == BuildFarmJobType.PACKAGEBUILD:
build = build.buildqueue_record.specific_job.build
if not build.current_source_publication:
- yield self._interactor.cleanSlave()
+ yield self._slave.clean()
build.updateStatus(BuildStatus.SUPERSEDED)
self.build.buildqueue_record.destroySelf()
return
@@ -229,7 +228,7 @@
"for the build %d." % (filename, build.id))
break
filenames_to_download[filemap[filename]] = out_file_name
- yield self._interactor.slave.getFiles(filenames_to_download)
+ yield self._slave.getFiles(filenames_to_download)
status = (
BuildStatus.UPLOADING if successful_copy_from_slave
@@ -261,7 +260,7 @@
if not os.path.exists(target_dir):
os.mkdir(target_dir)
- yield self._interactor.cleanSlave()
+ yield self._slave.clean()
self.build.buildqueue_record.destroySelf()
transaction.commit()
@@ -286,7 +285,7 @@
yield self.storeLogFromSlave()
if notify:
self.build.notify()
- yield self._interactor.cleanSlave()
+ yield self._slave.clean()
self.build.buildqueue_record.destroySelf()
transaction.commit()
@@ -320,24 +319,17 @@
"""Handle aborted builds.
If the build was explicitly cancelled, then mark it as such.
- Otherwise, the build has failed in some unexpected way.
+ Otherwise, the build has failed in some unexpected way; we'll
+ reset it it and clean up the slave.
"""
if self.build.status == BuildStatus.CANCELLING:
yield self.storeLogFromSlave()
self.build.buildqueue_record.cancel()
- transaction.commit()
- yield self._interactor.cleanSlave()
else:
+ self._builder.handleFailure(logger)
self.build.buildqueue_record.reset()
- try:
- self._builder.handleFailure(logger)
- yield self._interactor.resetOrFail(
- logger,
- BuildSlaveFailure("Builder unexpectedly returned ABORTED"))
- except Exception as e:
- # We've already done our best to mark the builder as failed.
- logger.error("Failed to rescue from ABORTED: %s" % e)
transaction.commit()
+ yield self._slave.clean()
@defer.inlineCallbacks
def _handleStatus_GIVENBACK(self, slave_status, logger, notify):
@@ -347,6 +339,6 @@
later, the build records is delayed by reducing the lastscore to
ZERO.
"""
- yield self._interactor.cleanSlave()
+ yield self._slave.clean()
self.build.buildqueue_record.reset()
transaction.commit()
=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-09-17 05:09:39 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-09-19 07:47:32 +0000
@@ -263,7 +263,8 @@
0, len(pop_notifications()), "Notifications received")
self.assertEqual(BuildStatus.NEEDSBUILD, self.build.status)
self.assertEqual(1, self.builder.failure_count)
- self.assertIn("resume", self.slave.call_log)
+ self.assertEqual(1, self.build.failure_count)
+ self.assertIn("clean", self.slave.call_log)
d = self.behavior.handleStatus(
self.build.buildqueue_record, "ABORTED", {})
=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py 2013-09-12 08:15:43 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py 2013-09-19 07:47:32 +0000
@@ -306,7 +306,7 @@
behavior = interactor._current_build_behavior
self.assertIsInstance(behavior, BinaryPackageBuildBehavior)
self.assertEqual(behavior._builder, builder)
- self.assertEqual(behavior._interactor, interactor)
+ self.assertEqual(behavior._slave, interactor.slave)
def _setupBuilder(self):
processor = self.factory.makeProcessor(name="i386")
=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py 2013-09-02 08:11:58 +0000
+++ lib/lp/code/model/recipebuilder.py 2013-09-19 07:47:32 +0000
@@ -135,7 +135,7 @@
distroarchseries.displayname)
logger.info(
"Sending chroot file for recipe build to %s" % self._builder.name)
- d = self._interactor.slave.cacheFile(logger, chroot)
+ d = self._slave.cacheFile(logger, chroot)
def got_cache_file(ignored):
# Generate a string which can be used to cross-check when
@@ -147,7 +147,7 @@
logger.info(
"Initiating build %s on %s" % (buildid, self._builder.url))
- return self._interactor.slave.build(
+ return self._slave.build(
cookie, "sourcepackagerecipe", chroot_sha1, {}, args)
def log_build_result((status, info)):
=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py 2013-09-02 06:31:24 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py 2013-09-19 07:47:32 +0000
@@ -124,7 +124,7 @@
# valid builder set.
job = self.makeJob()
builder = MockBuilder("bob-de-bouwer")
- job.setBuilderInteractor(BuilderInteractor(builder, OkSlave()))
+ job.setBuilder(builder, OkSlave())
logger = BufferLogger()
job.verifyBuildRequest(logger)
self.assertEquals("", logger.getLogBuffer())
@@ -134,7 +134,7 @@
job = self.makeJob()
builder = MockBuilder('non-virtual builder')
builder.virtualized = False
- job.setBuilderInteractor(BuilderInteractor(builder, OkSlave()))
+ job.setBuilder(builder, OkSlave())
logger = BufferLogger()
e = self.assertRaises(AssertionError, job.verifyBuildRequest, logger)
self.assertEqual(
@@ -146,8 +146,7 @@
pocket=PackagePublishingPocket.SECURITY)
job = self.factory.makeSourcePackageRecipeBuildJob(recipe_build=build)
job = IBuildFarmJobBehavior(job.specific_job)
- job.setBuilderInteractor(
- BuilderInteractor(MockBuilder("bob-de-bouwer"), OkSlave()))
+ job.setBuilder(MockBuilder("bob-de-bouwer"), OkSlave())
e = self.assertRaises(
AssertionError, job.verifyBuildRequest, BufferLogger())
self.assertIn('invalid pocket due to the series status of', str(e))
@@ -305,7 +304,7 @@
builder = MockBuilder("bob-de-bouwer")
processorfamily = ProcessorFamilySet().getByProcessorName('386')
builder.processor = processorfamily.processors[0]
- job.setBuilderInteractor(BuilderInteractor(builder, slave))
+ job.setBuilder(builder, slave)
logger = BufferLogger()
d = defer.maybeDeferred(job.dispatchBuildToSlave, "someid", logger)
@@ -336,7 +335,7 @@
builder = MockBuilder("bob-de-bouwer")
processorfamily = ProcessorFamilySet().getByProcessorName('386')
builder.processor = processorfamily.processors[0]
- job.setBuilderInteractor(BuilderInteractor(builder, OkSlave()))
+ job.setBuilder(builder, OkSlave())
logger = BufferLogger()
d = defer.maybeDeferred(job.dispatchBuildToSlave, "someid", logger)
return assert_fails_with(d, CannotBuild)
=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehavior.py'
--- lib/lp/soyuz/model/binarypackagebuildbehavior.py 2013-09-02 08:11:58 +0000
+++ lib/lp/soyuz/model/binarypackagebuildbehavior.py 2013-09-19 07:47:32 +0000
@@ -76,7 +76,7 @@
filemap[lfa.filename] = lfa.content.sha1
if not private:
dl.append(
- self._interactor.slave.cacheFile(
+ self._slave.cacheFile(
logger, source_file.libraryfile))
d = defer.gatherResults(dl)
return d.addCallback(lambda ignored: filemap)
@@ -87,7 +87,7 @@
# Start the binary package build on the slave builder. First
# we send the chroot.
chroot = self.build.distro_arch_series.getChroot()
- d = self._interactor.slave.cacheFile(logger, chroot)
+ d = self._slave.cacheFile(logger, chroot)
d.addCallback(self._buildFilemapStructure, logger)
def got_filemap(filemap):
@@ -101,7 +101,7 @@
"Initiating build %s on %s" % (buildid, self._builder.url))
args = self._extraBuildArgs(self.build)
- d = self._interactor.slave.build(
+ d = self._slave.build(
cookie, "binarypackage", chroot_sha1, filemap, args)
def got_build((status, info)):
@@ -214,7 +214,7 @@
"(%s, %s)" % (
self._builder.url, file_name, url, sha1))
dl.append(
- self._interactor.slave.sendFileToSlave(
+ self._slave.sendFileToSlave(
sha1, url, "buildd", archive.buildd_secret))
return dl
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py 2013-09-12 08:24:36 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py 2013-09-19 07:47:32 +0000
@@ -222,7 +222,7 @@
build.distro_arch_series.addOrUpdateChroot(lf)
candidate = build.queueBuild()
behavior = IBuildFarmJobBehavior(candidate.specific_job)
- behavior.setBuilderInteractor(BuilderInteractor(builder))
+ behavior.setBuilder(builder, None)
e = self.assertRaises(
AssertionError, behavior.verifyBuildRequest, BufferLogger())
expected_message = (
@@ -243,7 +243,7 @@
build.distro_arch_series.addOrUpdateChroot(lf)
candidate = build.queueBuild()
behavior = IBuildFarmJobBehavior(candidate.specific_job)
- behavior.setBuilderInteractor(BuilderInteractor(builder))
+ behavior.setBuilder(builder, None)
e = self.assertRaises(
AssertionError, behavior.verifyBuildRequest, BufferLogger())
self.assertEqual(
@@ -262,7 +262,7 @@
build.distro_arch_series.addOrUpdateChroot(lf)
candidate = build.queueBuild()
behavior = IBuildFarmJobBehavior(candidate.specific_job)
- behavior.setBuilderInteractor(BuilderInteractor(builder))
+ behavior.setBuilder(builder, None)
e = self.assertRaises(
AssertionError, behavior.verifyBuildRequest, BufferLogger())
self.assertEqual(
@@ -277,7 +277,7 @@
builder=builder, archive=archive)
candidate = build.queueBuild()
behavior = IBuildFarmJobBehavior(candidate.specific_job)
- behavior.setBuilderInteractor(BuilderInteractor(builder))
+ behavior.setBuilder(builder, None)
e = self.assertRaises(
CannotBuild, behavior.verifyBuildRequest, BufferLogger())
self.assertIn("Missing CHROOT", str(e))
@@ -327,8 +327,6 @@
self.build.distro_arch_series.addOrUpdateChroot(lf)
self.candidate = self.build.queueBuild()
self.candidate.markAsBuilding(self.builder)
- self.behavior = IBuildFarmJobBehavior(self.candidate.specific_job)
- self.behavior.setBuilderInteractor(BuilderInteractor(self.builder))
# This is required so that uploaded files from the buildd don't
# hang around between test runs.
self.addCleanup(self._cleanup)
@@ -475,11 +473,11 @@
return d.addCallback(got_update)
def test_log_file_collection(self):
- self.patch(BuilderSlave, 'makeBuilderSlave',
- FakeMethod(WaitingSlave('BuildStatus.OK')))
self.build.updateStatus(BuildStatus.FULLYBUILT)
old_tmps = sorted(os.listdir('/tmp'))
+ slave = WaitingSlave('BuildStatus.OK')
+
def got_log(logfile_lfa_id):
# Grabbing logs should not leave new files in /tmp (bug #172798)
logfile_lfa = getUtility(ILibraryFileAliasSet)[logfile_lfa_id]
@@ -514,12 +512,13 @@
orig_file_content = open(tmp_orig_file_name).read()
self.assertEqual(orig_file_content, uncompressed_file)
- d = removeSecurityProxy(self.interactor.slave).getFile(
+ d = removeSecurityProxy(slave).getFile(
'buildlog', tmp_orig_file_name)
return d.addCallback(got_orig_log)
- d = removeSecurityProxy(self.behavior).getLogFromSlave(
- self.build.buildqueue_record)
+ behavior = IBuildFarmJobBehavior(self.candidate.specific_job)
+ behavior.setBuilder(self.builder, slave)
+ d = behavior.getLogFromSlave(self.build.buildqueue_record)
return d.addCallback(got_log)
def test_private_build_log_storage(self):
=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py 2013-09-02 08:11:58 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py 2013-09-19 07:47:32 +0000
@@ -61,7 +61,7 @@
raise CannotBuild("Unable to find a chroot for %s" %
distroarchseries.displayname)
chroot_sha1 = chroot.content.sha1
- d = self._interactor.slave.cacheFile(logger, chroot)
+ d = self._slave.cacheFile(logger, chroot)
def got_cache_file(ignored):
args = {
@@ -71,7 +71,7 @@
filemap = {}
- return self._interactor.slave.build(
+ return self._slave.build(
self.getBuildCookie(), self.build_type, chroot_sha1, filemap,
args)
return d.addCallback(got_cache_file)
@@ -101,11 +101,9 @@
logger.error("Did not find templates tarball in slave output.")
return defer.succeed(None)
- slave = self._interactor.slave
-
fd, fname = tempfile.mkstemp()
tarball_file = os.fdopen(fd, 'wb')
- d = slave.getFile(slave_filename, tarball_file)
+ d = self._slave.getFile(slave_filename, tarball_file)
# getFile will close the file object.
return d.addCallback(lambda ignored: fname)
@@ -182,6 +180,6 @@
yield self.storeLogFromSlave(build_queue=queue_item)
- yield self._interactor.cleanSlave()
+ yield self._slave.clean()
queue_item.destroySelf()
transaction.commit()
=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2013-09-02 08:11:58 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2013-09-19 07:47:32 +0000
@@ -69,8 +69,7 @@
branch=branch)
behavior = IBuildFarmJobBehavior(specific_job)
slave = WaitingSlave()
- behavior.setBuilderInteractor(
- BuilderInteractor(self.factory.makeBuilder(), slave))
+ behavior.setBuilder(self.factory.makeBuilder(), slave)
if use_fake_chroot:
lf = self.factory.makeLibraryFileAlias()
self.layer.txn.commit()
@@ -132,7 +131,7 @@
def got_dispatch((status, info)):
# call_log lives on the mock WaitingSlave and tells us what
# calls to the slave that the behaviour class made.
- call_log = behavior._interactor.slave.call_log
+ call_log = behavior._slave.call_log
build_params = call_log[-1]
self.assertEqual('build', build_params[0])
build_type = build_params[2]
@@ -167,7 +166,7 @@
buildqueue = FakeBuildQueue(behavior)
path = behavior.templates_tarball_path
# Poke the file we're expecting into the mock slave.
- behavior._interactor.slave.valid_file_hashes.append(path)
+ behavior._slave.valid_file_hashes.append(path)
def got_tarball(filename):
tarball = open(filename, 'r')
@@ -186,7 +185,7 @@
behavior = self.makeBehavior()
behavior._uploadTarball = FakeMethod()
queue_item = FakeBuildQueue(behavior)
- slave = behavior._interactor.slave
+ slave = behavior._slave
d = behavior.dispatchBuildToSlave(queue_item, logging)
@@ -208,7 +207,7 @@
return (
behavior.handleStatus(
queue_item,
- behavior._interactor.extractBuildStatus(slave_status),
+ BuilderInteractor.extractBuildStatus(slave_status),
slave_status),
slave_call_log)
@@ -231,7 +230,7 @@
behavior = self.makeBehavior()
behavior._uploadTarball = FakeMethod()
queue_item = FakeBuildQueue(behavior)
- slave = behavior._interactor.slave
+ slave = behavior._slave
d = behavior.dispatchBuildToSlave(queue_item, logging)
def got_dispatch((status, info)):
@@ -252,7 +251,7 @@
self.assertNotIn('filemap', slave_status)
return behavior.handleStatus(
queue_item,
- behavior._interactor.extractBuildStatus(slave_status),
+ BuilderInteractor.extractBuildStatus(slave_status),
slave_status),
def build_updated(ignored):
@@ -274,7 +273,7 @@
behavior = self.makeBehavior()
behavior._uploadTarball = FakeMethod()
queue_item = FakeBuildQueue(behavior)
- slave = behavior._interactor.slave
+ slave = behavior._slave
d = behavior.dispatchBuildToSlave(queue_item, logging)
def got_dispatch((status, info)):
@@ -294,7 +293,7 @@
self.assertFalse('filemap' in slave_status)
return behavior.handleStatus(
queue_item,
- behavior._interactor.extractBuildStatus(slave_status),
+ BuilderInteractor.extractBuildStatus(slave_status),
slave_status),
def build_updated(ignored):
@@ -313,7 +312,7 @@
branch = productseries.branch
behavior = self.makeBehavior(branch=branch)
queue_item = FakeBuildQueue(behavior)
- slave = behavior._interactor.slave
+ slave = behavior._slave
d = behavior.dispatchBuildToSlave(queue_item, logging)
@@ -338,7 +337,7 @@
behavior.updateSlaveStatus(status, slave_status)
return behavior.handleStatus(
queue_item,
- behavior._interactor.extractBuildStatus(slave_status),
+ BuilderInteractor.extractBuildStatus(slave_status),
slave_status),
def build_updated(ignored):
Follow ups
