Merge lp:~allenap/launchpad/undo-read-only-transactions-in-buildmaster into lp:launchpad

[Gavin Panella]

[r=allenap][bug=905853,905855,906079] Revert r14499 and r14459 because read-only transactions in buildmaster are causing production issues.

Revert r14499 and r14459 because read-only transactions in buildmaster
are causing production issues. This should bring stable into line with
current cowboy on cesium.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-12-13 17:10:46 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-12-19 21:48:31 +0000
@@ -629,8 +629,7 @@
         from_addr, to_addrs, raw_msg = stub.test_emails.pop()
         foo_bar = "Foo Bar <[e-mail]>"
         daniel = "Daniel Silverstone <[e-mail]>"
-        self.assertContentEqual(
-            [foo_bar, daniel], [e.strip() for e in to_addrs])
+        self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel])
         self.assertTrue(
             "NEW" in raw_msg, "Expected email containing 'NEW', got:\n%s"
             % raw_msg)
@@ -664,8 +663,7 @@
         from_addr, to_addrs, raw_msg = stub.test_emails.pop()
         daniel = "Daniel Silverstone <[e-mail]>"
         foo_bar = "Foo Bar <[e-mail]>"
-        self.assertContentEqual(
-            [foo_bar, daniel], [e.strip() for e in to_addrs])
+        self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel])
         self.assertTrue("Waiting for approval" in raw_msg,
                         "Expected an 'upload awaits approval' email.\n"
                         "Got:\n%s" % raw_msg)

=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2011-11-09 11:50:17 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -25,10 +25,10 @@
     export_as_webservice_entry,
     export_read_operation,
     exported,
-    operation_for_version,
     operation_parameters,
+    operation_returns_entry,
     operation_returns_collection_of,
-    operation_returns_entry,
+    operation_for_version,
     )
 from lazr.restful.fields import (
     Reference,
@@ -50,12 +50,12 @@
 from lp.app.validators.name import name_validator
 from lp.app.validators.url import builder_url_validator
 from lp.registry.interfaces.role import IHasOwner
+from lp.soyuz.interfaces.processor import IProcessor
 from lp.services.fields import (
     Description,
     PersonChoice,
     Title,
     )
-from lp.soyuz.interfaces.processor import IProcessor
 
 
 class BuildDaemonError(Exception):
@@ -195,8 +195,6 @@
 
     def setSlaveForTesting(proxy):
         """Sets the RPC proxy through which to operate the build slave."""
-        # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this.
-        # It's a trap.  See bug for details.
 
     def verifySlaveBuildCookie(slave_build_id):
         """Verify that a slave's build cookie is consistent.

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2011-12-08 11:39:10 +0000
+++ lib/lp/buildmaster/manager.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Soyuz buildd slave manager logic."""
@@ -23,6 +23,10 @@
 from zope.component import getUtility
 
 from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.interfaces.buildfarmjobbehavior import (
+    BuildBehaviorMismatch,
+    )
+from lp.buildmaster.model.builder import Builder
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     BuildSlaveFailure,
@@ -30,11 +34,6 @@
     CannotFetchFile,
     CannotResumeHost,
     )
-from lp.buildmaster.interfaces.buildfarmjobbehavior import (
-    BuildBehaviorMismatch,
-    )
-from lp.buildmaster.model.builder import Builder
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 
 
 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
@@ -112,17 +111,13 @@
     # algorithm for polling.
     SCAN_INTERVAL = 15
 
-    def __init__(self, builder_name, logger, clock=None):
+    def __init__(self, builder_name, logger):
         self.builder_name = builder_name
         self.logger = logger
-        if clock is None:
-            clock = reactor
-        self._clock = clock
 
     def startCycle(self):
         """Scan the builder and dispatch to it or deal with failures."""
         self.loop = LoopingCall(self.singleCycle)
-        self.loop.clock = self._clock
         self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
         return self.stopping_deferred
 
@@ -143,58 +138,51 @@
         1. Print the error in the log
         2. Increment and assess failure counts on the builder and job.
         """
-        # Since this is a failure path, we could be in a broken
-        # transaction.  Get us a fresh one.
+        # Make sure that pending database updates are removed as it
+        # could leave the database in an inconsistent state (e.g. The
+        # job says it's running but the buildqueue has no builder set).
         transaction.abort()
 
         # If we don't recognise the exception include a stack trace with
         # the error.
         error_message = failure.getErrorMessage()
-        familiar_error = failure.check(
+        if failure.check(
             BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch,
-            CannotResumeHost, BuildDaemonError, CannotFetchFile)
-        if familiar_error:
-            self.logger.info(
-                "Scanning %s failed with: %s",
-                self.builder_name, error_message)
+            CannotResumeHost, BuildDaemonError, CannotFetchFile):
+            self.logger.info("Scanning %s failed with: %s" % (
+                self.builder_name, error_message))
         else:
-            self.logger.info(
-                "Scanning %s failed with: %s\n%s",
+            self.logger.info("Scanning %s failed with: %s\n%s" % (
                 self.builder_name, failure.getErrorMessage(),
-                failure.getTraceback())
+                failure.getTraceback()))
 
         # Decide if we need to terminate the job or fail the
         # builder.
         try:
             builder = get_builder(self.builder_name)
-            transaction.commit()
-
-            with DatabaseTransactionPolicy(read_only=False):
-                builder.gotFailure()
-
-                if builder.currentjob is None:
-                    self.logger.info(
-                        "Builder %s failed a probe, count: %s",
-                        self.builder_name, builder.failure_count)
-                else:
-                    build_farm_job = builder.getCurrentBuildFarmJob()
-                    build_farm_job.gotFailure()
-                    self.logger.info(
-                        "builder %s failure count: %s, "
-                        "job '%s' failure count: %s",
+            builder.gotFailure()
+            if builder.currentjob is not None:
+                build_farm_job = builder.getCurrentBuildFarmJob()
+                build_farm_job.gotFailure()
+                self.logger.info(
+                    "builder %s failure count: %s, "
+                    "job '%s' failure count: %s" % (
                         self.builder_name,
                         builder.failure_count,
                         build_farm_job.title,
-                        build_farm_job.failure_count)
-
-                assessFailureCounts(builder, failure.getErrorMessage())
-                transaction.commit()
+                        build_farm_job.failure_count))
+            else:
+                self.logger.info(
+                    "Builder %s failed a probe, count: %s" % (
+                        self.builder_name, builder.failure_count))
+            assessFailureCounts(builder, failure.getErrorMessage())
+            transaction.commit()
         except:
             # Catastrophic code failure! Not much we can do.
-            transaction.abort()
             self.logger.error(
                 "Miserable failure when trying to examine failure counts:\n",
                 exc_info=True)
+            transaction.abort()
 
     def checkCancellation(self, builder):
         """See if there is a pending cancellation request.
