Merge lp:~stevenk/launchpad/queue-recipes-better into lp:launchpad

[Steve Kowalik]

[r=lifeless,wgrant][ui=none][bug=710435] Teach makeDailyBuilds() to not create a build if the recipe has built into its daily build archive within the past 24 hours.

Change findStaleDailyBuilds() to only return builds which are interesting for a particular run of request_daily_builds -- that is, only returns recipes that either don't have a build, or have one that is older than 24 hours. I have also done a few drive-bys.

[Robert Collins (lifeless)]

So, whats this intended to achieve? As I read it it will result in builds building once every two days. That seems undesirable and confusing.

review: Needs Information

[Steve Kowalik (stevenk)]

The point of this branch is to foster work to getting the request_daily_builds script running more than once per day. To do so we need to skip daily builds which aren't old enough.

[Robert Collins (lifeless)]

On Tue, Jan 25, 2011 at 6:43 PM, Steve Kowalik <email address hidden> wrote:
> The point of this branch is to foster work to getting the request_daily_builds script running more than once per day. To do so we need to skip daily builds which aren't old enough.

That implies that we'll be considering builds which are not
interesting to build, which is inefficient. I don't think this is a
useful step towards where you want to get. Better to only iterate
recipes which are interesting to build on a given run - which won't
need any of the changes of this patch, and will be sufficient in and
of itself.

Seperately, remember than changing something takes time - a 24 hour
interval isn't 24 hours, its really 24 hours - the time we take to do
stuff - otherwise folk will get built one day, not the next, and so
on.

-Rob

[William Grant (wgrant)]

Thanks for this change, Steve. However, findStaleDailyBuilds is too liberal in what it considers to have been built within the last day: it needs to check that it was built for all daily series, and into the daily build archive.

A few other largely stylistic comments:

 - requestBuild should use Archive.default_component, not IComponentSet directly.
 - The join is unreadable; could you please fix up the indentation?
 - Why use a RightJoin instead of a LeftJoin? There is not a single other RightJoin callsite in the tree.
 - There is some trailing whitespace in findStaleDailyBuilds, and possibly other places too. 'make lint' should pick this up.

review: Needs Fixing

[Robert Collins (lifeless)]

Mentor review.

I think this is fine to land with a few small tweaks.

William: meta on the review - we try not to escalate the work done in a branch except to avoid regressions; in particular the defects about series and archive are already present in the tree; they are neutral as far as this branch is concerned. I agree that they are defects, but not new to the branch and so deferrable. When possible suggest actual fixes with diffs/something to copy and paste in. Lastly some of the prose could be improved - comments in particular can bitrot so its worth making sure they really are clear.

As for the right join, thats because this is appropriate; we have a really weird one-object,three-table split in the DB at the moment, and we need to treat the composed table of all three things as a single object: we can't left join with it because the condition for restricting interesting builds is to the right of one of the partitions. RightJoin is appropriate here from a SQL perspective - the generated query runs in 4-6ms on qastaging.

Steven: I agree about the join indentation. How about this:

+ joins = RightJoin(
+ Join(
+ Join(SourcePackageRecipeBuild, PackageBuild,
+ PackageBuild.id == SourcePackageRecipeBuild.package_build_id),
+ BuildFarmJob,
+ And(BuildFarmJob.id == PackageBuild.build_farm_job_id,
+ BuildFarmJob.date_created > one_day_ago)),
+ SourcePackageRecipe,
+ SourcePackageRecipeBuild.recipe == SourcePackageRecipe.id)

200 + # makeDailyBuilds() won't create a build if the recipe isn't stale.

->

200 + # only stale builds cause makeDailyBuilds() to make new builds

review: Approve

[William Grant (wgrant)]

The defects are new, since requesting a single build did not previously unset is_stale.

[Robert Collins (lifeless)]

On Mon, Jan 31, 2011 at 4:13 PM, William Grant <email address hidden> wrote:
> The defects are new, since requesting a single build did not previously unset is_stale.

Good catch, we do need to fix them - sorry that I misunderstood.

[William Grant (wgrant)]

I'd personally prefer it to be indented like http://pastebin.ubuntu.com/560770/. But this looks fine to land.

