Merge lp:~wgrant/launchpad/builderinteractor-updateBuild into lp:launchpad

[William Grant]

Move updateBuild from BuildFarmJobBehavior to BuilderInteractor.

Move updateBuild from BuildFarmJobBehavior to BuilderInteractor. BuildFarmJobBehaviors should override handleStatus instead, as all but TranslationTemplatesBuildBehavior already did.

This gets us closer to running BuilderInteractor without touching the database most of the time.

[Steve Kowalik (stevenk)]

My only niggle would be for a comment for the one circular import you've added, but if it's only temporary, then it's fine.

review: Approve (code)

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2013-08-27 11:35:09 +0000
+++ lib/lp/buildmaster/interactor.py	2013-08-28 11:11:24 +0000
@@ -15,6 +15,7 @@
 from urlparse import urlparse
 import xmlrpclib
 
+import transaction
 from twisted.internet import defer
 from twisted.web import xmlrpc
 from twisted.web.client import downloadPage
@@ -35,9 +36,12 @@
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
+from lp.services import encoding
 from lp.services.config import config
 from lp.services.helpers import filenameToContentType
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
+from lp.services.librarian.interfaces.client import ILibrarianClient
 from lp.services.librarian.utils import copy_and_close
 from lp.services.twistedsupport import cancel_on_timeout
 from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
@@ -610,7 +614,133 @@
 
         :return: A Deferred that fires when the slave dialog is finished.
         """
-        return self._current_build_behavior.updateBuild(queueItem)
+        logger = logging.getLogger('slave-scanner')
+
+        d = self.slaveStatus()
+
+        def got_failure(failure):
+            failure.trap(xmlrpclib.Fault, socket.error)
+            info = failure.value
+            info = ("Could not contact the builder %s, caught a (%s)"
+                    % (queueItem.builder.url, info))
+            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']
+            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)
+                # 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()
+                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)
+
+        d.addErrback(got_failure)
+        d.addCallback(got_status)
+        return d
+
+    def updateBuild_IDLE(self, queueItem, slave_status, logtail, 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, slave_status, logtail, logger):
+        """Build still building, collect the logtail"""
+        if queueItem.job.status != JobStatus.RUNNING:
+            queueItem.job.start()
+        queueItem.logtail = encoding.guess(str(logtail))
+        transaction.commit()
+
+    def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger):
+        """Build was ABORTED.
+
+        Master-side should wait until the slave finish the process correctly.
+        """
+        queueItem.logtail = "Waiting for slave process to be terminated"
+        transaction.commit()
+
+    def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger):
+        """ABORTING process has successfully terminated.
+
+        Clean the builder for another jobs.
+        """
+        d = self.cleanSlave()
+
+        def got_cleaned(ignored):
+            queueItem.builder = None
+            if queueItem.job.status != JobStatus.FAILED:
+                queueItem.job.fail()
+            queueItem.specific_job.jobAborted()
+            transaction.commit()
+        return d.addCallback(got_cleaned)
+
+    def extractBuildStatus(self, slave_status):
+        """Read build status name.
+
+        :param slave_status: build status dict as passed to the
+            updateBuild_* methods.
+        :return: the unqualified status name, e.g. "OK".
+        """
+        status_string = slave_status['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):
+        """Perform the actions needed for a slave in a WAITING state
+
+        Buildslave can be WAITING in five situations:
+
+        * Build has failed, no filemap is received (PACKAGEFAIL, DEPFAIL,
+                                                    CHROOTFAIL, BUILDERFAIL,
+                                                    ABORTED)
+
+        * Build has been built successfully (BuildStatus.OK), in this case
+          we have a 'filemap', so we can retrieve those files and store in
+          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?
+        d = self._current_build_behavior.handleStatus(
+            build_status, librarian, slave_status)
+        return d
 
     def transferSlaveFileToLibrarian(self, file_sha1, filename, private):
         """Transfer a file from the slave to the librarian.

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2013-08-22 04:07:24 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2013-08-28 11:11:24 +0000
@@ -66,11 +66,10 @@
             supposed to be.
         """
 