@@ -248,9 +236,14 @@
         """
         # 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)
 
         def status_updated(ignored):
+            # Commit the changes done while possibly rescuing jobs, to
+            # avoid holding table locks.
+            transaction.commit()
+
             # See if we think there's an active build on the builder.
             buildqueue = self.builder.getBuildQueue()
 
@@ -260,10 +253,14 @@
                 return self.builder.updateBuild(buildqueue)
 
         def build_updated(ignored):
+            # Commit changes done while updating the build, to avoid
+            # holding table locks.
+            transaction.commit()
+
             # If the builder is in manual mode, don't dispatch anything.
             if self.builder.manual:
                 self.logger.debug(
-                    '%s is in manual mode, not dispatching.',
+                    '%s is in manual mode, not dispatching.' %
                     self.builder.name)
                 return
 
@@ -281,33 +278,22 @@
                 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)
+                        "%s was made unavailable, resetting attached "
+                        "job" % self.builder.name)
+                    job.reset()
                     transaction.commit()
-                    with DatabaseTransactionPolicy(read_only=False):
-                        job.reset()
-                        transaction.commit()
                 return
 
             # See if there is a job we can dispatch to the builder slave.
 
-            # XXX JeroenVermeulen 2011-10-11, bug=872112: The job's
-            # failure count will be reset once the job has started
-            # successfully.  Because of intervening commits, you may see
-            # a build with a nonzero failure count that's actually going
-            # to succeed later (and have a failure count of zero).  Or
-            # it may fail yet end up with a lower failure count than you
-            # saw earlier.
             d = self.builder.findAndStartJob()
 
             def job_started(candidate):
                 if self.builder.currentjob is not None:
                     # After a successful dispatch we can reset the
                     # failure_count.
+                    self.builder.resetFailureCount()
                     transaction.commit()
-                    with DatabaseTransactionPolicy(read_only=False):
-                        self.builder.resetFailureCount()
-                        transaction.commit()
                     return self.builder.slave
                 else:
                     return None
@@ -386,7 +372,6 @@
         self.logger = self._setupLogger()
         self.new_builders_scanner = NewBuildersScanner(
             manager=self, clock=clock)
-        self.transaction_policy = DatabaseTransactionPolicy(read_only=True)
 
     def _setupLogger(self):
         """Set up a 'slave-scanner' logger that redirects to twisted.
@@ -405,28 +390,16 @@
         logger.setLevel(level)
         return logger
 
-    def enterReadOnlyDatabasePolicy(self):
-        """Set the database transaction policy to read-only.
-
-        Any previously pending changes are committed first.
-        """
-        transaction.commit()
-        self.transaction_policy.__enter__()
-
-    def exitReadOnlyDatabasePolicy(self, *args):
-        """Reset database transaction policy to the default read-write."""
-        self.transaction_policy.__exit__(None, None, None)
-
     def startService(self):
         """Service entry point, called when the application starts."""
+
+        # Get a list of builders and set up scanners on each one.
+
         # Avoiding circular imports.
         from lp.buildmaster.interfaces.builder import IBuilderSet
-
-        self.enterReadOnlyDatabasePolicy()
-
-        # Get a list of builders and set up scanners on each one.
-        self.addScanForBuilders(
-            [builder.name for builder in getUtility(IBuilderSet)])
+        builder_set = getUtility(IBuilderSet)
+        builders = [builder.name for builder in builder_set]
+        self.addScanForBuilders(builders)
         self.new_builders_scanner.scheduleScan()
 
         # Events will now fire in the SlaveScanner objects to scan each
@@ -447,7 +420,6 @@
         # stopped, so we can wait on them all at once here before
         # exiting.
         d = defer.DeferredList(deferreds, consumeErrors=True)
-        d.addCallback(self.exitReadOnlyDatabasePolicy)
         return d
 
     def addScanForBuilders(self, builders):

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2011-12-13 13:33:04 +0000
+++ lib/lp/buildmaster/model/builder.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009,2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -76,7 +76,6 @@
     specific_job_classes,
     )
 from lp.registry.interfaces.person import validate_public_person
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.propertycache import (
@@ -546,8 +545,6 @@
 
     def setSlaveForTesting(self, proxy):
         """See IBuilder."""
-        # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this.
-        # It's a trap.  See bug for details.
         self._testing_slave = proxy
         del get_property_cache(self).slave
 
@@ -676,13 +673,10 @@
                 bytes_written = out_file.tell()
                 out_file.seek(0)
 
-                transaction.commit()
-                with DatabaseTransactionPolicy(read_only=False):
-                    library_file = getUtility(ILibraryFileAliasSet).create(
-                        filename, bytes_written, out_file,
-                        contentType=filenameToContentType(filename),
-                        restricted=private)
-                    transaction.commit()
+                library_file = getUtility(ILibraryFileAliasSet).create(
+                    filename, bytes_written, out_file,
+                    contentType=filenameToContentType(filename),
+                    restricted=private)
             finally:
                 # Remove the temporary file.  getFile() closes the file
                 # object.
@@ -720,7 +714,7 @@
     def acquireBuildCandidate(self):
         """Acquire a build candidate in an atomic fashion.
 
-        When retrieving a candidate we need to mark it as building
+        When retrieiving a candidate we need to mark it as building
         immediately so that it is not dispatched by another builder in the
         build manager.
 
@@ -730,15 +724,12 @@
         can be in this code at the same time.
 
         If there's ever more than one build manager running at once, then
-        this code will need some sort of mutex, or run in a single
-        transaction.
+        this code will need some sort of mutex.
         """
         candidate = self._findBuildCandidate()
         if candidate is not None:
+            candidate.markAsBuilding(self)
             transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                candidate.markAsBuilding(self)
-                transaction.commit()
         return candidate
 
     def _findBuildCandidate(self):
@@ -801,17 +792,13 @@
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         candidate_jobs = store.execute(query).get_all()
 
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            for (candidate_id,) in candidate_jobs:
-                candidate = getUtility(IBuildQueueSet).get(candidate_id)
-                job_class = job_classes[candidate.job_type]
-                candidate_approved = job_class.postprocessCandidate(
-                    candidate, logger)
-                if candidate_approved:
-                    transaction.commit()
-                    return candidate
-            transaction.commit()
+        for (candidate_id,) in candidate_jobs:
+            candidate = getUtility(IBuildQueueSet).get(candidate_id)
+            job_class = job_classes[candidate.job_type]
+            candidate_approved = job_class.postprocessCandidate(
+                candidate, logger)
+            if candidate_approved:
+                return candidate
 
         return None
 

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2011-10-11 07:15:10 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -16,8 +16,8 @@
 import socket
 import xmlrpclib
 
-import transaction
 from twisted.internet import defer
+
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
@@ -32,7 +32,6 @@
     IBuildFarmJobBehavior,
     )
 from lp.services import encoding
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 
 
