Merge lp:~mwhudson/launchpad/back-off-failing-imports-bug-413637 into lp:launchpad/db-devel

[Michael Hudson-Doyle]

Hi there,

This small branch changes the code import system to exponentially back off on retrying code imports -- see the linked bug for more.

Currently it backs off so that it tries imports at 6, 12, 24 and 48 hours (for subversion) for a total of three and a bit days of trying before marking an import failed -- is that enough? It seems OK, though perhaps a bit on the low side, to me.

Cheers,
mwh

[Jonathan Lange (jml)]

On Mon, Mar 15, 2010 at 10:22 PM, Michael Hudson
<email address hidden> wrote:
> Michael Hudson has proposed merging lp:~mwhudson/launchpad/back-off-failing-imports-bug-413637 into lp:launchpad.
>
> Requested reviews:
>  Canonical Launchpad Engineering (launchpad)
> Related bugs:
>  #413637 imports disabled too rapidly
>  https://bugs.launchpad.net/bugs/413637
>
>
> Hi there,
>
> This small branch changes the code import system to exponentially back off on retrying code imports -- see the linked bug for more.
>

I think the branch is bigger than you intended. It's got a bunch of
build farm job stuff in the diff.

> Currently it backs off so that it tries imports at 6, 12, 24 and 48 hours (for subversion) for a total of three and a bit days of trying before marking an import failed -- is that enough?  It seems OK, though perhaps a bit on the low side, to me.
>

Sounds fine to me.

