launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15885
[Merge] lp:~wgrant/launchpad/bug-1221002 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-1221002 into lp:launchpad.
Commit message:
Rework SlaveScanner.scan() to make a bit more sense.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1221002 in Launchpad itself: "Lost builders don't have their jobs unassigned on rescue"
https://bugs.launchpad.net/launchpad/+bug/1221002
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1221002/+merge/184030
Rework SlaveScanner.scan() to make a bit more sense. It now ensures that the slave and DB states agree at the start, rescuing the slave and job if the early check fails. This means that the builderok == False case is now handled early and actually prevents all slave communication, as you'd think it would.
If the slave and DB states match, we update or dispatch as appropriate. BuilderInteractor.isAvailable() dies, since we know after the rescue that the slave is IDLE iff currentjob in the DB is None.
SlaveScanner.scan() now takes builder and interactor arguments for overriding things in tests.
--
https://code.launchpad.net/~wgrant/launchpad/bug-1221002/+merge/184030
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1221002 into lp:launchpad.
=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py 2013-09-03 09:35:07 +0000
+++ lib/lp/buildmaster/interactor.py 2013-09-05 07:57:27 +0000
@@ -288,28 +288,17 @@
status['logtail'] = status_sentence[2]
defer.returnValue((status_sentence, status))
- @defer.inlineCallbacks
- def isAvailable(self):
- """Whether or not a builder is available for building new jobs.
-
- :return: A Deferred that fires with True or False, depending on
- whether the builder is available or not.
- """
- if not self.builder.builderok:
- defer.returnValue(False)
- try:
- status = yield self.slave.status()
- except (xmlrpclib.Fault, socket.error):
- defer.returnValue(False)
- defer.returnValue(status[0] == 'BuilderStatus.IDLE')
-
- def verifySlaveBuildCookie(self, slave_build_cookie):
+ def verifySlaveBuildCookie(self, slave_cookie):
"""See `IBuildFarmJobBehavior`."""
if self._current_build_behavior is None:
- raise CorruptBuildCookie('No job assigned to builder')
- good_cookie = self._current_build_behavior.getBuildCookie()
- if slave_build_cookie != good_cookie:
- raise CorruptBuildCookie("Invalid slave build cookie.")
+ if slave_cookie is not None:
+ raise CorruptBuildCookie('Slave building when should be idle.')
+ else:
+ good_cookie = self._current_build_behavior.getBuildCookie()
+ if slave_cookie != good_cookie:
+ raise CorruptBuildCookie(
+ "Invalid slave build cookie: got %r, expected %r."
+ % (slave_cookie, good_cookie))
@defer.inlineCallbacks
def rescueIfLost(self, logger=None):
@@ -322,7 +311,8 @@
`IBuildFarmJobBehavior.verifySlaveBuildCookie`.
:return: A Deferred that fires when the dialog with the slave is
- finished. It does not have a return value.
+ finished. Its return value is True if the slave is lost,
+ False otherwise.
"""
# 'ident_position' dict relates the position of the job identifier
# token in the sentence received from status(), according to the
@@ -330,38 +320,40 @@
# for further information about sentence format.
ident_position = {
'BuilderStatus.BUILDING': 1,
+ 'BuilderStatus.ABORTING': 1,
'BuilderStatus.WAITING': 2
}
- # If slave is not building nor waiting, it's not in need of
- # rescuing.
+ # Determine the slave's current build cookie. For BUILDING, ABORTING
+ # and WAITING we extract the string from the slave status
+ # sentence, and for IDLE it is None.
status_sentence = yield self.slave.status()
status = status_sentence[0]
if status not in ident_position.keys():
- return
- slave_build_id = status_sentence[ident_position[status]]
+ slave_cookie = None
+ else:
+ slave_cookie = status_sentence[ident_position[status]]
+
+ # verifySlaveBuildCookie will raise CorruptBuildCookie if the
+ # slave cookie doesn't match the expected one, including
+ # verifying that the slave cookie is None iff we expect the
+ # slave to be idle.
try:
- self.verifySlaveBuildCookie(slave_build_id)
+ self.verifySlaveBuildCookie(slave_cookie)
+ defer.returnValue(False)
except CorruptBuildCookie as reason:
+ # An IDLE slave doesn't need rescuing (SlaveScanner.scan
+ # will rescue the DB side instead), and we just have to wait
+ # out an ABORTING one.
if status == 'BuilderStatus.WAITING':
yield self.cleanSlave()
- else:
+ elif status == 'BuilderStatus.BUILDING':
yield self.requestAbort()
if logger:
logger.info(
"Builder '%s' rescued from '%s': '%s'" %
- (self.builder.name, slave_build_id, reason))
-
- def updateStatus(self, logger=None):
- """Update the builder's status by probing it.
-
- :return: A Deferred that fires when the dialog with the slave is
- finished. It does not have a return value.
- """
- if logger:
- logger.debug('Checking %s' % self.builder.name)
-
- return self.rescueIfLost(logger)
+ (self.builder.name, slave_cookie, reason))
+ defer.returnValue(True)
def cleanSlave(self):
"""Clean any temporary files from the slave.
@@ -524,8 +516,11 @@
:return: A Deferred that fires when the slave dialog is finished.
"""
+ # IDLE is deliberately not handled here, because it should be
+ # impossible to get past rescueIfLost unless the slave matches
+ # the DB, and this method isn't called unless the DB says
+ # there's a job.
builder_status_handlers = {
- 'BuilderStatus.IDLE': self.updateBuild_IDLE,
'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
'BuilderStatus.WAITING': self.updateBuild_WAITING,
@@ -539,18 +534,6 @@
method = builder_status_handlers[builder_status]
yield method(queueItem, status_sentence, status_dict, logger)
- def updateBuild_IDLE(self, queueItem, status_sentence, status_dict,
- logger):
- """Somehow the builder forgot about the build job.
-
- Log this and reset the record.
- """
- logger.warn(
- "Builder %s forgot about buildqueue %d -- resetting buildqueue "
- "record" % (queueItem.builder.url, queueItem.id))
- queueItem.reset()
- transaction.commit()
-
def updateBuild_BUILDING(self, queueItem, status_sentence, status_dict,
logger):
"""Build still building, collect the logtail"""
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py 2013-09-03 04:06:08 +0000
+++ lib/lp/buildmaster/manager.py 2013-09-05 07:57:27 +0000
@@ -250,82 +250,64 @@
defer.returnValue(value is not None)
@defer.inlineCallbacks
- def scan(self):
+ def scan(self, builder=None, interactor=None):
"""Probe the builder and update/dispatch/collect as appropriate.
- There are several steps to scanning:
-
- 1. If the builder is marked as "ok" then probe it to see what state
- it's in. This is where lost jobs are rescued if we think the
- builder is doing something that it later tells us it's not,
- and also where the multi-phase abort procedure happens.
- See IBuilder.rescueIfLost, which is called by
- IBuilder.updateStatus().
- 2. If the builder is still happy, we ask it if it has an active build
- and then either update the build in Launchpad or collect the
- completed build. (builder.updateBuild)
- 3. If the builder is not happy or it was marked as unavailable
- mid-build, we need to reset the job that we thought it had, so
- that the job is dispatched elsewhere.
- 4. If the builder is idle and we have another build ready, dispatch
- it.
-
- :return: A Deferred that fires when the scan is complete, whose
- value is A `BuilderSlave` if we dispatched a job to it, or None.
+ :return: A Deferred that fires when the scan is complete.
"""
- # We need to re-fetch the builder object on each cycle as the
- # Storm store is invalidated over transaction boundaries.
-
- self.builder = get_builder(self.builder_name)
- self.interactor = BuilderInteractor(self.builder)
-
- if self.builder.builderok:
+ self.logger.debug("Scanning %s." % self.builder_name)
+ # Commit and refetch the Builder object to ensure we have the
+ # latest data from the DB.
+ transaction.commit()
+ self.builder = builder or get_builder(self.builder_name)
+ self.interactor = interactor or BuilderInteractor(self.builder)
+
+ # Confirm that the DB and slave sides are in a valid, mutually
+ # agreeable state.
+ lost_reason = None
+ if not self.builder.builderok:
+ lost_reason = '%s is disabled' % self.builder.name
+ else:
cancelled = yield self.checkCancellation(self.builder)
if cancelled:
return
- yield self.interactor.updateStatus(self.logger)
-
- # Commit the changes done while possibly rescuing jobs, to
- # avoid holding table locks.
- transaction.commit()
-
- buildqueue = self.builder.currentjob
- if buildqueue is not None:
+ lost = yield self.interactor.rescueIfLost(self.logger)
+ if lost:
+ lost_reason = '%s is lost' % self.builder.name
+
+ # The slave is lost or the builder is disabled. We can't
+ # continue to update the job status or dispatch a new job, so
+ # just rescue the assigned job, if any, so it can be dispatched
+ # to another slave.
+ if lost_reason is not None:
+ if self.builder.currentjob is not None:
+ self.logger.warn(
+ "%s. Resetting BuildQueue %d.", lost_reason,
+ self.builder.currentjob.id)
+ self.builder.currentjob.reset()
+ transaction.commit()
+ return
+
+ # We've confirmed that the slave state matches the DB. Continue
+ # with updating the job status, or dispatching a new job if the
+ # builder is idle.
+ if self.builder.currentjob is not None:
# Scan the slave and get the logtail, or collect the build
# if it's ready. Yes, "updateBuild" is a bad name.
- yield self.interactor.updateBuild(buildqueue)
-
- # If the builder is in manual mode, don't dispatch anything.
- if self.builder.manual:
+ yield self.interactor.updateBuild(self.builder.currentjob)
+ elif self.builder.manual:
+ # If the builder is in manual mode, don't dispatch anything.
self.logger.debug(
'%s is in manual mode, not dispatching.' %
self.builder.name)
- return
-
- # If the builder is marked unavailable, don't dispatch anything.
- # Additionaly, because builders can be removed from the pool at
- # any time, we need to see if we think there was a build running
- # on it before it was marked unavailable. In this case we reset
- # the build thusly forcing it to get re-dispatched to another
- # builder.
- available = yield self.interactor.isAvailable()
- if not available:
- job = self.builder.currentjob
- if job is not None and not self.builder.builderok:
- self.logger.info(
- "%s was made unavailable, resetting attached "
- "job" % self.builder.name)
- job.reset()
+ else:
+ # See if there is a job we can dispatch to the builder slave.
+ yield self.interactor.findAndStartJob()
+ if self.builder.currentjob is not None:
+ # After a successful dispatch we can reset the
+ # failure_count.
+ self.builder.resetFailureCount()
transaction.commit()
- return
-
- # See if there is a job we can dispatch to the builder slave.
- yield self.interactor.findAndStartJob()
- if self.builder.currentjob is not None:
- # After a successful dispatch we can reset the
- # failure_count.
- self.builder.resetFailureCount()
- transaction.commit()
class NewBuildersScanner:
=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py 2013-09-02 12:45:50 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py 2013-09-05 07:57:27 +0000
@@ -80,28 +80,33 @@
self.assertRaises(
AssertionError, interactor.extractBuildStatus, slave_status)
- def test_verifySlaveBuildCookie_good(self):
+ def test_verifySlaveBuildCookie_building_match(self):
interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
interactor.verifySlaveBuildCookie('trivial')
- def test_verifySlaveBuildCookie_bad(self):
+ def test_verifySlaveBuildCookie_building_mismatch(self):
interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
self.assertRaises(
CorruptBuildCookie,
interactor.verifySlaveBuildCookie, 'difficult')
- def test_verifySlaveBuildCookie_idle(self):
+ def test_verifySlaveBuildCookie_idle_match(self):
+ interactor = BuilderInteractor(MockBuilder())
+ self.assertIs(None, interactor._current_build_behavior)
+ interactor.verifySlaveBuildCookie(None)
+
+ def test_verifySlaveBuildCookie_idle_mismatch(self):
interactor = BuilderInteractor(MockBuilder())
self.assertIs(None, interactor._current_build_behavior)
self.assertRaises(
CorruptBuildCookie, interactor.verifySlaveBuildCookie, 'foo')
- def test_updateStatus_aborts_lost_and_broken_slave(self):
+ def test_rescueIfLost_aborts_lost_and_broken_slave(self):
# A slave that's 'lost' should be aborted; when the slave is
# broken then abort() should also throw a fault.
slave = LostBuildingBrokenSlave()
interactor = BuilderInteractor(MockBuilder(), slave, TrivialBehavior())
- d = interactor.updateStatus(DevNullLogger())
+ d = interactor.rescueIfLost(DevNullLogger())
def check_slave_status(failure):
self.assertIn('abort', slave.call_log)
@@ -170,31 +175,37 @@
interactor = BuilderInteractor(builder)
self.assertEqual(5, interactor.slave.timeout)
+ @defer.inlineCallbacks
+ def test_recover_idle_slave(self):
+ # An idle slave is not rescued, even if it's not meant to be
+ # idle. SlaveScanner.scan() will clean up the DB side, because
+ # we still report that it's lost.
+ slave = OkSlave()
+ lost = yield BuilderInteractor(
+ MockBuilder(), slave, TrivialBehavior()).rescueIfLost()
+ self.assertTrue(lost)
+ self.assertEqual([], slave.call_log)
+
+ @defer.inlineCallbacks
def test_recover_ok_slave(self):
- # An idle slave is not rescued.
+ # An idle slave that's meant to be idle is not rescued.
slave = OkSlave()
- d = BuilderInteractor(
- MockBuilder(), slave, TrivialBehavior()).rescueIfLost()
-
- def check_slave_calls(ignored):
- self.assertNotIn('abort', slave.call_log)
- self.assertNotIn('clean', slave.call_log)
-
- return d.addCallback(check_slave_calls)
-
+ lost = yield BuilderInteractor(
+ MockBuilder(), slave, None).rescueIfLost()
+ self.assertFalse(lost)
+ self.assertEqual([], slave.call_log)
+
+ @defer.inlineCallbacks
def test_recover_waiting_slave_with_good_id(self):
# rescueIfLost does not attempt to abort or clean a builder that is
# WAITING.
waiting_slave = WaitingSlave(build_id='trivial')
- d = BuilderInteractor(
+ lost = yield BuilderInteractor(
MockBuilder(), waiting_slave, TrivialBehavior()).rescueIfLost()
-
- def check_slave_calls(ignored):
- self.assertNotIn('abort', waiting_slave.call_log)
- self.assertNotIn('clean', waiting_slave.call_log)
-
- return d.addCallback(check_slave_calls)
-
+ self.assertFalse(lost)
+ self.assertEqual(['status'], waiting_slave.call_log)
+
+ @defer.inlineCallbacks
def test_recover_waiting_slave_with_bad_id(self):
# If a slave is WAITING with a build for us to get, and the build
# cookie cannot be verified, which means we don't recognize the build,
@@ -202,40 +213,30 @@
# builder is reset for a new build, and the corrupt build is
# discarded.
waiting_slave = WaitingSlave(build_id='non-trivial')
- d = BuilderInteractor(
+ lost = yield BuilderInteractor(
MockBuilder(), waiting_slave, TrivialBehavior()).rescueIfLost()
-
- def check_slave_calls(ignored):
- self.assertNotIn('abort', waiting_slave.call_log)
- self.assertIn('clean', waiting_slave.call_log)
-
- return d.addCallback(check_slave_calls)
-
+ self.assertTrue(lost)
+ self.assertEqual(['status', 'clean'], waiting_slave.call_log)
+
+ @defer.inlineCallbacks
def test_recover_building_slave_with_good_id(self):
# rescueIfLost does not attempt to abort or clean a builder that is
# BUILDING.
building_slave = BuildingSlave(build_id='trivial')
- d = BuilderInteractor(
+ lost = yield BuilderInteractor(
MockBuilder(), building_slave, TrivialBehavior()).rescueIfLost()
-
- def check_slave_calls(ignored):
- self.assertNotIn('abort', building_slave.call_log)
- self.assertNotIn('clean', building_slave.call_log)
-
- return d.addCallback(check_slave_calls)
-
+ self.assertFalse(lost)
+ self.assertEqual(['status'], building_slave.call_log)
+
+ @defer.inlineCallbacks
def test_recover_building_slave_with_bad_id(self):
# If a slave is BUILDING with a build id we don't recognize, then we
# abort the build, thus stopping it in its tracks.
building_slave = BuildingSlave(build_id='non-trivial')
- d = BuilderInteractor(
+ lost = yield BuilderInteractor(
MockBuilder(), building_slave, TrivialBehavior()).rescueIfLost()
-
- def check_slave_calls(ignored):
- self.assertIn('abort', building_slave.call_log)
- self.assertNotIn('clean', building_slave.call_log)
-
- return d.addCallback(check_slave_calls)
+ self.assertTrue(lost)
+ self.assertEqual(['status', 'abort'], building_slave.call_log)
class TestBuilderInteractorSlaveStatus(TestCase):
@@ -285,21 +286,6 @@
self.assertStatus(
AbortingSlave(), builder_status='BuilderStatus.ABORTING')
- def test_isAvailable_with_not_builderok(self):
- # isAvailable() is a wrapper around BuilderSlave.status()
- builder = MockBuilder()
- builder.builderok = False
- d = BuilderInteractor(builder).isAvailable()
- return d.addCallback(self.assertFalse)
-
- def test_isAvailable_with_slave_fault(self):
- d = BuilderInteractor(MockBuilder(), BrokenSlave()).isAvailable()
- return d.addCallback(self.assertFalse)
-
- def test_isAvailable_with_slave_idle(self):
- d = BuilderInteractor(MockBuilder(), OkSlave()).isAvailable()
- return d.addCallback(self.assertTrue)
-
class TestBuilderInteractorDB(TestCaseWithFactory):
"""BuilderInteractor tests that need a DB."""
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py 2013-09-03 03:41:02 +0000
+++ lib/lp/buildmaster/tests/test_manager.py 2013-09-05 07:57:27 +0000
@@ -47,7 +47,9 @@
BuildingSlave,
LostBuildingBrokenSlave,
make_publisher,
+ MockBuilder,
OkSlave,
+ TrivialBehavior,
)
from lp.registry.interfaces.distribution import IDistributionSet
from lp.services.config import config
@@ -178,28 +180,30 @@
build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
self.assertEqual(build.status, BuildStatus.NEEDSBUILD)
+ @defer.inlineCallbacks
def testScanRescuesJobFromBrokenBuilder(self):
# The job assigned to a broken builder is rescued.
- self.useFixture(BuilddSlaveTestSetup())
-
# Sampledata builder is enabled and is assigned to an active job.
builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+ self.patch(
+ BuilderSlave, 'makeBuilderSlave',
+ FakeMethod(BuildingSlave(build_id='PACKAGEBUILD-8')))
self.assertTrue(builder.builderok)
job = builder.currentjob
self.assertBuildingJob(job, builder)
+ scanner = self._getScanner()
+ yield scanner.scan()
+ self.assertIsNot(None, builder.currentjob)
+
# Disable the sampledata builder
- login('foo.bar@xxxxxxxxxxxxx')
builder.builderok = False
transaction.commit()
- login(ANONYMOUS)
# Run 'scan' and check its result.
- switch_dbuser(config.builddmaster.dbuser)
- scanner = self._getScanner()
- d = defer.maybeDeferred(scanner.scan)
- d.addCallback(self._checkJobRescued, builder, job)
- return d
+ slave = yield scanner.scan()
+ self.assertIs(None, builder.currentjob)
+ self._checkJobRescued(slave, builder, job)
def _checkJobUpdated(self, slave, builder, job):
"""`SlaveScanner.scan` updates legitimate jobs.
@@ -223,7 +227,7 @@
login('foo.bar@xxxxxxxxxxxxx')
builder.builderok = True
self.patch(BuilderSlave, 'makeBuilderSlave',
- FakeMethod(BuildingSlave(build_id='8-1')))
+ FakeMethod(BuildingSlave(build_id='PACKAGEBUILD-8')))
transaction.commit()
login(ANONYMOUS)
@@ -437,6 +441,75 @@
self.assertEqual(BuildStatus.CANCELLED, build.status)
+class FakeBuildQueue:
+
+ def __init__(self):
+ self.id = 1
+ self.reset = FakeMethod()
+
+
+class TestSlaveScannerWithoutDB(TestCase):
+
+ run_tests_with = AsynchronousDeferredRunTest
+
+ @defer.inlineCallbacks
+ def test_scan_with_job(self):
+ # SlaveScanner.scan calls updateBuild() when a job is building.
+ interactor = BuilderInteractor(
+ MockBuilder(), BuildingSlave('trivial'), TrivialBehavior())
+ scanner = SlaveScanner('mock', BufferLogger())
+
+ # Instrument updateBuild and currentjob.reset
+ interactor.updateBuild = FakeMethod()
+ interactor.builder.currentjob = FakeBuildQueue()
+ # XXX: checkCancellation needs more than a FakeBuildQueue.
+ scanner.checkCancellation = FakeMethod(defer.succeed(False))
+
+ yield scanner.scan(builder=interactor.builder, interactor=interactor)
+ self.assertEqual(['status'], interactor.slave.call_log)
+ self.assertEqual(1, interactor.updateBuild.call_count)
+ self.assertEqual(0, interactor.builder.currentjob.reset.call_count)
+
+ @defer.inlineCallbacks
+ def test_scan_aborts_lost_slave_with_job(self):
+ # SlaveScanner.scan uses BuilderInteractor.rescueIfLost to abort
+ # slaves that don't have the expected job.
+ interactor = BuilderInteractor(
+ MockBuilder(), BuildingSlave('nontrivial'), TrivialBehavior())
+ scanner = SlaveScanner('mock', BufferLogger())
+
+ # Instrument updateBuild and currentjob.reset
+ interactor.updateBuild = FakeMethod()
+ interactor.builder.currentjob = FakeBuildQueue()
+ # XXX: checkCancellation needs more than a FakeBuildQueue.
+ scanner.checkCancellation = FakeMethod(defer.succeed(False))
+
+ # A single scan will call status(), notice that the slave is
+ # lost, abort() the slave, then reset() the job without calling
+ # updateBuild().
+ yield scanner.scan(builder=interactor.builder, interactor=interactor)
+ self.assertEqual(['status', 'abort'], interactor.slave.call_log)
+ self.assertEqual(0, interactor.updateBuild.call_count)
+ self.assertEqual(1, interactor.builder.currentjob.reset.call_count)
+
+ @defer.inlineCallbacks
+ def test_scan_aborts_lost_slave_when_idle(self):
+ # SlaveScanner.scan uses BuilderInteractor.rescueIfLost to abort
+ # slaves that aren't meant to have a job.
+ interactor = BuilderInteractor(MockBuilder(), BuildingSlave(), None)
+ scanner = SlaveScanner('mock', BufferLogger())
+
+ # Instrument updateBuild.
+ interactor.updateBuild = FakeMethod()
+
+ # A single scan will call status(), notice that the slave is
+ # lost, abort() the slave, then reset() the job without calling
+ # updateBuild().
+ yield scanner.scan(builder=interactor.builder, interactor=interactor)
+ self.assertEqual(['status', 'abort'], interactor.slave.call_log)
+ self.assertEqual(0, interactor.updateBuild.call_count)
+
+
class TestCancellationChecking(TestCaseWithFactory):
"""Unit tests for the checkCancellation method."""
Follow ups