@@ -71,25 +70,6 @@
         if slave_build_cookie != expected_cookie:
             raise CorruptBuildCookie("Invalid slave build cookie.")
 
-    def _getBuilderStatusHandler(self, status_text, logger):
-        """Look up the handler method for a given builder status.
-
-        If status is not a known one, logs an error and returns None.
-        """
-        builder_status_handlers = {
-            'BuilderStatus.IDLE': self.updateBuild_IDLE,
-            'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
-            'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
-            'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
-            'BuilderStatus.WAITING': self.updateBuild_WAITING,
-            }
-        handler = builder_status_handlers.get(status_text)
-        if handler is None:
-            logger.critical(
-                "Builder on %s returned unknown status %s; failing it.",
-                self._builder.url, status_text)
-        return handler
-
     def updateBuild(self, queueItem):
         """See `IBuildFarmJobBehavior`."""
         logger = logging.getLogger('slave-scanner')
@@ -97,7 +77,6 @@
         d = self._builder.slaveStatus()
 
         def got_failure(failure):
-            transaction.abort()
             failure.trap(xmlrpclib.Fault, socket.error)
             info = failure.value
             info = ("Could not contact the builder %s, caught a (%s)"
@@ -105,22 +84,27 @@
             raise BuildSlaveFailure(info)
 
         def got_status(slave_status):
+            builder_status_handlers = {
+                'BuilderStatus.IDLE': self.updateBuild_IDLE,
+                'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
+                'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
+                'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
+                'BuilderStatus.WAITING': self.updateBuild_WAITING,
+                }
+
             builder_status = slave_status['builder_status']
-            status_handler = self._getBuilderStatusHandler(
-                builder_status, logger)
-            if status_handler is None:
-                error = (
+            if builder_status not in builder_status_handlers:
+                logger.critical(
+                    "Builder on %s returned unknown status %s, failing it"
+                    % (self._builder.url, builder_status))
+                self._builder.failBuilder(
                     "Unknown status code (%s) returned from status() probe."
                     % builder_status)
-                transaction.commit()
-                with DatabaseTransactionPolicy(read_only=False):
-                    self._builder.failBuilder(error)
-                    # XXX: This will leave the build and job in a bad
-                    # state, but should never be possible since our
-                    # builder statuses are known.
-                    queueItem._builder = None
-                    queueItem.setDateStarted(None)
-                    transaction.commit()
+                # XXX: This will leave the build and job in a bad state, but
+                # should never be possible, since our builder statuses are
+                # known.
+                queueItem._builder = None
+                queueItem.setDateStarted(None)
                 return
 
             # Since logtail is a xmlrpclib.Binary container and it is
@@ -130,8 +114,9 @@
             # will simply remove the proxy.
             logtail = removeSecurityProxy(slave_status.get('logtail'))
 
+            method = builder_status_handlers[builder_status]
             return defer.maybeDeferred(
-                status_handler, queueItem, slave_status, logtail, logger)
+                method, queueItem, slave_status, logtail, logger)
 
         d.addErrback(got_failure)
         d.addCallback(got_status)
@@ -143,32 +128,22 @@
         Log this and reset the record.
         """
         logger.warn(
-            "Builder %s forgot about buildqueue %d -- "
-            "resetting buildqueue record.",
-            queueItem.builder.url, queueItem.id)
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            queueItem.reset()
-            transaction.commit()
+            "Builder %s forgot about buildqueue %d -- resetting buildqueue "
+            "record" % (queueItem.builder.url, queueItem.id))
+        queueItem.reset()
 
     def updateBuild_BUILDING(self, queueItem, slave_status, logtail, logger):
         """Build still building, collect the logtail"""
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            if queueItem.job.status != JobStatus.RUNNING:
-                queueItem.job.start()
-            queueItem.logtail = encoding.guess(str(logtail))
-            transaction.commit()
+        if queueItem.job.status != JobStatus.RUNNING:
+            queueItem.job.start()
+        queueItem.logtail = encoding.guess(str(logtail))
 
     def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger):
         """Build was ABORTED.
 
         Master-side should wait until the slave finish the process correctly.
         """
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            queueItem.logtail = "Waiting for slave process to be terminated"
-            transaction.commit()
+        queueItem.logtail = "Waiting for slave process to be terminated"
 
     def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger):
         """ABORTING process has successfully terminated.
@@ -176,16 +151,11 @@
         Clean the builder for another jobs.
         """
         d = queueItem.builder.cleanSlave()
-
         def got_cleaned(ignored):
-            transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                queueItem.builder = None
-                if queueItem.job.status != JobStatus.FAILED:
-                    queueItem.job.fail()
-                queueItem.specific_job.jobAborted()
-                transaction.commit()
-
+            queueItem.builder = None
+            if queueItem.job.status != JobStatus.FAILED:
+                queueItem.job.fail()
+            queueItem.specific_job.jobAborted()
         return d.addCallback(got_cleaned)
 
     def extractBuildStatus(self, slave_status):

=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2011-12-08 11:39:10 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -9,11 +9,11 @@
     ]
 
 
-from cStringIO import StringIO
 import datetime
 import logging
 import os.path
 
+from cStringIO import StringIO
 from lazr.delegates import delegates
 import pytz
 from storm.expr import Desc
@@ -24,7 +24,6 @@
     Storm,
     Unicode,
     )
-import transaction
 from zope.component import getUtility
 from zope.interface import (
     classProvides,
@@ -44,8 +43,8 @@
     MAIN_STORE,
     )
 from lp.buildmaster.enums import (
+    BuildStatus,
     BuildFarmJobType,
-    BuildStatus,
     )
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSource
 from lp.buildmaster.interfaces.packagebuild import (
@@ -58,8 +57,9 @@
     BuildFarmJobDerived,
     )
 from lp.buildmaster.model.buildqueue import BuildQueue
-from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
+from lp.registry.interfaces.pocket import (
+    PackagePublishingPocket,
+    )
 from lp.soyuz.adapters.archivedependencies import (
     default_component_dependency_name,
     )
@@ -179,24 +179,19 @@
     def storeBuildInfo(build, librarian, slave_status):
         """See `IPackageBuild`."""
         def got_log(lfa_id):
-            dependencies = slave_status.get('dependencies')
-            if dependencies is not None:
-                dependencies = unicode(dependencies)
-
             # log, builder and date_finished are read-only, so we must
             # currently remove the security proxy to set them.
             naked_build = removeSecurityProxy(build)
-
-            transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                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)
-                build.dependencies = dependencies
-                transaction.commit()
+            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
 
         d = build.getLogFromSlave(build)
         return d.addCallback(got_log)
@@ -297,41 +292,22 @@
 
     def handleStatus(self, status, librarian, slave_status):
         """See `IPackageBuild`."""
-        # Avoid circular imports.
         from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME
-
         logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
         send_notification = status in self.ALLOWED_STATUS_NOTIFICATIONS
         method = getattr(self, '_handleStatus_' + status, None)
         if method is None:
-            logger.critical(
-                "Unknown BuildStatus '%s' for builder '%s'",
-                status, self.buildqueue_record.builder.url)
-            return None
-
+            logger.critical("Unknown BuildStatus '%s' for builder '%s'"
+                            % (status, self.buildqueue_record.builder.url))
+            return
         d = method(librarian, slave_status, logger, send_notification)
         return d
 
-    def _destroy_buildqueue_record(self, unused_arg):
-        """Destroy this build's `BuildQueue` record."""
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            self.buildqueue_record.destroySelf()
-            transaction.commit()
-
     def _release_builder_and_remove_queue_item(self):
         # Release the builder for another job.
         d = self.buildqueue_record.builder.cleanSlave()
         # Remove BuildQueue record.
-        return d.addCallback(self._destroy_buildqueue_record)
-
-    def _notify_if_appropriate(self, appropriate=True, extra_info=None):
-        """If `appropriate`, call `self.notify` in a write transaction."""
-        if appropriate:
-            transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                self.notify(extra_info=extra_info)
-                transaction.commit()
+        return d.addCallback(lambda x: self.buildqueue_record.destroySelf())
 
     def _handleStatus_OK(self, librarian, slave_status, logger,
                          send_notification):
@@ -347,19 +323,16 @@
             self.buildqueue_record.specific_job.build.title,
             self.buildqueue_record.builder.name))
 