...
> === modified file 'lib/lp/code/model/codeimportjob.py'
> --- lib/lp/code/model/codeimportjob.py  2010-02-24 10:18:16 +0000
> +++ lib/lp/code/model/codeimportjob.py  2010-03-15 22:22:20 +0000
> @@ -10,6 +10,8 @@
>     'CodeImportJobWorkflow',
>     ]
>
> +import datetime
> +
>  from sqlobject import ForeignKey, IntCol, SQLObjectNotFound, StringCol
>
>  from zope.component import getUtility
> @@ -140,7 +142,7 @@
>
>     implements(ICodeImportJobWorkflow)
>
> -    def newJob(self, code_import, date_due=None):
> +    def newJob(self, code_import, interval=None):
>         """See `ICodeImportJobWorkflow`."""
>         assert code_import.review_status == CodeImportReviewStatus.REVIEWED, (
>             "Review status of %s is not REVIEWED: %s" % (
> @@ -149,23 +151,22 @@
>             "Already associated to a CodeImportJob: %s" % (
>             code_import.branch.unique_name))
>
> +        if interval is None:
> +            interval = code_import.effective_update_interval
> +
>         job = CodeImportJob(code_import=code_import, date_due=UTC_NOW)
>
> -        if date_due is None:
> -            # Find the most recent CodeImportResult for this CodeImport. We
> -            # sort by date_created because we do not have an index on
> -            # date_job_started in the database, and that should give the same
> -            # sort order.
> -            most_recent_result_list = list(CodeImportResult.selectBy(
> -                code_import=code_import).orderBy(['-date_created']).limit(1))
> +        # Find the most recent CodeImportResult for this CodeImport. We
> +        # sort by date_created because we do not have an index on
> +        # date_job_started in the database, and that should give the same
> +        # sort order.
> +        most_recent_result_list = list(CodeImportResult.selectBy(
> +            code_import=code_import).orderBy(['-date_created']).limit(1))
>

Why aren't you using Storm, with its 'one()' notation for this?

> @@ -281,15 +282,19 @@
>         # If the import has failed too many times in a row, mark it as
>         # FAILING.
>         failure_limit = config.codeimport.consecutive_failure_limit
> -        if code_import.consecutive_failure_count >= failure_limit:
> +        failure_count = code_import.consecutive_failure_count...

[Michael Hudson-Doyle (mwhudson)]

Jonathan Lange wrote:
> On Mon, Mar 15, 2010 at 10:22 PM, Michael Hudson
> <email address hidden> wrote:
>> Michael Hudson has proposed merging lp:~mwhudson/launchpad/back-off-failing-imports-bug-413637 into lp:launchpad.
>>
>> Requested reviews:
>> Canonical Launchpad Engineering (launchpad)
>> Related bugs:
>> #413637 imports disabled too rapidly
>> https://bugs.launchpad.net/bugs/413637
>>
>>
>> Hi there,
>>
>> This small branch changes the code import system to exponentially back off on retrying code imports -- see the linked bug for more.
>>
>
> I think the branch is bigger than you intended. It's got a bunch of
> build farm job stuff in the diff.

Grar grar, it's proposed into db-devel of course.

I proposed again:
https://code.edge.launchpad.net/~mwhudson/launchpad/back-off-failing-imports-bug-413637/+merge/21411

>> Currently it backs off so that it tries imports at 6, 12, 24 and 48 hours (for subversion) for a total of three and a bit days of trying before marking an import failed -- is that enough? It seems OK, though perhaps a bit on the low side, to me.
>>
>
> Sounds fine to me.
>
> ...
>> === modified file 'lib/lp/code/model/codeimportjob.py'
>> --- lib/lp/code/model/codeimportjob.py 2010-02-24 10:18:16 +0000
>> +++ lib/lp/code/model/codeimportjob.py 2010-03-15 22:22:20 +0000
>> @@ -10,6 +10,8 @@
>> 'CodeImportJobWorkflow',
>> ]
>>
>> +import datetime
>> +
>> from sqlobject import ForeignKey, IntCol, SQLObjectNotFound, StringCol
>>
>> from zope.component import getUtility
>> @@ -140,7 +142,7 @@
>>
>> implements(ICodeImportJobWorkflow)
>>
>> - def newJob(self, code_import, date_due=None):
>> + def newJob(self, code_import, interval=None):
>> """See `ICodeImportJobWorkflow`."""
>> assert code_import.review_status == CodeImportReviewStatus.REVIEWED, (
>> "Review status of %s is not REVIEWED: %s" % (
>> @@ -149,23 +151,22 @@
>> "Already associated to a CodeImportJob: %s" % (
>> code_import.branch.unique_name))
>>
>> + if interval is None:
>> + interval = code_import.effective_update_interval
>> +
>> job = CodeImportJob(code_import=code_import, date_due=UTC_NOW)
>>
>> - if date_due is None:
>> - # Find the most recent CodeImportResult for this CodeImport. We
>> - # sort by date_created because we do not have an index on
>> - # date_job_started in the database, and that should give the same
>> - # sort order.
>> - most_recent_result_list = list(CodeImportResult.selectBy(
>> - code_import=code_import).orderBy(['-date_created']).limit(1))
>> + # Find the most recent CodeImportResult for this CodeImport. We
>> + # sort by date_created because we do not have an index on
>> + # date_job_started in the database, and that should give the same
>> + # sort order.
>> + most_recent_result_list = list(CodeImportResult.selectBy(
>> + code_import=code_import).orderBy(['-date_created']).limit(1))
>>
>
> Why aren't you using Storm, with its 'one()' notation for this?

Because all I did to this code was dedent it. I ...

=== modified file 'lib/canonical/buildd/debian/launchpad-buildd.init'
--- lib/canonical/buildd/debian/launchpad-buildd.init	2009-12-16 00:16:24 +0000
+++ lib/canonical/buildd/debian/launchpad-buildd.init	2010-03-15 22:22:20 +0000
@@ -29,7 +29,7 @@
     CONF=$1
     PIDFILE="$PIDROOT"/"$CONF".pid
     LOGFILE="$LOGROOT"/"$CONF".log
-    su - buildd -c "BUILDD_SLAVE_CONFIG=$CONFROOT/$CONF PYTHONPATH=/usr/share/launchpad-buildd twistd --no_save --pidfile $PIDFILE --python $TACFILE --logfile $LOGFILE"
+    su - buildd -c "BUILDD_SLAVE_CONFIG=$CONFROOT/$CONF PYTHONPATH=/usr/share/launchpad-buildd twistd --no_save --pidfile $PIDFILE --python $TACFILE --logfile $LOGFILE --umask 022"
 }
 
 #

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-01-21 05:03:16 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-03-15 22:22:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 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

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-05 13:52:32 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-15 22:22:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 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
@@ -17,17 +17,22 @@
 import xmlrpclib
 
 from sqlobject import SQLObjectNotFound
+
 from zope.component import getUtility
 from zope.interface import implements
-from zope.security.proxy import removeSecurityProxy
+from zope.security.proxy import removeSecurityProxy, isinstance as zisinstance
 
 from canonical import encoding
 from canonical.librarian.interfaces import ILibrarianClient
+
+from canonical.launchpad.webapp.interfaces import NotFoundError
 from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     BuildBehaviorMismatch, IBuildFarmJobBehavior)
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.services.job.interfaces.job import JobStatus
+from lp.soyuz.interfaces.build import IBuildSet
 
 
 class BuildFarmJobBehaviorBase:
@@ -59,22 +64,101 @@
         The default behavior is that we don't add any extra values."""
         return {}
 