review: Approve (code*)

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-01-14 05:46:14 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-02-01 01:40:41 +0000
@@ -11,7 +11,17 @@
     'SourcePackageRecipe',
     ]
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
 from lazr.delegates import delegates
+from pytz import utc
+from storm.expr import (
+    And,
+    Join,
+    RightJoin,
+    )
 from storm.locals import (
     Bool,
     Desc,
@@ -56,7 +66,6 @@
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.model.distroseries import DistroSeries
 from lp.soyuz.interfaces.archive import IArchiveSet
-from lp.soyuz.interfaces.component import IComponentSet
 
 
 def get_buildable_distroseries_set(user):
@@ -178,8 +187,22 @@
 
     @classmethod
     def findStaleDailyBuilds(cls):
-        store = IStore(cls)
-        return store.find(cls, cls.is_stale == True, cls.build_daily == True)
+        one_day_ago = datetime.now(utc) - timedelta(hours=23, minutes=50)
+        joins = RightJoin(
+            Join(
+                Join(SourcePackageRecipeBuild, PackageBuild,
+                    PackageBuild.id ==
+                    SourcePackageRecipeBuild.package_build_id),
+                BuildFarmJob,
+                And(BuildFarmJob.id == PackageBuild.build_farm_job_id,
+                    BuildFarmJob.date_created > one_day_ago)),
+            SourcePackageRecipe,
+            And(SourcePackageRecipeBuild.recipe == SourcePackageRecipe.id,
+                SourcePackageRecipe.daily_build_archive_id ==
+                    PackageBuild.archive_id))
+        return IStore(cls).using(joins).find(
+            cls, cls.is_stale == True, cls.build_daily == True,
+            BuildFarmJob.date_created == None).config(distinct=True)
 
     @staticmethod
     def exists(owner, name):
@@ -216,14 +239,14 @@
             raise ValueError('Source package recipe builds disabled.')
         if not archive.is_ppa:
             raise NonPPABuildRequest
-        component = getUtility(IComponentSet)["multiverse"]
 
         buildable_distros = get_buildable_distroseries_set(archive.owner)
         if distroseries not in buildable_distros:
             raise BuildNotAllowedForDistro(self, distroseries)
 
         reject_reason = archive.checkUpload(
-            requester, self.distroseries, None, component, pocket)
+            requester, self.distroseries, None, archive.default_component,
+            pocket)
         if reject_reason is not None:
             raise reject_reason
         if self.isOverQuota(requester, distroseries):
@@ -271,14 +294,8 @@
 
     def getLastBuild(self):
         """See `ISourcePackageRecipeBuild`."""
-        store = Store.of(self)
-        result = store.find(
-            (SourcePackageRecipeBuild),
-            SourcePackageRecipeBuild.recipe == self,
-            SourcePackageRecipeBuild.package_build_id == PackageBuild.id,
-            PackageBuild.build_farm_job_id == BuildFarmJob.id)
-        result.order_by(Desc(BuildFarmJob.date_finished))
-        return result.first()
+        return self._getBuilds(
+            True, Desc(BuildFarmJob.date_finished)).first()
 
     def getMedianBuildDuration(self):
         """Return the median duration of builds of this recipe."""
 
=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2011-01-20 22:01:29 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2011-02-01 01:40:41 +0000
@@ -10,7 +10,10 @@
     'SourcePackageRecipeBuild',
     ]
 
-import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import logging
 import sys
 
@@ -259,9 +262,9 @@
     def getRecentBuilds(cls, requester, recipe, distroseries, _now=None):
         from lp.buildmaster.model.buildfarmjob import BuildFarmJob
         if _now is None:
-            _now = datetime.datetime.now(utc)
+            _now = datetime.now(utc)
         store = IMasterStore(SourcePackageRecipeBuild)