-        # If this is a binary package build for a source that is no
-        # longer published, discard it.
+        # If this is a binary package build, discard it if its source is
+        # no longer published.
         if self.build_farm_job_type == BuildFarmJobType.PACKAGEBUILD:
             build = self.buildqueue_record.specific_job.build
             if not build.current_source_publication:
-                transaction.commit()
-                with DatabaseTransactionPolicy(read_only=False):
-                    build.status = BuildStatus.SUPERSEDED
-                    transaction.commit()
+                build.status = BuildStatus.SUPERSEDED
                 return self._release_builder_and_remove_queue_item()
 
-        # Explode rather than collect a binary that is denied in this
-        # distroseries/pocket.
+        # Explode before collect a binary that is denied in this
+        # distroseries/pocket
         if not self.archive.allowUpdatesToReleasePocket():
             assert self.distro_series.canUploadToPocket(self.pocket), (
                 "%s (%s) can not be built for pocket %s: illegal status"
@@ -404,26 +377,18 @@
             # files from the slave.
             if successful_copy_from_slave:
                 logger.info(
-                    "Gathered %s %d completely. "
-                    "Moving %s to uploader queue.",
-                    self.__class__.__name__, self.id, upload_leaf)
+                    "Gathered %s %d completely. Moving %s to uploader queue."
+                    % (self.__class__.__name__, self.id, upload_leaf))
                 target_dir = os.path.join(root, "incoming")
-                resulting_status = BuildStatus.UPLOADING
+                self.status = BuildStatus.UPLOADING
             else:
                 logger.warning(
-                    "Copy from slave for build %s was unsuccessful.",
-                    self.id)
+                    "Copy from slave for build %s was unsuccessful.", self.id)
+                self.status = BuildStatus.FAILEDTOUPLOAD
+                if send_notification:
+                    self.notify(
+                        extra_info='Copy from slave was unsuccessful.')
                 target_dir = os.path.join(root, "failed")
-                resulting_status = BuildStatus.FAILEDTOUPLOAD
-
-            transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                self.status = resulting_status
-                transaction.commit()
-
-            if not successful_copy_from_slave:
-                self._notify_if_appropriate(
-                    send_notification, "Copy from slave was unsuccessful.")
 
             if not os.path.exists(target_dir):
                 os.mkdir(target_dir)
@@ -431,6 +396,10 @@
             # Release the builder for another job.
             d = self._release_builder_and_remove_queue_item()
 
+            # Commit so there are no race conditions with archiveuploader
+            # about self.status.
+            Store.of(self).commit()
+
             # Move the directory used to grab the binaries into
             # the incoming directory so the upload processor never
             # sees half-finished uploads.