+    def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):
+        """Helper for verifying parts of a `BuildFarmJob` name.
+
+        Different `IBuildFarmJob` implementations can have different
+        ways of constructing their identifying names.  The names are
+        produced by `IBuildFarmJob.getName` and verified by
+        `IBuildFarmJobBehavior.verifySlaveBuildID`.
+
+        This little helper makes it easier to verify an object id
+        embedded in that name, check that it's a valid number, and
+        retrieve the associated database object.
+
+        :param raw_id: An unverified id string as extracted from the
+            build name.  The method will verify that it is a number, and
+            try to retrieve the associated object.
+        :param item_type: The type of object this id represents.  Should
+            be a class.
+        :param finder: A function that, given an integral id, finds the
+            associated object of type `item_type`.
+        :raise CorruptBuildID: If `raw_id` is malformed in some way or
+            the associated `item_type` object is not found.
+        :return: An object that is an instance of `item_type`.
+        """
+        type_name = item_type.__name__
+        try:
+            numeric_id = int(raw_id)
+        except ValueError:
+            raise CorruptBuildID(
+                "%s ID is not a number: '%s'" % (type_name, raw_id))
+
+        try:
+            item = finder(numeric_id)
+        except (NotFoundError, SQLObjectNotFound), reason:
+            raise CorruptBuildID(
+                "%s %d is not available: %s" % (
+                    type_name, numeric_id, reason))
+        except Exception, reason:
+            raise CorruptBuildID(
+                "Error while looking up %s %d: %s" % (
+                    type_name, numeric_id, reason))
+
+        if item is None:
+            raise CorruptBuildID("There is no %s with id %d." % (
+                type_name, numeric_id))
+
+        assert zisinstance(item, item_type), (
+            "Looked for %s, but found %s." % (type_name, repr(item)))
+
+        return item
+
+    def getVerifiedBuild(self, raw_id):
+        """Verify and retrieve the `Build` component of a slave build id.
+
+        This does part of the verification for `verifySlaveBuildID`.
+
+        By default, a `BuildFarmJob` has an identifying name of the form
+        "b-q", where b is the id of its `Build` and q is the id of its
+        `BuildQueue` record.
+
+        Use `getVerifiedBuild` to verify the "b" part, and retrieve the
+        associated `Build`.
+        """
+        # Avoid circular import.
+        from lp.soyuz.model.build import Build
+
+        return self._helpVerifyBuildIDComponent(
+            raw_id, Build, getUtility(IBuildSet).getByBuildID)
+
+    def getVerifiedBuildQueue(self, raw_id):
+        """Verify and retrieve the `BuildQueue` component of a slave build id.
+
+        This does part of the verification for `verifySlaveBuildID`.
+
+        By default, a `BuildFarmJob` has an identifying name of the form
+        "b-q", where b is the id of its `Build` and q is the id of its
+        `BuildQueue` record.
+
+        Use `getVerifiedBuildQueue` to verify the "q" part, and retrieve
+        the associated `BuildQueue` object.
+        """
+        return self._helpVerifyBuildIDComponent(
+            raw_id, BuildQueue, getUtility(IBuildQueueSet).get)
+
     def verifySlaveBuildID(self, slave_build_id):
         """See `IBuildFarmJobBehavior`."""
         # Extract information from the identifier.
         try:
             build_id, queue_item_id = slave_build_id.split('-')
-            build_id = int(build_id)
-            queue_item_id = int(queue_item_id)
         except ValueError:
             raise CorruptBuildID('Malformed build ID')
+            
+        build = self.getVerifiedBuild(build_id)
+        queue_item = self.getVerifiedBuildQueue(queue_item_id)
 
-        try:
-            queue_item = getUtility(IBuildQueueSet).get(queue_item_id)
-            # Check whether build and buildqueue are properly related.
-        except SQLObjectNotFound, reason:
-            raise CorruptBuildID(str(reason))
-        if queue_item.specific_job.build.id != build_id:
+        if build != queue_item.specific_job.build:
             raise CorruptBuildID('Job build entry mismatch')
 
     def updateBuild(self, queueItem):

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2010-03-06 04:57:40 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2010-03-15 22:22:20 +0000
@@ -3,10 +3,18 @@
 
 """Unit tests for BuildFarmJobBehaviorBase."""
 
-from unittest import TestCase, TestLoader
+from unittest import TestLoader
+
+from zope.component import getUtility
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.testing import TestCaseWithFactory
 
 from lp.buildmaster.interfaces.buildbase import BuildStatus
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase
+from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.soyuz.interfaces.processor import IProcessorFamilySet
 
 
 class FakeBuildFarmJob:
@@ -14,27 +22,91 @@
     pass
 
 
-class TestBuildFarmJobBehaviorBase(TestCase):
+class TestBuildFarmJobBehaviorBase(TestCaseWithFactory):
     """Test very small, basic bits of BuildFarmJobBehaviorBase."""
 