-    def updateBuild(queueItem):
-        """Verify the current build job status.
-
-        Perform the required actions for each state.
-
-        :param queueItem: The `BuildQueue` for the build.
-        :return: A Deferred that fires when the update is done.
+    def handleStatus(status, librarian, slave_status):
+        """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`.
         """

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2013-08-28 04:40:32 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2013-08-28 11:11:24 +0000
@@ -13,14 +13,10 @@
 import datetime
 import logging
 import os.path
-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
 
 from lp.buildmaster.enums import (
     BuildFarmJobType,
@@ -34,10 +30,7 @@
     BuildBehaviorMismatch,
     IBuildFarmJobBehavior,
     )
-from lp.services import encoding
 from lp.services.config import config
-from lp.services.job.interfaces.job import JobStatus
-from lp.services.librarian.interfaces.client import ILibrarianClient
 
 
 SLAVE_LOG_FILENAME = 'buildlog'
@@ -105,135 +98,6 @@
         self.build.setLog(lfa_id)
         transaction.commit()
 
-    def updateBuild(self, queueItem):
-        """See `IBuildFarmJobBehavior`."""
-        logger = logging.getLogger('slave-scanner')
-
-        d = self._interactor.slaveStatus()
-
-        def got_failure(failure):
-            failure.trap(xmlrpclib.Fault, socket.error)
-            info = failure.value
-            info = ("Could not contact the builder %s, caught a (%s)"
-                    % (queueItem.builder.url, info))
-            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']
-            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)
-                # 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()
-                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)
-
-        d.addErrback(got_failure)
-        d.addCallback(got_status)
-        return d
-
-    def updateBuild_IDLE(self, queueItem, slave_status, logtail, 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, slave_status, logtail, logger):
-        """Build still building, collect the logtail"""
-        if queueItem.job.status != JobStatus.RUNNING:
-            queueItem.job.start()
-        queueItem.logtail = encoding.guess(str(logtail))
-        transaction.commit()
-
-    def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger):
-        """Build was ABORTED.
-
-        Master-side should wait until the slave finish the process correctly.
-        """
-        queueItem.logtail = "Waiting for slave process to be terminated"
-        transaction.commit()
-
-    def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger):
-        """ABORTING process has successfully terminated.
-
-        Clean the builder for another jobs.
-        """
-        d = self._interactor.cleanSlave()
-
-        def got_cleaned(ignored):
-            queueItem.builder = None
-            if queueItem.job.status != JobStatus.FAILED:
-                queueItem.job.fail()
-            queueItem.specific_job.jobAborted()
-            transaction.commit()
-        return d.addCallback(got_cleaned)
-
-    def extractBuildStatus(self, slave_status):
-        """Read build status name.
-
-        :param slave_status: build status dict as passed to the
-            updateBuild_* methods.
-        :return: the unqualified status name, e.g. "OK".
-        """
-        status_string = slave_status['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):
-        """Perform the actions needed for a slave in a WAITING state
-
-        Buildslave can be WAITING in five situations:
-
-        * Build has failed, no filemap is received (PACKAGEFAIL, DEPFAIL,
-                                                    CHROOTFAIL, BUILDERFAIL,
-                                                    ABORTED)
-
-        * Build has been built successfully (BuildStatus.OK), in this case
-          we have a 'filemap', so we can retrieve those files and store in
-          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?
-        d = self.handleStatus(build_status, librarian, slave_status)
-        return d
-
     # The list of build status values for which email notifications are
     # allowed to be sent. It is up to each callback as to whether it will
     # consider sending a notification but it won't do so if the status is not
@@ -241,7 +105,7 @@
     ALLOWED_STATUS_NOTIFICATIONS = ['OK', 'PACKAGEFAIL', 'CHROOTFAIL']
 
     def handleStatus(self, status, librarian, slave_status):
-        """See `IPackageBuild`."""
+        """See `IBuildFarmJobBehavior`."""
         from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME
         logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
         send_notification = status in self.ALLOWED_STATUS_NOTIFICATIONS

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2013-08-27 10:33:01 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2013-08-28 11:11:24 +0000
@@ -251,6 +251,23 @@
 
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
 
+    def test_extractBuildStatus_baseline(self):
+        # extractBuildStatus picks the name of the build status out of a
+        # dict describing the slave's status.
+        slave_status = {'build_status': 'BuildStatus.BUILDING'}
+        interactor = BuilderInteractor(MockBuilder())
+        self.assertEqual(
+            BuildStatus.BUILDING.name,
+            interactor.extractBuildStatus(slave_status))
+
+    def test_extractBuildStatus_malformed(self):
+        # extractBuildStatus errors out when the status string is not
+        # of the form it expects.
+        slave_status = {'build_status': 'BUILDING'}
+        interactor = BuilderInteractor(MockBuilder())
+        self.assertRaises(
+            AssertionError, interactor.extractBuildStatus, slave_status)
+
     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.

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2013-08-27 11:35:09 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2013-08-28 11:11:24 +0000
@@ -78,23 +78,6 @@
         """Create a `BuildQueue` object."""
         return self.factory.makeSourcePackageRecipeBuildJob()
 
-    def test_extractBuildStatus_baseline(self):
-        # extractBuildStatus picks the name of the build status out of a
-        # dict describing the slave's status.
-        slave_status = {'build_status': 'BuildStatus.BUILDING'}
-        behavior = self._makeBehavior()
-        self.assertEqual(
-            BuildStatus.BUILDING.name,
-            behavior.extractBuildStatus(slave_status))
-
-    def test_extractBuildStatus_malformed(self):
-        # extractBuildStatus errors out when the status string is not
-        # of the form it expects.
-        slave_status = {'build_status': 'BUILDING'}
-        behavior = self._makeBehavior()
-        self.assertRaises(
-            AssertionError, behavior.extractBuildStatus, slave_status)
-
     def test_cookie_baseline(self):
         buildfarmjob = self.factory.makeTranslationTemplatesBuildJob()
 

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2013-08-19 23:23:19 +0000
+++ lib/lp/code/model/recipebuilder.py	2013-08-28 11:11:24 +0000
@@ -43,8 +43,6 @@
     ALLOWED_STATUS_NOTIFICATIONS = [
         'OK', 'PACKAGEFAIL', 'DEPFAIL', 'CHROOTFAIL']
 
-    status = None
-
     @property
     def build(self):
         return self.buildfarmjob.build

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2013-08-19 23:23:19 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2013-08-28 11:11:24 +0000
@@ -11,6 +11,7 @@
     'TranslationTemplatesBuildBehavior',
     ]
 
+import logging
 import os
 import tempfile
 
@@ -118,8 +119,8 @@
                 status['filemap'] = raw_slave_status[3]
 
     @defer.inlineCallbacks
-    def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):
-        """Deal with a finished ("WAITING" state, perversely) build job.
+    def handleStatus(self, status, librarian, slave_status, queue_item=None):
+        """Deal with a finished build job.
 
         Retrieves tarball and logs from the slave, then cleans up the
         slave so it's ready for a next job and destroys the queue item.
@@ -127,15 +128,17 @@
         If this fails for whatever unforeseen reason, a future run will
         retry it.
         """
-        build_status = self.extractBuildStatus(slave_status)
-
+        from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME
+        logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
+        if queue_item is None:
+            queue_item = self.build.buildqueue_record
         logger.info(
             "Templates generation job %s for %s finished with status %s." % (
             queue_item.specific_job.getName(),
             queue_item.specific_job.branch.bzr_identity,
-            build_status))
+            status))
 
-        if build_status == 'OK':
+        if status == 'OK':
             self.build.updateStatus(
                 BuildStatus.UPLOADING, builder=queue_item.builder)
             transaction.commit()

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2013-08-27 09:17:07 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2013-08-28 11:11:24 +0000
@@ -188,7 +188,7 @@
         d = behavior._readTarball(buildqueue, {path: path}, logging)
         return d.addCallback(got_tarball)
 
-    def test_updateBuild_WAITING_OK(self):
+    def test_handleStatus_OK(self):
         # Hopefully, a build will succeed and produce a tarball.
         behavior = self.makeBehavior()
         behavior._uploadTarball = FakeMethod()
@@ -212,8 +212,11 @@
                 'build_status': status[1],
                 'filemap': {'translation-templates.tar.gz': 'foo'},
                 }
-            return behavior.updateBuild_WAITING(
-                queue_item, slave_status, None, logging), slave_call_log
+            return (
+                behavior.handleStatus(
+                    behavior._interactor.extractBuildStatus(slave_status),
+                    None, slave_status, queue_item=queue_item),
+                slave_call_log)
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)
@@ -229,7 +232,7 @@
         d.addCallback(build_updated)
         return d
 
-    def test_updateBuild_WAITING_failed(self):
+    def test_handleStatus_failed(self):
         # Builds may also fail (and produce no tarball).
         behavior = self.makeBehavior()
         behavior._uploadTarball = FakeMethod()
@@ -247,15 +250,15 @@
                 'BuildStatus.FAILEDTOBUILD',
                 status[2],
                 )
-            status_dict = {
+            slave_status = {
                 'builder_status': raw_status[0],
                 'build_status': raw_status[1],
                 }
-            behavior.updateSlaveStatus(raw_status, status_dict)
-            self.assertNotIn('filemap', status_dict)
-
-            return behavior.updateBuild_WAITING(
-                queue_item, status_dict, None, logging)
+            behavior.updateSlaveStatus(raw_status, slave_status)
+            self.assertNotIn('filemap', slave_status)
+            return behavior.handleStatus(
+                    behavior._interactor.extractBuildStatus(slave_status),
+                    None, slave_status, queue_item=queue_item),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FAILEDTOBUILD, behavior.build.status)
@@ -270,7 +273,7 @@
         d.addCallback(build_updated)
         return d
 
-    def test_updateBuild_WAITING_notarball(self):
+    def test_handleStatus_notarball(self):
         # Even if the build status is "OK," absence of a tarball will
         # not faze the Behavior class.
         behavior = self.makeBehavior()
@@ -288,14 +291,15 @@
                 'BuildStatus.OK',
                 status[2],
                 )
-            status_dict = {
+            slave_status = {
                 'builder_status': raw_status[0],
                 'build_status': raw_status[1],
                 }
-            behavior.updateSlaveStatus(raw_status, status_dict)
-            self.assertFalse('filemap' in status_dict)
-            return behavior.updateBuild_WAITING(
-                queue_item, status_dict, None, logging)
+            behavior.updateSlaveStatus(raw_status, slave_status)
+            self.assertFalse('filemap' in slave_status)
+            return behavior.handleStatus(
+                    behavior._interactor.extractBuildStatus(slave_status),
+                    None, slave_status, queue_item=queue_item),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)
@@ -308,7 +312,7 @@
         d.addCallback(build_updated)
         return d
 
-    def test_updateBuild_WAITING_uploads(self):
+    def test_handleStatus_uploads(self):
         productseries = self.makeProductSeriesWithBranchForTranslation()
         branch = productseries.branch
         behavior = self.makeBehavior(branch=branch)
@@ -336,8 +340,9 @@
                 'build_id': status[2],
                 }
             behavior.updateSlaveStatus(status, slave_status)
-            return behavior.updateBuild_WAITING(
-                queue_item, slave_status, None, logging)
+            return behavior.handleStatus(
+                    behavior._interactor.extractBuildStatus(slave_status),
+                    None, slave_status, queue_item=queue_item),
 
         def build_updated(ignored):
             self.assertEqual(BuildStatus.FULLYBUILT, behavior.build.status)