@@ -454,15 +423,14 @@
         set the job status as FAILEDTOBUILD, store available info and
         remove Buildqueue entry.
         """
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            self.status = BuildStatus.FAILEDTOBUILD
-            transaction.commit()
+        self.status = BuildStatus.FAILEDTOBUILD
 
         def build_info_stored(ignored):
-            self._notify_if_appropriate(send_notification)
+            if send_notification:
+                self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(self._destroy_buildqueue_record)
+            return d.addCallback(
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -480,9 +448,11 @@
         def build_info_stored(ignored):
             logger.critical("***** %s is MANUALDEPWAIT *****"
                             % self.buildqueue_record.builder.name)
-            self._notify_if_appropriate(send_notification)
+            if send_notification:
+                self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(self._destroy_buildqueue_record)
+            return d.addCallback(
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
@@ -498,24 +468,17 @@
         self.status = BuildStatus.CHROOTWAIT
 
         def build_info_stored(ignored):
-            logger.critical(
-                "***** %s is CHROOTWAIT *****",
-                self.buildqueue_record.builder.name)
-
-            self._notify_if_appropriate(send_notification)
+            logger.critical("***** %s is CHROOTWAIT *****" %
+                            self.buildqueue_record.builder.name)
+            if send_notification:
+                self.notify()
             d = self.buildqueue_record.builder.cleanSlave()
-            return d.addCallback(self._destroy_buildqueue_record)
+            return d.addCallback(
+                lambda x: self.buildqueue_record.destroySelf())
 
         d = self.storeBuildInfo(self, librarian, slave_status)
         return d.addCallback(build_info_stored)
 
-    def _reset_buildqueue_record(self, ignored_arg=None):
-        """Reset the `BuildQueue` record, in a write transaction."""
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=False):
-            self.buildqueue_record.reset()
-            transaction.commit()
-
     def _handleStatus_BUILDERFAIL(self, librarian, slave_status, logger,
                                   send_notification):
         """Handle builder failures.
@@ -529,8 +492,11 @@
         self.buildqueue_record.builder.failBuilder(
             "Builder returned BUILDERFAIL when asked for its status")
 
+        def build_info_stored(ignored):
+            # simply reset job
+            self.buildqueue_record.reset()
         d = self.storeBuildInfo(self, librarian, slave_status)
-        return d.addCallback(self._reset_buildqueue_record)
+        return d.addCallback(build_info_stored)
 
     def _handleStatus_GIVENBACK(self, librarian, slave_status, logger,
                                 send_notification):
@@ -550,7 +516,7 @@
             # the next Paris Summit, infinity has some ideas about how
             # to use this content. For now we just ensure it's stored.
             d = self.buildqueue_record.builder.cleanSlave()
-            self._reset_buildqueue_record()
+            self.buildqueue_record.reset()
             return d
 
         d = self.storeBuildInfo(self, librarian, slave_status)

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2011-12-13 13:33:04 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Builder features."""
@@ -8,14 +8,13 @@
 import tempfile
 import xmlrpclib
 
-from lpbuildd.slave import BuilderStatus
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
     AsynchronousDeferredRunTestForBrokenTwisted,
     SynchronousDeferredRunTest,
     )
-import transaction
+
 from twisted.internet.defer import (
     CancelledError,
     DeferredList,
@@ -23,12 +22,15 @@
 from twisted.internet.task import Clock
 from twisted.python.failure import Failure
 from twisted.web.client import getPage
+
 from zope.component import getUtility
 from zope.security.proxy import (
     isinstance as zope_isinstance,
     removeSecurityProxy,
     )
 
+from lpbuildd.slave import BuilderStatus
+
 from canonical.config import config
 from canonical.database.sqlbase import flush_database_updates
 from canonical.launchpad.webapp.interfaces import (
@@ -43,7 +45,6 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.builder import (
     CannotFetchFile,
-    CannotResumeHost,
     IBuilder,
     IBuilderSet,
     )
@@ -51,6 +52,7 @@
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.interfaces.builder import CannotResumeHost
 from lp.buildmaster.model.builder import (
     BuilderSlave,
     ProxyWithConnectionTimeout,
@@ -72,8 +74,6 @@
     TrivialBehavior,
     WaitingSlave,
     )
-from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.log.logger import BufferLogger
 from lp.soyuz.enums import (
@@ -155,7 +155,7 @@
         d = lostbuilding_builder.updateStatus(BufferLogger())
         def check_slave_status(failure):
             self.assertIn('abort', slave.call_log)
-            # 'Fault' comes from the LostBuildingBrokenSlave.  This is
+            # 'Fault' comes from the LostBuildingBrokenSlave, this is
             # just testing that the value is passed through.
             self.assertIsInstance(failure.value, xmlrpclib.Fault)
         return d.addBoth(check_slave_status)
@@ -534,26 +534,6 @@
         # And the old_candidate is superseded:
         self.assertEqual(BuildStatus.SUPERSEDED, build.status)
 
-    def test_findBuildCandidate_postprocesses_in_read_write_policy(self):
-        # _findBuildCandidate invokes BuildFarmJob.postprocessCandidate,
-        # which may modify the database.  This happens in a read-write
-        # transaction even if _findBuildCandidate itself runs in a
-        # read-only transaction policy.
-
-        # PackageBuildJob.postprocessCandidate will attempt to delete
-        # security builds.
-        pub = self.publisher.getPubSource(
-            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
-            archive=self.factory.makeArchive(),
-            pocket=PackagePublishingPocket.SECURITY)
-        pub.createMissingBuilds()
-        transaction.commit()
-        with DatabaseTransactionPolicy(read_only=True):
-            removeSecurityProxy(self.frog_builder)._findBuildCandidate()
-            # The test is that this passes without a "transaction is
-            # read-only" error.
-            transaction.commit()
-
     def test_acquireBuildCandidate_marks_building(self):
         # acquireBuildCandidate() should call _findBuildCandidate and
         # mark the build as building.

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2011-12-08 11:51:59 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2011-12-19 21:48:31 +0000
@@ -1,36 +1,40 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the renovated slave scanner aka BuilddManager."""
 
-from collections import namedtuple
 import os
 import signal
 import time
 import xmlrpclib
 