-    def setUp(self):
-        self.behavior = BuildFarmJobBehaviorBase(FakeBuildFarmJob())
+    layer = ZopelessDatabaseLayer
+
+    def _makeBehavior(self, buildfarmjob=None):
+        """Create a `BuildFarmJobBehaviorBase`."""
+        if buildfarmjob is None:
+            buildfarmjob = FakeBuildFarmJob()
+        return BuildFarmJobBehaviorBase(buildfarmjob)
+
+    def _makeBuild(self):
+        """Create a `Build` object."""
+        x86 = getUtility(IProcessorFamilySet).getByName('x86')
+        distroarchseries = self.factory.makeDistroArchSeries(
+            architecturetag='x86', processorfamily=x86)
+        distroseries = distroarchseries.distroseries
+        archive = self.factory.makeArchive(
+            distribution=distroseries.distribution)
+        pocket = PackagePublishingPocket.RELEASE
+        spr = self.factory.makeSourcePackageRelease(
+            distroseries=distroseries, archive=archive)
+
+        return spr.createBuild(
+            distroarchseries=distroarchseries, pocket=pocket, archive=archive)
+
+    def _makeBuildQueue(self):
+        """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,
-            self.behavior.extractBuildStatus(slave_status))
+            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'}
-        self.assertRaises(
-            AssertionError,
-            self.behavior.extractBuildStatus, slave_status)
+        behavior = self._makeBehavior()
+        self.assertRaises(
+            AssertionError, behavior.extractBuildStatus, slave_status)
+
+    def test_getVerifiedBuild_success(self):
+        build = self._makeBuild()
+        behavior = self._makeBehavior()
+        raw_id = str(build.id)
+
+        self.assertEqual(build, behavior.getVerifiedBuild(raw_id))
+
+    def test_getVerifiedBuild_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.getVerifiedBuild, 'hi!')
+
+    def test_getVerifiedBuild_notfound(self):
+        build = self._makeBuild()
+        behavior = self._makeBehavior()
+        nonexistent_id = str(build.id + 99)
+
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuild, nonexistent_id)
+
+    def test_getVerifiedBuildQueue_success(self):
+        buildqueue = self._makeBuildQueue()
+        behavior = self._makeBehavior()
+        raw_id = str(buildqueue.id)
+
+        self.assertEqual(buildqueue, behavior.getVerifiedBuildQueue(raw_id))
+
+    def test_getVerifiedBuildQueue_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuildQueue, 'bye!')
+
+    def test_getVerifiedBuildQueue_notfound(self):
+        buildqueue = self._makeBuildQueue()
+        behavior = self._makeBehavior()
+        nonexistent_id = str(buildqueue.id + 99)
+
+        self.assertRaises(
+            CorruptBuildID, behavior.getVerifiedBuildQueue, nonexistent_id)
 
 
 def test_suite():

=== modified file 'lib/lp/code/interfaces/codeimportjob.py'
--- lib/lp/code/interfaces/codeimportjob.py	2010-02-22 08:10:03 +0000
+++ lib/lp/code/interfaces/codeimportjob.py	2010-03-15 22:22:20 +0000
@@ -16,6 +16,8 @@
     'ICodeImportJobWorkflow',
     ]
 
+import datetime
+
 from zope.interface import Interface
 from zope.schema import Choice, Datetime, Int, Object, Text
 
@@ -137,7 +139,7 @@
 class ICodeImportJobWorkflow(Interface):
     """Utility to manage `CodeImportJob` objects through their life cycle."""
 
-    def newJob(code_import):
+    def newJob(code_import, interval=datetime.timedelta(0)):
         """Create a `CodeImportJob` associated with a reviewed `CodeImport`.
 
         Call this method from `CodeImport.updateFromData` when the
@@ -218,9 +220,11 @@
             display for diagnostics. May be None.
         :precondition: `import_job`.state == RUNNING.
         :postcondition: `import_job` is deleted.
-        :postcondition: `code_import.import_job` is not None.
+        :postcondition: `code_import.import_job` is not None unless the job
+            has failed more than consecutive_failure_limit times in a row.
         :postcondition: `code_import.import_job.date_due` is
