launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15833
[Merge] lp:~wgrant/launchpad/builderinteractor-cookies into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/builderinteractor-cookies into lp:launchpad.
Commit message:
Move slave cookie verification from BuildFarmJobBehavior to BuilderInteractor, and only call updateSlaveStatus in updateBuild_WAITING.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/builderinteractor-cookies/+merge/182813
Move slave cookie verification from BuildFarmJobBehavior to BuilderInteractor. All behaviours verify the cookie the same way, so all they need to know is how to calculate it.
I also adjusted BuilderInteractor.slaveStatus to not call BuildFarmJobBehavior.updateSlaveStatus. Only BuildFarmJobBehavior.handleStatus needs the augmented status dict, so updateBuild_WAITING calls updateSlaveStatus directly. This means that the regular builder scan path only touched the BFJB to generate the expected slave cookie.
--
https://code.launchpad.net/~wgrant/launchpad/builderinteractor-cookies/+merge/182813
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/builderinteractor-cookies into lp:launchpad.
=== modified file 'lib/lp/buildmaster/doc/buildfarmjobbehavior.txt'
--- lib/lp/buildmaster/doc/buildfarmjobbehavior.txt 2013-08-27 10:23:43 +0000
+++ lib/lp/buildmaster/doc/buildfarmjobbehavior.txt 2013-08-29 04:17:48 +0000
@@ -103,11 +103,3 @@
...
BuildBehaviorMismatch: Builder was idle when asked to log the start of a
build.
-
-If a slave is working on a job while we think it is idle, it will always be
-aborted.
-
- >>> interactor._current_build_behavior.verifySlaveBuildCookie('foo')
- Traceback (most recent call last):
- ...
- CorruptBuildCookie: No job assigned to builder
=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py 2013-08-28 08:03:32 +0000
+++ lib/lp/buildmaster/interactor.py 2013-08-29 04:17:48 +0000
@@ -303,10 +303,7 @@
else:
if status['builder_status'] == 'BuilderStatus.BUILDING':
status['logtail'] = status_sentence[2]
-
- self._current_build_behavior.updateSlaveStatus(
- status_sentence, status)
- return status
+ return (status_sentence, status)
return d.addCallback(got_status)
@@ -328,6 +325,14 @@
return status[0] == 'BuilderStatus.IDLE'
return d.addCallbacks(check_available, catch_fault)
+ def verifySlaveBuildCookie(self, slave_build_cookie):
+ """See `IBuildFarmJobBehavior`."""
+ if isinstance(self._current_build_behavior, IdleBuildBehavior):
+ raise CorruptBuildCookie('No job assigned to builder')
+ good_cookie = self._current_build_behavior.generateSlaveBuildCookie()
+ if slave_build_cookie != good_cookie:
+ raise CorruptBuildCookie("Invalid slave build cookie.")
+
def rescueIfLost(self, logger=None):
"""Reset the slave if its job information doesn't match the DB.
@@ -396,8 +401,7 @@
return
slave_build_id = status_sentence[ident_position[status]]
try:
- self._current_build_behavior.verifySlaveBuildCookie(
- slave_build_id)
+ self.verifySlaveBuildCookie(slave_build_id)
except CorruptBuildCookie as reason:
if status == 'BuilderStatus.WAITING':
d = self.cleanSlave()
@@ -625,7 +629,7 @@
% (queueItem.builder.url, info))
raise BuildSlaveFailure(info)
- def got_status(slave_status):
+ def got_status(statuses):
builder_status_handlers = {
'BuilderStatus.IDLE': self.updateBuild_IDLE,
'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
@@ -633,8 +637,8 @@
'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
'BuilderStatus.WAITING': self.updateBuild_WAITING,
}
-
- builder_status = slave_status['builder_status']
+ status_sentence, status_dict = statuses
+ builder_status = status_dict['builder_status']
if builder_status not in builder_status_handlers:
logger.critical(
"Builder on %s returned unknown status %s, failing it"
@@ -650,22 +654,16 @@
transaction.commit()
return
- # Since logtail is a xmlrpclib.Binary container and it is
- # returned from the IBuilder content class, it arrives
- # protected by a Zope Security Proxy, which is not declared,
- # thus empty. Before passing it to the status handlers we
- # will simply remove the proxy.
- logtail = removeSecurityProxy(slave_status.get('logtail'))
-
method = builder_status_handlers[builder_status]
return defer.maybeDeferred(
- method, queueItem, slave_status, logtail, logger)
+ method, queueItem, status_sentence, status_dict, logger)
d.addErrback(got_failure)
d.addCallback(got_status)
return d
- def updateBuild_IDLE(self, queueItem, slave_status, logtail, 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.
@@ -676,14 +674,16 @@
queueItem.reset()
transaction.commit()
- def updateBuild_BUILDING(self, queueItem, slave_status, logtail, logger):
+ def updateBuild_BUILDING(self, queueItem, status_sentence, status_dict,
+ logger):
"""Build still building, collect the logtail"""
if queueItem.job.status != JobStatus.RUNNING:
queueItem.job.start()
- queueItem.logtail = encoding.guess(str(logtail))
+ queueItem.logtail = encoding.guess(str(status_dict.get('logtail')))
transaction.commit()
- def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger):
+ def updateBuild_ABORTING(self, queueItem, status_sentence, status_dict,
+ logger):
"""Build was ABORTED.
Master-side should wait until the slave finish the process correctly.
@@ -691,7 +691,8 @@
queueItem.logtail = "Waiting for slave process to be terminated"
transaction.commit()
- def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger):
+ def updateBuild_ABORTED(self, queueItem, status_sentence, status_dict,
+ logger):
"""ABORTING process has successfully terminated.
Clean the builder for another jobs.
@@ -706,21 +707,22 @@
transaction.commit()
return d.addCallback(got_cleaned)
- def extractBuildStatus(self, slave_status):
+ def extractBuildStatus(self, status_dict):
"""Read build status name.
- :param slave_status: build status dict as passed to the
+ :param status_dict: build status dict as passed to the
updateBuild_* methods.
:return: the unqualified status name, e.g. "OK".
"""
- status_string = slave_status['build_status']
+ status_string = status_dict['build_status']
lead_string = 'BuildStatus.'
assert status_string.startswith(lead_string), (
"Malformed status string: '%s'" % status_string)
return status_string[len(lead_string):]
- def updateBuild_WAITING(self, queueItem, slave_status, logtail, logger):
+ def updateBuild_WAITING(self, queueItem, status_sentence, status_dict,
+ logger):
"""Perform the actions needed for a slave in a WAITING state
Buildslave can be WAITING in five situations:
@@ -734,12 +736,11 @@
Librarian with getFileFromSlave() and then pass the binaries to
the uploader for processing.
"""
- librarian = getUtility(ILibrarianClient)
- build_status = self.extractBuildStatus(slave_status)
-
- # XXX: dsilvers 2005-03-02: Confirm the builder has the right build?
+ self._current_build_behavior.updateSlaveStatus(
+ status_sentence, status_dict)
d = self._current_build_behavior.handleStatus(
- build_status, librarian, slave_status)
+ self.extractBuildStatus(status_dict),
+ getUtility(ILibrarianClient), status_dict)
return d
def transferSlaveFileToLibrarian(self, file_sha1, filename, private):
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
--- lib/lp/buildmaster/interfaces/buildfarmjob.py 2013-05-01 22:53:22 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2013-08-29 04:17:48 +0000
@@ -149,11 +149,8 @@
def generateSlaveBuildCookie():
"""Produce a cookie for the slave as a token of the job it's doing.
- The cookie need not be unique, but should be hard for a
- compromised slave to guess.
-
- :return: a hard-to-guess ASCII string that can be reproduced
- accurately based on this job's properties.
+ The cookie should uniquely represent the current dispatch of this
+ build.
"""
def cleanUp():
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2013-08-28 10:13:18 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2013-08-29 04:17:48 +0000
@@ -46,30 +46,29 @@
:param logger: A logger to be used to log diagnostic information.
"""
- def updateSlaveStatus(raw_slave_status, status):
+ def generateSlaveBuildCookie():
+ """Produce a cookie for the slave as a token of the job it's doing.
+
+ The cookie should uniquely represent the current dispatch of the
+ current build.
+ """
+
+ def updateSlaveStatus(status_sentence, status_dict):
"""Update the slave status dict with custom values for this behavior.
- :param raw_slave_status: The value returned by the build slave's
+ :param status_sentence: The value returned by the build slave's
status() method.
- :param status: A dict of the processed slave status values provided
- by all types: builder_status, build_id, and optionally build_status
- or logtail. This should have any behaviour-specific values
- added to it.
- """
-
- def verifySlaveBuildCookie(slave_build_cookie):
- """Verify that a slave's build cookie shows no signs of corruption.
-
- :param slave_build_cookie: The slave's build cookie, as specified in
- `dispatchBuildToSlave`.
- :raises CorruptBuildCookie: if the build cookie isn't what it's
- supposed to be.
- """
-
- def handleStatus(status, librarian, slave_status):
+ :param status_dict: A dict of the processed slave status values
+ provided by all types: builder_status, build_id, and optionally
+ build_status or logtail. This should have any behaviour-specific
+ values added to it.
+ """
+
+ def handleStatus(status, librarian, status_dict):
"""Update the build from a WAITING slave result.
:param status: The tail of the BuildStatus (eg. OK or PACKAGEFAIL).
:param librarian: An `ILibrarianClient` to use for file uploads.
- :param slave_status: Slave status from `BuilderInteractor.slaveStatus`.
+ :param status_dict: Slave status dict from
+ `BuilderInteractor.slaveStatus` and `updateSlaveStatus`.
"""
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-08-28 10:13:18 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2013-08-29 04:17:48 +0000
@@ -22,10 +22,7 @@
BuildFarmJobType,
BuildStatus,
)
-from lp.buildmaster.interfaces.builder import (
- BuildSlaveFailure,
- CorruptBuildCookie,
- )
+from lp.buildmaster.interfaces.builder import BuildSlaveFailure
from lp.buildmaster.interfaces.buildfarmjobbehavior import (
BuildBehaviorMismatch,
IBuildFarmJobBehavior,
@@ -66,11 +63,8 @@
The default behavior is that we don't add any extra values."""
pass
- def verifySlaveBuildCookie(self, slave_build_cookie):
- """See `IBuildFarmJobBehavior`."""
- expected_cookie = self.buildfarmjob.generateSlaveBuildCookie()
- if slave_build_cookie != expected_cookie:
- raise CorruptBuildCookie("Invalid slave build cookie.")
+ def generateSlaveBuildCookie(self):
+ return self.buildfarmjob.generateSlaveBuildCookie()
def getBuildCookie(self):
"""See `IPackageBuild`."""
@@ -336,12 +330,3 @@
"""See `IBuildFarmJobBehavior`."""
raise BuildBehaviorMismatch(
"Builder was idle when asked to dispatch a build to the slave.")
-
- @property
- def status(self):
- """See `IBuildFarmJobBehavior`."""
- return "Idle"
-
- def verifySlaveBuildCookie(self, slave_build_id):
- """See `IBuildFarmJobBehavior`."""
- raise CorruptBuildCookie('No job assigned to builder')
=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py 2013-08-27 09:17:07 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py 2013-08-29 04:17:48 +0000
@@ -33,10 +33,7 @@
from twisted.web import xmlrpc
from lp.buildmaster.interactor import BuilderSlave
-from lp.buildmaster.interfaces.builder import (
- CannotFetchFile,
- CorruptBuildCookie,
- )
+from lp.buildmaster.interfaces.builder import CannotFetchFile
from lp.services.config import config
from lp.testing.sampledata import I386_ARCHITECTURE_NAME
@@ -238,16 +235,10 @@
return defer.fail(xmlrpclib.Fault(8001, "Broken slave"))
-class CorruptBehavior:
-
- def verifySlaveBuildCookie(self, cookie):
- raise CorruptBuildCookie("Bad value: %r" % (cookie,))
-
-
class TrivialBehavior:
- def verifySlaveBuildCookie(self, cookie):
- pass
+ def generateSlaveBuildCookie(self):
+ return 'trivial'
class DeadProxy(xmlrpc.Proxy):
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py 2013-08-28 09:26:17 +0000
+++ lib/lp/buildmaster/tests/test_builder.py 2013-08-29 04:17:48 +0000
@@ -18,6 +18,7 @@
from twisted.internet.defer import (
CancelledError,
DeferredList,
+ inlineCallbacks,
)
from twisted.internet.task import Clock
from twisted.python.failure import Failure
@@ -36,6 +37,7 @@
from lp.buildmaster.interfaces.builder import (
CannotFetchFile,
CannotResumeHost,
+ CorruptBuildCookie,
IBuilder,
IBuilderSet,
)
@@ -47,7 +49,6 @@
AbortingSlave,
BrokenSlave,
BuildingSlave,
- CorruptBehavior,
DeadProxy,
LostBuildingBrokenSlave,
make_publisher,
@@ -268,11 +269,28 @@
self.assertRaises(
AssertionError, interactor.extractBuildStatus, slave_status)
+ def test_verifySlaveBuildCookie_good(self):
+ interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
+ interactor.verifySlaveBuildCookie('trivial')
+
+ def test_verifySlaveBuildCookie_bad(self):
+ interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
+ self.assertRaises(
+ CorruptBuildCookie,
+ interactor.verifySlaveBuildCookie, 'difficult')
+
+ def test_verifySlaveBuildCookie_idle(self):
+ interactor = BuilderInteractor(MockBuilder())
+ self.assertTrue(
+ isinstance(interactor._current_build_behavior, IdleBuildBehavior))
+ self.assertRaises(
+ CorruptBuildCookie, interactor.verifySlaveBuildCookie, 'foo')
+
def test_updateStatus_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, CorruptBehavior())
+ interactor = BuilderInteractor(MockBuilder(), slave, TrivialBehavior())
d = interactor.updateStatus(BufferLogger())
def check_slave_status(failure):
@@ -383,7 +401,7 @@
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()
+ waiting_slave = WaitingSlave(build_id='trivial')
d = BuilderInteractor(
MockBuilder(), waiting_slave, TrivialBehavior()).rescueIfLost()
@@ -399,9 +417,9 @@
# then rescueBuilderIfLost should attempt to abort it, so that the
# builder is reset for a new build, and the corrupt build is
# discarded.
- waiting_slave = WaitingSlave()
+ waiting_slave = WaitingSlave(build_id='non-trivial')
d = BuilderInteractor(
- MockBuilder(), waiting_slave, CorruptBehavior()).rescueIfLost()
+ MockBuilder(), waiting_slave, TrivialBehavior()).rescueIfLost()
def check_slave_calls(ignored):
self.assertNotIn('abort', waiting_slave.call_log)
@@ -412,7 +430,7 @@
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()
+ building_slave = BuildingSlave(build_id='trivial')
d = BuilderInteractor(
MockBuilder(), building_slave, TrivialBehavior()).rescueIfLost()
@@ -425,9 +443,9 @@
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()
+ building_slave = BuildingSlave(build_id='non-trivial')
d = BuilderInteractor(
- MockBuilder(), building_slave, CorruptBehavior()).rescueIfLost()
+ MockBuilder(), building_slave, TrivialBehavior()).rescueIfLost()
def check_slave_calls(ignored):
self.assertIn('abort', building_slave.call_log)
@@ -442,29 +460,28 @@
run_tests_with = AsynchronousDeferredRunTest
+ @inlineCallbacks
def assertStatus(self, slave, builder_status=None,
build_status=None, logtail=False, filemap=None,
dependencies=None):
- d = BuilderInteractor(MockBuilder(), slave).slaveStatus()
-
- def got_status(status_dict):
- expected = {}
- if builder_status is not None:
- expected["builder_status"] = builder_status
- if build_status is not None:
- expected["build_status"] = build_status
- if dependencies is not None:
- expected["dependencies"] = dependencies
-
- # We don't care so much about the content of the logtail,
- # just that it's there.
- if logtail:
- tail = status_dict.pop("logtail")
- self.assertIsInstance(tail, xmlrpclib.Binary)
-
- self.assertEqual(expected, status_dict)
-
- return d.addCallback(got_status)
+ statuses = yield BuilderInteractor(MockBuilder(), slave).slaveStatus()
+ status_dict = statuses[1]
+
+ expected = {}
+ if builder_status is not None:
+ expected["builder_status"] = builder_status
+ if build_status is not None:
+ expected["build_status"] = build_status
+ if dependencies is not None:
+ expected["dependencies"] = dependencies
+
+ # We don't care so much about the content of the logtail,
+ # just that it's there.
+ if logtail:
+ tail = status_dict.pop("logtail")
+ self.assertIsInstance(tail, xmlrpclib.Binary)
+
+ self.assertEqual(expected, status_dict)
def test_slaveStatus_idle_slave(self):
self.assertStatus(
=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-08-28 09:26:17 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py 2013-08-29 04:17:48 +0000
@@ -16,7 +16,6 @@
from lp.archiveuploader.uploadprocessor import parse_build_upload_leaf_name
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interactor import BuilderInteractor
-from lp.buildmaster.interfaces.builder import CorruptBuildCookie
from lp.buildmaster.interfaces.buildfarmjobbehavior import (
IBuildFarmJobBehavior,
)
@@ -54,11 +53,6 @@
buildfarmjob = removeSecurityProxy(buildfarmjob)
return BuildFarmJobBehaviorBase(buildfarmjob)
- def _changeBuildFarmJobName(self, buildfarmjob):
- """Manipulate `buildfarmjob` so that its `getName` changes."""
- name = buildfarmjob.getName() + 'x'
- removeSecurityProxy(buildfarmjob).getName = FakeMethod(result=name)
-
def _makeBuild(self):
"""Create a `Build` object."""
x86 = getUtility(IProcessorFamilySet).getByName('x86')
@@ -78,6 +72,11 @@
"""Create a `BuildQueue` object."""
return self.factory.makeSourcePackageRecipeBuildJob()
+ def _changeBuildFarmJobName(self, buildfarmjob):
+ """Manipulate `buildfarmjob` so that its `getName` changes."""
+ name = buildfarmjob.getName() + 'x'
+ removeSecurityProxy(buildfarmjob).getName = FakeMethod(result=name)
+
def test_cookie_baseline(self):
buildfarmjob = self.factory.makeTranslationTemplatesBuildJob()
@@ -89,43 +88,13 @@
self.assertEqual(cookie, buildfarmjob.generateSlaveBuildCookie())
- def test_verifySlaveBuildCookie_good(self):
- buildfarmjob = self.factory.makeTranslationTemplatesBuildJob()
- behavior = self._makeBehavior(buildfarmjob)
-
- cookie = buildfarmjob.generateSlaveBuildCookie()
-
- # The correct cookie validates successfully.
- behavior.verifySlaveBuildCookie(cookie)
-
- def test_verifySlaveBuildCookie_bad(self):
- buildfarmjob = self.factory.makeTranslationTemplatesBuildJob()
- behavior = self._makeBehavior(buildfarmjob)
-
- cookie = buildfarmjob.generateSlaveBuildCookie()
-
- self.assertRaises(
- CorruptBuildCookie,
- behavior.verifySlaveBuildCookie,
- cookie + 'x')
-
def test_cookie_includes_job_name(self):
# The cookie is a hash that includes the job's name.
buildfarmjob = self.factory.makeTranslationTemplatesBuildJob()
- buildfarmjob = removeSecurityProxy(buildfarmjob)
- behavior = self._makeBehavior(buildfarmjob)
cookie = buildfarmjob.generateSlaveBuildCookie()
-
- self._changeBuildFarmJobName(buildfarmjob)
-
- self.assertRaises(
- CorruptBuildCookie,
- behavior.verifySlaveBuildCookie,
- cookie)
-
- # However, the name is not included in plaintext so as not to
- # provide a compromised slave a starting point for guessing
- # another slave's cookie.
+ self._changeBuildFarmJobName(removeSecurityProxy(buildfarmjob))
+
+ self.assertNotEqual(cookie, buildfarmjob.generateSlaveBuildCookie())
self.assertNotIn(buildfarmjob.getName(), cookie)
def test_cookie_includes_more_than_name(self):
=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2013-08-28 09:21:19 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2013-08-29 04:17:48 +0000
@@ -41,17 +41,6 @@
)
-class FakeBuilder:
- """Pretend `Builder`."""
-
- def __init__(self, slave):
- self.slave = slave
- self.cleanSlave = FakeMethod()
-
- def slaveStatus(self):
- return self.slave._status
-
-
class FakeBuildQueue:
"""Pretend `BuildQueue`."""
Follow ups