-from lpbuildd.tests import BuilddSlaveTestSetup
 from testtools.deferredruntest import (
     assert_fails_with,
     AsynchronousDeferredRunTest,
     )
+
 import transaction
+
 from twisted.internet import (
     defer,
     reactor,
     task,
     )
-from twisted.internet.task import deferLater
+from twisted.internet.task import (
+    deferLater,
+    )
 from twisted.python.failure import Failure
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lpbuildd.tests import BuilddSlaveTestSetup
+
 from canonical.config import config
-from canonical.database.constants import UTC_NOW
 from canonical.launchpad.ftests import (
     ANONYMOUS,
     login,
     )
+from lp.services.log.logger import BufferLogger
 from canonical.testing.layers import (
     LaunchpadScriptLayer,
     LaunchpadZopelessLayer,
@@ -46,18 +50,14 @@
     SlaveScanner,
     )
 from lp.buildmaster.model.builder import Builder
-from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.harness import BuilddManagerTestSetup
 from lp.buildmaster.tests.mock_slaves import (
     BrokenSlave,
     BuildingSlave,
     make_publisher,
     OkSlave,
-    WaitingSlave,
     )
 from lp.registry.interfaces.distribution import IDistributionSet
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
-from lp.services.log.logger import BufferLogger
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.testing import (
     TestCase,
@@ -65,13 +65,10 @@
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
-from lp.testing.sampledata import (
-    BOB_THE_BUILDER_NAME,
-    FROG_THE_BUILDER_NAME,
-    )
-
-
-class TestSlaveScannerScan(TestCaseWithFactory):
+from lp.testing.sampledata import BOB_THE_BUILDER_NAME
+
+
+class TestSlaveScannerScan(TestCase):
     """Tests `SlaveScanner.scan` method.
 
     This method uses the old framework for scanning and dispatching builds.
@@ -86,8 +83,6 @@
         'bob' builder.
         """
         super(TestSlaveScannerScan, self).setUp()
-        self.read_only = DatabaseTransactionPolicy(read_only=True)
-
         # Creating the required chroots needed for dispatching.
         test_publisher = make_publisher()
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
@@ -95,15 +90,6 @@
         test_publisher.setUpDefaultDistroSeries(hoary)
         test_publisher.addFakeChroots()
 
-    def _enterReadOnly(self):
-        """Go into read-only transaction policy."""
-        self.read_only.__enter__()
-        self.addCleanup(self._exitReadOnly)
-
-    def _exitReadOnly(self):
-        """Leave read-only transaction policy."""
-        self.read_only.__exit__(None, None, None)
-
     def _resetBuilder(self, builder):
         """Reset the given builder and its job."""
 
@@ -114,23 +100,6 @@
 
         transaction.commit()
 
-    def getFreshBuilder(self, slave=None, name=BOB_THE_BUILDER_NAME,
-                        failure_count=0):
-        """Return a builder.
-
-        The builder is taken from sample data, but reset to a usable state.
-        Be careful: this is not a proper factory method.  Identical calls
-        return (and reset) the same builder.  Don't rely on that though;
-        maybe someday we'll have a proper factory here.
-        """
-        if slave is None:
-            slave = OkSlave()
-        builder = getUtility(IBuilderSet)[name]
-        self._resetBuilder(builder)
-        builder.setSlaveForTesting(slave)
-        builder.failure_count = failure_count
-        return builder
-
     def assertBuildingJob(self, job, builder, logtail=None):
         """Assert the given job is building on the given builder."""
         from lp.services.job.interfaces.job import JobStatus
@@ -145,14 +114,14 @@
         self.assertEqual(build.status, BuildStatus.BUILDING)
         self.assertEqual(job.logtail, logtail)
 
-    def _getScanner(self, builder_name=None, clock=None):
+    def _getScanner(self, builder_name=None):
         """Instantiate a SlaveScanner object.
 
         Replace its default logging handler by a testing version.
         """
         if builder_name is None:
             builder_name = BOB_THE_BUILDER_NAME
-        scanner = SlaveScanner(builder_name, BufferLogger(), clock=clock)
+        scanner = SlaveScanner(builder_name, BufferLogger())
         scanner.logger.name = 'slave-scanner'
 
         return scanner
@@ -168,15 +137,17 @@
     def testScanDispatchForResetBuilder(self):
         # A job gets dispatched to the sampledata builder after it's reset.
 
-        # Obtain a builder.   Initialize failure count to 1 so that
-        # _checkDispatch can make sure that a successful dispatch resets
-        # the count to 0.
-        builder = self.getFreshBuilder(failure_count=1)
+        # Reset sampledata builder.
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
+        # Set this to 1 here so that _checkDispatch can make sure it's
+        # reset to 0 after a successful dispatch.
+        builder.failure_count = 1
 
         # Run 'scan' and check its result.
         self.layer.txn.commit()
         self.layer.switchDbUser(config.builddmaster.dbuser)
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkDispatch, builder)
@@ -189,18 +160,20 @@
         to the asynchonous dispatcher and the builder remained active
         and IDLE.
         """
-        self.assertIs(None, slave, "Unexpected slave.")
+        self.assertTrue(slave is None, "Unexpected slave.")
 
         builder = getUtility(IBuilderSet).get(builder.id)
         self.assertTrue(builder.builderok)
-        self.assertIs(None, builder.currentjob)
+        self.assertTrue(builder.currentjob is None)
 
     def testNoDispatchForMissingChroots(self):
         # When a required chroot is not present the `scan` method
         # should not return any `RecordingSlaves` to be processed
         # and the builder used should remain active and IDLE.
 
-        builder = self.getFreshBuilder()
+        # Reset sampledata builder.
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
 
         # Remove hoary/i386 chroot.
         login('[e-mail]')
@@ -213,7 +186,6 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.singleCycle)
         d.addCallback(self._checkNoDispatch, builder)
@@ -255,7 +227,6 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobRescued, builder, job)
@@ -291,27 +262,25 @@
 
         # Run 'scan' and check its result.
         self.layer.switchDbUser(config.builddmaster.dbuser)
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobUpdated, builder, job)
         return d
 
     def test_scan_with_nothing_to_dispatch(self):
-        builder = self.factory.makeBuilder()
+        factory = LaunchpadObjectFactory()
+        builder = factory.makeBuilder()
         builder.setSlaveForTesting(OkSlave())
-        transaction.commit()
-        self._enterReadOnly()
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
         return d.addCallback(self._checkNoDispatch, builder)
 
     def test_scan_with_manual_builder(self):
         # Reset sampledata builder.
-        builder = self.getFreshBuilder()
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
         builder.manual = True
-        transaction.commit()
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = scanner.scan()
         d.addCallback(self._checkNoDispatch, builder)
@@ -319,10 +288,10 @@
 
     def test_scan_with_not_ok_builder(self):
         # Reset sampledata builder.
-        builder = self.getFreshBuilder()
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
         builder.builderok = False
-        transaction.commit()
-        self._enterReadOnly()
         scanner = self._getScanner()
         d = scanner.scan()
         # Because the builder is not ok, we can't use _checkNoDispatch.
@@ -331,27 +300,25 @@
         return d
 
     def test_scan_of_broken_slave(self):
-        builder = self.getFreshBuilder(slave=BrokenSlave())
-        transaction.commit()
-        self._enterReadOnly()
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(BrokenSlave())
+        builder.failure_count = 0
         scanner = self._getScanner(builder_name=builder.name)
         d = scanner.scan()
         return assert_fails_with(d, xmlrpclib.Fault)
 
     def _assertFailureCounting(self, builder_count, job_count,
                                expected_builder_count, expected_job_count):
-        # Avoid circular imports.
-        from lp.buildmaster import manager as manager_module
-
         # If scan() fails with an exception, failure_counts should be
         # incremented.  What we do with the results of the failure
         # counts is tested below separately, this test just makes sure that
         # scan() is setting the counts.
         def failing_scan():
             return defer.fail(Exception("fake exception"))
-
         scanner = self._getScanner()
         scanner.scan = failing_scan
+        from lp.buildmaster import manager as manager_module
         self.patch(manager_module, 'assessFailureCounts', FakeMethod())
         builder = getUtility(IBuilderSet)[scanner.builder_name]
 
@@ -499,60 +466,6 @@
         d.addCallback(check_cancelled, builder, buildqueue)
         return d
 
-    def makeFakeFailure(self):
-        """Produce a fake failure for use with SlaveScanner._scanFailed."""
-        FakeFailure = namedtuple('FakeFailure', ['getErrorMessage', 'check'])
-        return FakeFailure(
-            FakeMethod(self.factory.getUniqueString()),
-            FakeMethod(True))
-
-    def test_interleaved_success_and_failure_do_not_interfere(self):
-        # It's possible for one builder to fail while another continues
-        # to function properly.  When that happens, the failed builder
-        # may cause database changes to be rolled back.  But that does
-        # not affect the functioning builder.
-        clock = task.Clock()
-
-        broken_builder = self.getFreshBuilder(
-            slave=BrokenSlave(), name=BOB_THE_BUILDER_NAME)
-        broken_scanner = self._getScanner(builder_name=broken_builder.name)
-        good_builder = self.getFreshBuilder(
-            slave=WaitingSlave(), name=FROG_THE_BUILDER_NAME)
-        good_build = self.factory.makeBinaryPackageBuild(
-            distroarchseries=self.factory.makeDistroArchSeries())
-
-        # The good build is being handled by the good builder.
-        buildqueue = good_build.queueBuild()
-        buildqueue.builder = good_builder
-
-        removeSecurityProxy(good_build.build_farm_job).date_started = UTC_NOW
-
-        # The good builder requests information from a successful build,
-        # and up receiving it, updates the build's metadata.
-        # Our dependencies string goes into the build, and its
-        # date_finished will be set.
-        dependencies = self.factory.getUniqueString()
-        PackageBuild.storeBuildInfo(
-            good_build, None, {'dependencies': dependencies})
-        clock.advance(1)
-
-        # The broken scanner experiences a failure before the good
-        # scanner is receiving its data.  This aborts the ongoing
-        # transaction.
-        # As a somewhat weird example, if the builder changed its own
-        # title, that change will be rolled back.
-        original_broken_builder_title = broken_builder.title
-        broken_builder.title = self.factory.getUniqueString()
-        broken_scanner._scanFailed(self.makeFakeFailure())
-
-        # The work done by the good scanner is retained.  The
-        # storeBuildInfo code committed it.
-        self.assertEqual(dependencies, good_build.dependencies)
-        self.assertIsNot(None, good_build.date_finished)
-
-        # The work done by the broken scanner is rolled back.
-        self.assertEqual(original_broken_builder_title, broken_builder.title)
-
 
 class TestCancellationChecking(TestCaseWithFactory):
     """Unit tests for the checkCancellation method."""

=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2011-11-09 11:50:17 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `IPackageBuild`."""
@@ -22,7 +22,9 @@
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
     )