-            import_job.date_due + code_import.effective_update_interval`.
+            import_job.date_due + code_import.effective_update_interval`, with
+            scaling to retry failing imports less often.
         :postcondition: A `CodeImportResult` was created.
         :postcondition: A FINISH `CodeImportEvent` was created.
         """

=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2010-02-24 10:18:16 +0000
+++ lib/lp/code/model/codeimportjob.py	2010-03-15 22:22:20 +0000
@@ -10,6 +10,8 @@
     'CodeImportJobWorkflow',
     ]
 
+import datetime
+
 from sqlobject import ForeignKey, IntCol, SQLObjectNotFound, StringCol
 
 from zope.component import getUtility
@@ -140,7 +142,7 @@
 
     implements(ICodeImportJobWorkflow)
 
-    def newJob(self, code_import, date_due=None):
+    def newJob(self, code_import, interval=None):
         """See `ICodeImportJobWorkflow`."""
         assert code_import.review_status == CodeImportReviewStatus.REVIEWED, (
             "Review status of %s is not REVIEWED: %s" % (
@@ -149,23 +151,22 @@
             "Already associated to a CodeImportJob: %s" % (
             code_import.branch.unique_name))
 
+        if interval is None:
+            interval = code_import.effective_update_interval
+
         job = CodeImportJob(code_import=code_import, date_due=UTC_NOW)
 
-        if date_due is None:
-            # Find the most recent CodeImportResult for this CodeImport. We
-            # sort by date_created because we do not have an index on
-            # date_job_started in the database, and that should give the same
-            # sort order.
-            most_recent_result_list = list(CodeImportResult.selectBy(
-                code_import=code_import).orderBy(['-date_created']).limit(1))
+        # Find the most recent CodeImportResult for this CodeImport. We
+        # sort by date_created because we do not have an index on
+        # date_job_started in the database, and that should give the same
+        # sort order.
+        most_recent_result_list = list(CodeImportResult.selectBy(
+            code_import=code_import).orderBy(['-date_created']).limit(1))
 
-            if len(most_recent_result_list) != 0:
-                [most_recent_result] = most_recent_result_list
-                interval = code_import.effective_update_interval
-                date_due = most_recent_result.date_job_started + interval
-                job.date_due = max(job.date_due, date_due)
-        else:
-            job.date_due = date_due
+        if len(most_recent_result_list) != 0:
+            [most_recent_result] = most_recent_result_list
+            date_due = most_recent_result.date_job_started + interval
+            job.date_due = max(job.date_due, date_due)
 
         return job
 
@@ -281,15 +282,19 @@
         # If the import has failed too many times in a row, mark it as
         # FAILING.
         failure_limit = config.codeimport.consecutive_failure_limit
-        if code_import.consecutive_failure_count >= failure_limit:
+        failure_count = code_import.consecutive_failure_count
+        if failure_count >= failure_limit:
             code_import.updateFromData(
                 dict(review_status=CodeImportReviewStatus.FAILING), None)
+        elif status == CodeImportResultStatus.SUCCESS_PARTIAL:
+            interval = datetime.timedelta(0)
+        elif failure_count > 0:
+            interval = code_import.effective_update_interval * (2**(failure_count - 1))
+        else:
+            interval = code_import.effective_update_interval
         # Only start a new one if the import is still in the REVIEWED state.
         if code_import.review_status == CodeImportReviewStatus.REVIEWED:
-            extra = {}
-            if status == CodeImportResultStatus.SUCCESS_PARTIAL:
-                extra['date_due'] = UTC_NOW
-            self.newJob(code_import, **extra)
+            self.newJob(code_import, interval=interval)
         # If the status was successful, update date_last_successful.
         if status in [CodeImportResultStatus.SUCCESS,
                       CodeImportResultStatus.SUCCESS_NOCHANGE]:
@@ -321,7 +326,7 @@
             import_job, CodeImportResultStatus.RECLAIMED, None)
         # 3)
         if code_import.review_status == CodeImportReviewStatus.REVIEWED:
-            self.newJob(code_import, UTC_NOW)
+            self.newJob(code_import, datetime.timedelta(0))
         # 4)
         getUtility(ICodeImportEventSet).newReclaim(
             code_import, machine, job_id)

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-02-05 15:06:28 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-03-15 22:22:20 +0000
@@ -8,7 +8,7 @@
     'RecipeBuildBehavior',
     ]
 
-from zope.component import adapts
+from zope.component import adapts, getUtility
 from zope.interface import implements
 
 from lp.archiveuploader.permission import check_upload_to_pocket
@@ -18,7 +18,8 @@
 from lp.buildmaster.model.buildfarmjobbehavior import (
     BuildFarmJobBehaviorBase)
 from lp.code.interfaces.sourcepackagerecipebuild import (
-    ISourcePackageRecipeBuildJob)
+    ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuildSource)
+from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.soyuz.adapters.archivedependencies import (
     get_primary_current_component, get_sources_list_for_building)
@@ -168,3 +169,11 @@
                 extra_info['logtail'] = raw_slave_status[2]
 
         return extra_info
+
+    def getVerifiedBuild(self, raw_id):
+        """See `IBuildFarmJobBehavior`."""
+        # This type of job has a build that is of type BuildBase but not
+        # actually a Build.
+        return self._helpVerifyBuildIDComponent(
+            raw_id, SourcePackageRecipeBuild,
+            getUtility(ISourcePackageRecipeBuildSource).getById)

=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2010-03-14 20:15:34 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2010-03-15 22:22:20 +0000
@@ -778,6 +778,31 @@
         self.assertEqual(new_job.machine, None)
         self.assertSqlAttributeEqualsDate(new_job, 'date_due', UTC_NOW)
 
+    def test_failures_back_off(self):
+        # We wait for longer and longer between retrying failing imports, to
+        # make it less likely that an import is marked failing just because
+        # someone's DNS went down for a day.
+        running_job = self.makeRunningJob()
+        intervals = []
+        interval = running_job.code_import.effective_update_interval
+        expected_intervals = []
+        for i in range(config.codeimport.consecutive_failure_limit - 1):
+            expected_intervals.append(interval)
+            interval *= 2
+        # Fail an import a bunch of times and record how far in the future the
+        # next job was scheduled.
+        for i in range(config.codeimport.consecutive_failure_limit - 1):
+            code_import = running_job.code_import
+            getUtility(ICodeImportJobWorkflow).finishJob(
+                running_job, CodeImportResultStatus.FAILURE, None)
+            intervals.append(
+                code_import.import_job.date_due -
+                code_import.results[-1].date_job_started)
+            running_job = code_import.import_job
+            getUtility(ICodeImportJobWorkflow).startJob(
+                running_job, self.machine)
+        self.assertEqual(expected_intervals, intervals)
+
     def test_doesntCreateNewJobIfCodeImportNotReviewed(self):
         # finishJob() creates a new CodeImportJob for the given CodeImport,
         # unless the CodeImport has been suspended or marked invalid.

=== modified file 'lib/lp/soyuz/doc/buildd-slavescanner.txt'
--- lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-03-10 12:50:18 +0000
+++ lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-03-15 22:22:20 +0000
@@ -95,15 +95,16 @@
     >>> lostbuilding_builder = MockBuilder(
     ...     'Lost Building Slave', LostBuildingSlave())
     >>> buildergroup.rescueBuilderIfLost(lostbuilding_builder)
-    WARNING:root:Builder 'Lost Building Slave' rescued from
-    '1000-10000': 'Object not found'
+    WARNING:root:Builder 'Lost Building Slave' rescued from '1000-10000':
+    'Build 1000 is not available: 'Object not found''
 
 Then a lost slave in status 'WAITING':
 
     >>> lostwaiting_builder = MockBuilder(
     ...     'Lost Waiting Slave', LostWaitingSlave())
     >>> buildergroup.rescueBuilderIfLost(lostwaiting_builder)
-    WARNING:root:Builder 'Lost Waiting Slave' rescued from '1000-10000': 'Object not found'
+    WARNING:root:Builder 'Lost Waiting Slave' rescued from '1000-10000':
+    'Build 1000 is not available: 'Object not found''
 
 Both got rescued, as expected.
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-03-11 20:54:36 +0000
+++ lib/lp/testing/factory.py	2010-03-15 22:22:20 +0000
@@ -2008,6 +2008,81 @@
     def getAnySourcePackageUrgency(self):
         return SourcePackageUrgency.MEDIUM
 
+    def makeSourcePackageRelease(self, archive=None, sourcepackagename=None,
+                                 distroseries=None, maintainer=None,
+                                 creator=None, component=None, section=None,
+                                 urgency=None, version=None,
+                                 builddepends=None, builddependsindep=None,
+                                 build_conflicts=None,
+                                 build_conflicts_indep=None,
+                                 architecturehintlist='all',
+                                 dsc_maintainer_rfc822=None,
+                                 dsc_standards_version='3.6.2',
+                                 dsc_format='1.0', dsc_binaries='foo-bin',
+                                 date_uploaded=UTC_NOW):
+        """Make a `SourcePackageRelease`."""
+        if distroseries is None:
+            if archive is None:
+                distribution = None
+            else:
+                distribution = archive.distribution
+            distroseries = self.makeDistroRelease(distribution=distribution)
+
+        if archive is None:
+            archive = self.makeArchive(
+                distribution=distroseries.distribution,
+                purpose=ArchivePurpose.PRIMARY)
+
+        if sourcepackagename is None:
+            sourcepackagename = self.makeSourcePackageName()
+
+        if component is None:
+            component = self.makeComponent()
+
+        if urgency is None:
+            urgency = self.getAnySourcePackageUrgency()
+
+        if section is None:
+            section = self.getUniqueString('section')
+        section = getUtility(ISectionSet).ensure(section)
+
+        if maintainer is None:
+            maintainer = self.makePerson()
+
+        maintainer_email = '%s <%s>' % (
+            maintainer.displayname,
+            maintainer.preferredemail.email)
+
+        if creator is None:
+            creator = self.makePerson()
+
+        if version is None:
+            version = self.getUniqueString('version')
+
+        return distroseries.createUploadedSourcePackageRelease(
+            sourcepackagename=sourcepackagename,
+            maintainer=maintainer,
+            creator=creator,
+            component=component,
+            section=section,
+            urgency=urgency,
+            version=version,
+            builddepends=builddepends,
+            builddependsindep=builddependsindep,
+            build_conflicts=build_conflicts,
+            build_conflicts_indep=build_conflicts_indep,
+            architecturehintlist=architecturehintlist,
+            changelog_entry=None,
+            dsc=None,
+            copyright=self.getUniqueString(),
+            dscsigningkey=None,
+            dsc_maintainer_rfc822=maintainer_email,
+            dsc_standards_version=dsc_standards_version,
+            dsc_format=dsc_format,
+            dsc_binaries=dsc_binaries,
+            archive=archive,
+            dateuploaded=date_uploaded)
+
     def makeSourcePackagePublishingHistory(self, sourcepackagename=None,
                                            distroseries=None, maintainer=None,
                                            creator=None, component=None,
@@ -2027,54 +2102,31 @@
                                            dsc_format='1.0',
                                            dsc_binaries='foo-bin',
                                            ):
-        if sourcepackagename is None:
-            sourcepackagename = self.makeSourcePackageName()
-        spn = sourcepackagename
-
+        """Make a `SourcePackagePublishingHistory`."""
         if distroseries is None:
-            distroseries = self.makeDistroRelease()
+            if archive is None:
+                distribution = None
+            else:
+                distribution = archive.distribution
+            distroseries = self.makeDistroRelease(distribution=distribution)
 
         if archive is None:
             archive = self.makeArchive(
                 distribution=distroseries.distribution,
                 purpose=ArchivePurpose.PRIMARY)
 
-        if component is None:
-            component = self.makeComponent()
-
         if pocket is None:
             pocket = self.getAnyPocket()
 
         if status is None:
             status = PackagePublishingStatus.PENDING
 
-        if urgency is None:
-            urgency = self.getAnySourcePackageUrgency()
-
-        if section is None:
-            section = self.getUniqueString('section')
-        section = getUtility(ISectionSet).ensure(section)
-
-        if maintainer is None:
-            maintainer = self.makePerson()
-
-        maintainer_email = '%s <%s>' % (
-            maintainer.displayname,
-            maintainer.preferredemail.email)
-
-        if creator is None:
-            creator = self.makePerson()
-
-        if version is None:
-            version = self.getUniqueString('version')
-
-        gpg_key = self.makeGPGKey(creator)
-
-        spr = distroseries.createUploadedSourcePackageRelease(
-            sourcepackagename=spn,
+        spr = self.makeSourcePackageRelease(
+            archive=archive,
+            sourcepackagename=sourcepackagename,
+            distroseries=distroseries,
             maintainer=maintainer,
-            creator=creator,
-            component=component,
+            creator=creator, component=component,
             section=section,
             urgency=urgency,
             version=version,
@@ -2083,15 +2135,10 @@
             build_conflicts=build_conflicts,
             build_conflicts_indep=build_conflicts_indep,
             architecturehintlist=architecturehintlist,
-            changelog_entry=None,
-            dsc=None,
-            copyright=self.getUniqueString(),
-            dscsigningkey=gpg_key,
-            dsc_maintainer_rfc822=maintainer_email,
             dsc_standards_version=dsc_standards_version,
             dsc_format=dsc_format,
             dsc_binaries=dsc_binaries,
-            archive=archive, dateuploaded=date_uploaded)
+            date_uploaded=date_uploaded)
 
         sspph = SourcePackagePublishingHistory(
             distroseries=distroseries,

=== modified file 'lib/lp/translations/doc/translationtemplatesbuildbehavior.txt'
--- lib/lp/translations/doc/translationtemplatesbuildbehavior.txt	2010-03-05 13:52:32 +0000
+++ lib/lp/translations/doc/translationtemplatesbuildbehavior.txt	2010-03-15 22:22:20 +0000
@@ -2,13 +2,21 @@
 
 == Setup ==
 
-Set up build environment.
+Set up build environment.  Clear out the build queue.
 
     >>> import transaction
     >>> import logging
     >>> logger = logging.getLogger()
     >>> logger.setLevel(logging.CRITICAL)
 
+    >>> from canonical.database.sqlbase import quote
+    >>> from canonical.launchpad.interfaces.lpstorm import IMasterStore
+    >>> from lp.services.job.interfaces.job import JobStatus
+    >>> from lp.services.job.model.job import Job
+    >>> store = IMasterStore(Job)
+    >>> query = store.execute("UPDATE Job SET status = %s" % quote(
+    ...     JobStatus.FAILED))
+
     >>> from lp.buildmaster.master import BuilddMaster
     >>> from canonical.buildd.tests import BuilddSlaveTestSetup
     >>> bm = BuilddMaster(logger, transaction)
@@ -39,7 +47,21 @@
 
 Make a builder to process our build request.
 
-    >>> builder = factory.makeBuilder(virtualized=False, processor=processor)
+    >>> builder = factory.makeBuilder(
+    ...     virtualized=True, processor=processor, vm_host='hostname')
+
+The builder doesn't talk to a real slave.  We don't have those in our
+test suite.  But we give it a fake one.
+
+    >>> from lp.testing.fakemethod import FakeMethod
+    >>> class FakeSlave:
+    ...     build = FakeMethod()
+    ...     cacheFile = FakeMethod()
+    ...     resume = FakeMethod(result=('Output here', 'Errors here', 0))
+
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> slave = FakeSlave()
+    >>> removeSecurityProxy(builder).slave = slave
 
 
 == Get a job! ==
@@ -60,20 +82,27 @@
     >>> print buildqueue.date_started
     None
 
-Dispatch the job to the build slave.  The proper method to call here
-would have been Builder.findAndStartJob, but it has not been generalised
-to handle new job types yet.
-
-# XXX JeroenVermeulen bug=506617: call findAndStartJob instead.
-
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> removeSecurityProxy(builder)._dispatchBuildCandidate(buildqueue)
-
-The build is now marked as started.
+Our job is now first in line to be executed.
+
+    >>> removeSecurityProxy(builder)._findBuildCandidate() == buildqueue
+    True
+
+Dispatch the first job in line (ours!) to the build slave.
+
+    >>> activated_job = builder.findAndStartJob()
+    >>> activated_job == buildqueue
+    True
+
+Our build is now marked as started.
 
     >>> buildqueue.date_started is None
     False
 
+The slave's build method has been called.
+
+    >>> slave.build.call_count
+    1
+
 
 == Teardown ==
 

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-02-11 19:11:11 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-03-15 22:22:20 +0000
@@ -17,6 +17,7 @@
 
 from canonical.launchpad.interfaces import ILaunchpadCelebrities
 
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior)
 from lp.buildmaster.model.buildfarmjobbehavior import (
@@ -40,12 +41,27 @@
         self._builder.slave.cacheFile(logger, chroot)
         buildid = self.buildfarmjob.getName()
 
-        args = { 'branch_url': self.buildfarmjob.branch.url }
+        args = self.buildfarmjob.metadata
         filemap = {}
 
         self._builder.slave.build(
             buildid, self.build_type, chroot_sha1, filemap, args)
 
+    def verifySlaveBuildID(self, slave_build_id):
+        """See `IBuildFarmJobBehavior`."""
+        try:
+            branch_name, queue_item_id = slave_build_id.rsplit('-', 1)
+        except ValueError:
+            raise CorruptBuildID(
+                "Malformed translation templates build id: '%s'" % (
+                    slave_build_id))
+
+        buildqueue = self.getVerifiedBuildQueue(queue_item_id)
+        if buildqueue.job != self.buildfarmjob.job:
+            raise CorruptBuildID(
+                "ID mismatch for translation templates build '%s'" % (
+                    slave_build_id))
+
     def _getChroot(self):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         return ubuntu.currentseries.nominatedarchindep.getChroot()

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-12 08:31:43 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-15 22:22:20 +0000
@@ -19,6 +19,7 @@
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior)
+from lp.buildmaster.interfaces.builder import CorruptBuildID
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.testing import TestCaseWithFactory
 from lp.testing.fakemethod import FakeMethod
@@ -123,6 +124,10 @@
         self.assertEqual(
             'translation-templates', slave_status['test_build_type'])
         self.assertIn('branch_url', slave_status['test_build_args'])
+        # The slave receives the public http URL for the branch.
+        self.assertEqual(
+            behavior.buildfarmjob.branch.composePublicURL(),
+            slave_status['test_build_args']['branch_url'])
 
     def test_getChroot(self):
         # _getChroot produces the current chroot for the current Ubuntu
@@ -170,6 +175,38 @@
         self.assertEqual(1, builder.cleanSlave.call_count)
         self.assertEqual(0, behavior._uploadTarball.call_count)
 
+    def test_verifySlaveBuildID_success(self):
+        # TranslationTemplatesBuildJob.getName generates slave build ids
+        # that TranslationTemplatesBuildBehavior.verifySlaveBuildID
+        # accepts.
+        behavior = self._makeBehavior()
+        buildfarmjob = behavior.buildfarmjob
+        job = buildfarmjob.job
+
+        # The test is that this not raise CorruptBuildID (or anything
+        # else, for that matter).
+        behavior.verifySlaveBuildID(behavior.buildfarmjob.getName())
+
+    def test_verifySlaveBuildID_handles_dashes(self):
+        # TranslationTemplatesBuildBehavior.verifySlaveBuildID can deal
+        # with dashes in branch names.
+        behavior = self._makeBehavior()
+        buildfarmjob = behavior.buildfarmjob
+        job = buildfarmjob.job
+        buildfarmjob.branch.name = 'x-y-z--'
+
+        # The test is that this not raise CorruptBuildID (or anything
+        # else, for that matter).
+        behavior.verifySlaveBuildID(behavior.buildfarmjob.getName())
+
+    def test_verifySlaveBuildID_malformed(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.verifySlaveBuildID, 'huh?')
+
+    def test_verifySlaveBuildID_notfound(self):
+        behavior = self._makeBehavior()
+        self.assertRaises(CorruptBuildID, behavior.verifySlaveBuildID, '1-1')
+
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)