-        old_threshold = _now - datetime.timedelta(days=1)
+        old_threshold = _now - timedelta(days=1)
         return store.find(cls, cls.distroseries_id == distroseries.id,
             cls.requester_id == requester.id, cls.recipe_id == recipe.id,
             BuildFarmJob.date_created > old_threshold,
@@ -282,7 +285,7 @@
         median = self.recipe.getMedianBuildDuration()
         if median is not None:
             return median
-        return datetime.timedelta(minutes=10)
+        return timedelta(minutes=10)
 
     def verifySuccessfulUpload(self):
         return self.source_package_release is not None
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-01-13 03:53:53 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-02-01 01:40:41 +0000
@@ -561,6 +561,22 @@
         self.assertContentEqual([stale_daily],
             SourcePackageRecipe.findStaleDailyBuilds())
 
+    def test_findStaleDailyBuildsDistinct(self):
+        # If a recipe has 2 builds due to 2 distroseries, it only returns
+        # one recipe.
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
+        hoary = self.factory.makeSourcePackageRecipeDistroseries("hoary")
+        recipe.distroseries.add(hoary)
+        for series in recipe.distroseries:
+            build = recipe.requestBuild(
+                recipe.daily_build_archive, recipe.owner,
+                series, PackagePublishingPocket.RELEASE)
+            removeSecurityProxy(build).date_created = (
+                datetime.now(UTC) - timedelta(hours=24, seconds=1))
+        stale_recipes = SourcePackageRecipe.findStaleDailyBuilds()
+        self.assertEqual([recipe], list(stale_recipes))
+
     def test_getMedianBuildDuration(self):
 
         def set_duration(build, minutes):
 
=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-01-20 22:01:29 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2011-02-01 01:40:41 +0000
@@ -5,9 +5,12 @@
 
 __metaclass__ = type
 
-import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
+from pytz import utc
 import re
-import unittest
 
 from storm.locals import Store
 import transaction
@@ -177,16 +180,13 @@
         # If there are no successful builds, estimate 10 minutes.
         spb = self.makeSourcePackageRecipeBuild()
         cur_date = self.factory.getUniqueDate()
-        self.assertEqual(
-            datetime.timedelta(minutes=10), spb.estimateDuration())
+        self.assertEqual(timedelta(minutes=10), spb.estimateDuration())
         for minutes in [20, 5, 1]:
             build = removeSecurityProxy(
                 self.factory.makeSourcePackageRecipeBuild(recipe=spb.recipe))
             build.date_started = cur_date
-            build.date_finished = (
-                cur_date + datetime.timedelta(minutes=minutes))
-        self.assertEqual(
-            datetime.timedelta(minutes=5), spb.estimateDuration())
+            build.date_finished = cur_date + timedelta(minutes=minutes)
+        self.assertEqual(timedelta(minutes=5), spb.estimateDuration())
 
     def test_getFileByName(self):
         """getFileByName returns the logs when requested by name."""
@@ -269,20 +269,74 @@
         SourcePackageRecipeBuild.makeDailyBuilds()[0]
         self.assertFalse(recipe.is_stale)
 
-    def test_makeDailyBuilds_skips_pending(self):
-        """When creating daily builds, skip ones that are already pending."""
+    def test_makeDailyBuilds_skips_if_built_in_last_24_hours(self):
+        # We won't create a build during makeDailyBuilds() if the recipe
+        # has been built in the last 24 hours.
         recipe = self.factory.makeSourcePackageRecipe(
             build_daily=True, is_stale=True)
-        first_distroseries = list(recipe.distroseries)[0]
         recipe.requestBuild(
-            recipe.daily_build_archive, recipe.owner, first_distroseries,
+            recipe.daily_build_archive, recipe.owner,
+            list(recipe.distroseries)[0], PackagePublishingPocket.RELEASE)
+        daily_builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        self.assertEqual([], daily_builds)
+
+    def test_makeDailyBuilds_skips_non_stale_builds(self):
+        # If the recipe isn't stale, makeDailyBuilds() won't create a build.
+        self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=False)
+        daily_builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        self.assertEqual([], daily_builds)
+
+    def test_makeDailyBuilds_with_an_older_build(self):
+        # If a previous build is more than 24 hours old, and the recipe is
+        # stale, we'll fire another off.
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
+        build = recipe.requestBuild(
+            recipe.daily_build_archive, recipe.owner,
+            list(recipe.distroseries)[0], PackagePublishingPocket.RELEASE)
+        nb = removeSecurityProxy(build)
+        nb.date_created = datetime.now(utc) - timedelta(hours=24, seconds=1)
+        # The build also needs to be completed
+        nb.status = BuildStatus.FULLYBUILT
+        daily_builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        self.assertEquals(1, len(daily_builds))
+        actual_title = [b.title for b in daily_builds]
+        self.assertEquals([build.title], actual_title)
+
+    def test_makeDailyBuilds_with_an_older_and_newer_build(self):
+        # If a recipe has been built twice, and the most recent build is
+        # within 24 hours, makeDailyBuilds() won't create a build.
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
+        for timediff in (timedelta(hours=24, seconds=1), timedelta(hours=8)):
+            build = recipe.requestBuild(
+                recipe.daily_build_archive, recipe.owner,
+                list(recipe.distroseries)[0],
+                PackagePublishingPocket.RELEASE)
+            nb = removeSecurityProxy(build)
+            nb.date_created = datetime.now(utc) - timediff
+            # The build also needs to be completed
+            nb.status = BuildStatus.FULLYBUILT
+        daily_builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        self.assertEquals([], list(daily_builds))
+
+    def test_makeDailyBuilds_with_new_build_different_archive(self):
+        # If a recipe has been built into an archive that isn't the
+        # daily_build_archive, we will create a build.
+        recipe = self.factory.makeSourcePackageRecipe(
+            build_daily=True, is_stale=True)
+        archive = self.factory.makeArchive(owner=recipe.owner)
+        build = recipe.requestBuild(
+            archive, recipe.owner, list(recipe.distroseries)[0],
             PackagePublishingPocket.RELEASE)