-from lp.archiveuploader.uploadprocessor import parse_build_upload_leaf_name
+from lp.archiveuploader.uploadprocessor import (
+    parse_build_upload_leaf_name,
+    )
 from lp.buildmaster.enums import (
     BuildFarmJobType,
     BuildStatus,
@@ -32,11 +34,12 @@
     IPackageBuildSet,
     IPackageBuildSource,
     )
-from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
-from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.pocket import (
+    PackagePublishingPocket,
+    )
 from lp.testing import (
     login,
     login_person,
@@ -282,7 +285,10 @@
 
 
 class TestHandleStatusMixin:
-    """Tests for `IPackageBuild`s handleStatus method."""
+    """Tests for `IPackageBuild`s handleStatus method.
+
+    This should be run with a Trial TestCase.
+    """
 
     layer = LaunchpadZopelessLayer
 
@@ -301,7 +307,7 @@
         self.build.buildqueue_record.setDateStarted(UTC_NOW)
         self.slave = WaitingSlave('BuildStatus.OK')
         self.slave.valid_file_hashes.append('test_file_hash')
-        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(self.slave))
+        builder.setSlaveForTesting(self.slave)
 
         # We overwrite the buildmaster root to use a temp directory.
         tempdir = tempfile.mkdtemp()
@@ -342,7 +348,7 @@
         def got_status(ignored):
             self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.status)
             self.assertResultCount(0, "failed")