-        second_distroseries = \
-            self.factory.makeSourcePackageRecipeDistroseries("hoary")
-        recipe.distroseries.add(second_distroseries)
-        builds = SourcePackageRecipeBuild.makeDailyBuilds()
-        self.assertEqual(
-            [second_distroseries], [build.distroseries for build in builds])
+        nb = removeSecurityProxy(build)
+        nb.date_created = datetime.now(utc) - timedelta(hours=8)
+        # The build also needs to be completed
+        nb.status = BuildStatus.FULLYBUILT
+        daily_builds = SourcePackageRecipeBuild.makeDailyBuilds()
+        actual_title = [b.title for b in daily_builds]
+        self.assertEquals([build.title], actual_title)
 
     def test_getRecentBuilds(self):
         """Recent builds match the same person, series and receipe.
@@ -306,12 +360,12 @@
             return SourcePackageRecipeBuild.getRecentBuilds(
                 requester, recipe, series, _now=now)
         self.assertContentEqual([], get_recent())
-        yesterday = now - datetime.timedelta(days=1)
+        yesterday = now - timedelta(days=1)
         recent_build = self.factory.makeSourcePackageRecipeBuild(
             recipe=recipe, distroseries=series, requester=requester,
             date_created=yesterday)
         self.assertContentEqual([], get_recent())
-        a_second = datetime.timedelta(seconds=1)
+        a_second = timedelta(seconds=1)
         removeSecurityProxy(recent_build).date_created += a_second
         self.assertContentEqual([recent_build], get_recent())
 
@@ -363,7 +417,6 @@
         # getSpecificJob returns the SourcePackageRecipeBuild
         sprb = self.makeSourcePackageRecipeBuild()
         Store.of(sprb).flush()
-        build = sprb.build_farm_job
         job = sprb.build_farm_job.getSpecificJob()
         self.assertEqual(sprb, job)
 
@@ -443,7 +496,7 @@
         build = self.factory.makeSourcePackageRecipeBuild(
             distroseries=distroseries,
             status=BuildStatus.FULLYBUILT,
-            duration=datetime.timedelta(minutes=5))
+            duration=timedelta(minutes=5))
         build.queueBuild(build)
         return build
 
@@ -456,7 +509,3 @@
 class TestHandleStatusForSPRBuild(
     MakeSPRecipeBuildMixin, TestHandleStatusMixin, TrialTestCase):
     """IPackageBuild.handleStatus works with SPRecipe builds."""
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)