-            self.assertIs(None, self.build.buildqueue_record)
+            self.assertIdentical(None, self.build.buildqueue_record)
 
         d = self.build.handleStatus('OK', None, {
             'filemap': {'/tmp/myfile.py': 'test_file_hash'},
@@ -384,10 +390,14 @@
 
         def got_status(ignored):
             if expected_notification:
-                self.assertNotEqual(
-                    0, len(pop_notifications()), "No notifications received.")
+                self.failIf(
+                    len(pop_notifications()) == 0,
+                    "No notifications received")
             else:
-                self.assertContentEqual([], pop_notifications())
+                self.failIf(
+                    len(pop_notifications()) > 0,
+                    "Notifications received")
+
         d = self.build.handleStatus(status, None, {})
         return d.addCallback(got_status)
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-12-08 16:04:13 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-12-19 21:48:31 +0000
@@ -13,8 +13,8 @@
 
 from pytz import utc
 from storm.locals import Store
-from testtools.deferredruntest import AsynchronousDeferredRunTest
 import transaction
+from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -28,7 +28,6 @@
 from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
-from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
@@ -589,11 +588,14 @@
         self.assertEquals(0, len(notifications))
 
 
-class TestBuildNotifications(TestCaseWithFactory):
+class TestBuildNotifications(TrialTestCase):
 
     layer = LaunchpadZopelessLayer
 
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
+    def setUp(self):
+        super(TestBuildNotifications, self).setUp()
+        from lp.testing.factory import LaunchpadObjectFactory
+        self.factory = LaunchpadObjectFactory()
 
     def prepare_build(self, fake_successful_upload=False):
         queue_record = self.factory.makeSourcePackageRecipeBuildJob()
@@ -606,7 +608,7 @@
                 result=True)
         queue_record.builder = self.factory.makeBuilder()
         slave = WaitingSlave('BuildStatus.OK')
-        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
+        queue_record.builder.setSlaveForTesting(slave)
         return build
 
     def assertDeferredNotifyCount(self, status, build, expected_count):
@@ -664,5 +666,5 @@
 
 
 class TestHandleStatusForSPRBuild(
-    MakeSPRecipeBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
+    MakeSPRecipeBuildMixin, TestHandleStatusMixin, TrialTestCase):
     """IPackageBuild.handleStatus works with SPRecipe builds."""

=== modified file 'lib/lp/services/database/transaction_policy.py'
--- lib/lp/services/database/transaction_policy.py	2011-10-10 06:23:12 +0000
+++ lib/lp/services/database/transaction_policy.py	2011-12-19 21:48:31 +0000
@@ -133,11 +133,8 @@
     def _isInTransaction(self):
         """Is our store currently in a transaction?"""
         pg_connection = self.store._connection._raw_connection
-        if pg_connection is None:
-            return False
-        else:
-            status = pg_connection.get_transaction_status()
-            return status != TRANSACTION_STATUS_IDLE
+        status = pg_connection.get_transaction_status()
+        return status != TRANSACTION_STATUS_IDLE
 
     def _checkNoTransaction(self, error_msg):
         """Verify that no transaction is ongoing.

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-11-14 06:36:57 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test Build features."""
@@ -10,7 +10,7 @@
 
 import pytz
 from storm.store import Store
-from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -25,7 +25,6 @@
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
-from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.tests.mock_slaves import WaitingSlave
 from lp.buildmaster.tests.test_packagebuild import (
@@ -54,7 +53,6 @@
     logout,
     TestCaseWithFactory,
     )
-from lp.testing.fakemethod import FakeMethod
 
 
 class TestBinaryPackageBuild(TestCaseWithFactory):
@@ -524,9 +522,7 @@
         self.build = gedit_src_hist.createMissingBuilds()[0]
 
         self.builder = self.factory.makeBuilder()
-        self.patch(
-            BuilderSlave, 'makeBuilderSlave',
-            FakeMethod(WaitingSlave('BuildStatus.OK')))
+        self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK'))
         self.build.buildqueue_record.markAsBuilding(self.builder)
 
     def testDependencies(self):
@@ -572,12 +568,9 @@
 
 
 class TestHandleStatusForBinaryPackageBuild(
-    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory):
+    MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TrialTestCase):
     """IPackageBuild.handleStatus works with binary builds."""
 
-    layer = LaunchpadZopelessLayer
-    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
-
 
 class TestBinaryPackageBuildWebservice(TestCaseWithFactory):
     """Test cases for BinaryPackageBuild on the webservice.

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-10-20 07:45:52 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2011-12-19 21:48:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """An `IBuildFarmJobBehavior` for `TranslationTemplatesBuildJob`.
@@ -13,10 +13,9 @@
 
 import datetime
 import os
+import pytz
 import tempfile
 
-import pytz
-import transaction
 from twisted.internet import defer
 from zope.component import getUtility
 from zope.interface import implements
@@ -29,7 +28,6 @@
     )
 from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase
 from lp.registry.interfaces.productseries import IProductSeriesSet
-from lp.services.database.transaction_policy import DatabaseTransactionPolicy
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
     )
@@ -134,16 +132,13 @@
     def storeBuildInfo(build, queue_item, build_status):
         """See `IPackageBuild`."""
         def got_log(lfa_id):
-            transaction.commit()
-            with DatabaseTransactionPolicy(read_only=False):
-                build.build.log = lfa_id
-                build.build.builder = queue_item.builder
-                build.build.date_started = queue_item.date_started
-                # XXX cprov 20060615 bug=120584: Currently buildduration
-                # includes the scanner latency.  It should really be
-                # asking the slave for the duration spent building locally.
-                build.build.date_finished = datetime.datetime.now(pytz.UTC)
-                transaction.commit()
+            build.build.log = lfa_id
+            build.build.builder = queue_item.builder
+            build.build.date_started = queue_item.date_started
+            # XXX cprov 20060615 bug=120584: Currently buildduration includes
+            # the scanner latency, it should really be asking the slave for
+            # the duration spent building locally.
+            build.build.date_finished = datetime.datetime.now(pytz.UTC)
 
         d = build.getLogFromSlave(build, queue_item)
         return d.addCallback(got_log)