Merge lp:~jtv/launchpad/db-bug-782096 into lp:launchpad/db-devel

[Jeroen T. Vermeulen]

[r=jcsackett][bug=782096] Active PlainPackageCopyJobs must be in WAITING state, and sort oldest first.

= Summary =

IPlainPackageCopyJobSource.getActiveJobs() had two flaws that made it a poor way to fetch the next job to execute:

1. It failed to check for job status, so unless finished jobs were immediately deleted, it would pick up completed or failed jobs.

2. It failed to impose a sane ordering, so it wouldn't necessarily pick up the oldest available job.

This fixes that.

The test uses the makeJob helper, which isn't actually in db-devel at the time of writing. But it's on the way through EC2 and should land soon. Tests pass when I merge in that branch, but I wanted to keep the diff minimal so I left it out. (There were no text conflicts to complicate this, luckily).

== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob
}}}

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/soyuz/tests/test_packagecopyjob.py

[j.c.sackett (jcsackett)]

With the return of the order_by, this looks good to land.

review: Approve

=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py	2011-05-18 18:39:03 +0000
+++ lib/lp/registry/browser/distroseries.py	2011-05-19 17:24:21 +0000
@@ -105,6 +105,7 @@
 from lp.soyuz.browser.packagesearch import PackageSearchViewBase
 from lp.soyuz.interfaces.packagecopyjob import IPlainPackageCopyJobSource
 from lp.soyuz.interfaces.queue import IPackageUploadSet
+from lp.soyuz.model.packagecopyjob import specify_dsd_package
 from lp.soyuz.model.queue import PackageUploadQueue
 from lp.translations.browser.distroseries import (
     check_distroseries_translations_viewable,
@@ -851,6 +852,25 @@
         return (has_perm and
                 self.cached_differences.batch.total() > 0)
 
+    @cachedproperty
+    def pending_syncs(self):
+        """Pending synchronization jobs for this distroseries.
+
+        :return: A dict mapping (name, version) package specifications to
+            pending sync jobs.
+        """
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        return job_source.getPendingJobsPerPackage(self.context)
+
+    def hasPendingSync(self, dsd):
+        """Is there a package-copying job pending to resolve `dsd`?"""
+        return self.pending_syncs.get(specify_dsd_package(dsd)) is not None
+
+    def canRequestSync(self, dsd):
+        """Does it make sense to request a sync for this difference?"""
+        # XXX JeroenVermeulen bug=783435: Also compare versions.
+        return not self.hasPendingSync(dsd)
+
     @property
     def specified_name_filter(self):
         """If specified, return the name filter from the GET form data."""

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-19 02:36:21 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-19 17:24:21 +0000
@@ -72,6 +72,7 @@
     ISourcePackageFormatSelectionSet,
     )
 from lp.soyuz.model.archivepermission import ArchivePermission
+from lp.soyuz.model.packagecopyjob import specify_dsd_package
 from lp.testing import (
     anonymous_logged_in,
     celebrity_logged_in,
@@ -1348,6 +1349,32 @@
 
             self.assertFalse(view.canPerformSync())
 
+    def test_hasPendingSync_returns_False_if_no_pending_sync(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertFalse(view.hasPendingSync(dsd))
+
+    def test_hasPendingSync_returns_True_if_pending_sync(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.pending_syncs = {specify_dsd_package(dsd): object()}
+        self.assertTrue(view.hasPendingSync(dsd))
+
+    def test_canRequestSync_returns_False_if_pending_sync(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        view.pending_syncs = {specify_dsd_package(dsd): object()}
+        self.assertFalse(view.canRequestSync(dsd))
+
+    def test_canRequestSync_returns_True_if_sync_makes_sense(self):
+        dsd = self.factory.makeDistroSeriesDifference()
+        view = create_initialized_view(
+            dsd.derived_series, '+localpackagediffs')
+        self.assertTrue(view.canRequestSync(dsd))
+
     def _syncAndGetView(self, derived_series, person, sync_differences,
                         difference_type=None, view_name='+localpackagediffs'):
         # A helper to get the POST'ed sync view.

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-13 15:13:54 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-19 17:24:21 +0000
@@ -74,14 +74,20 @@
                 tal:attributes="class src_name">
 
               <td>
-                <input tal:condition="can_perform_sync"
-                  name="field.selected_differences" type="checkbox"
-                  tal:attributes="
-                    value diff_id;
-                    id string:field.selected_differences.${diff_id}"/>
+                <tal:checkbox tal:condition="python: view.canRequestSync(difference)">
+                  <input tal:condition="can_perform_sync"
+                    name="field.selected_differences" type="checkbox"
+                    tal:attributes="
+                      value diff_id;
+                      id string:field.selected_differences.${diff_id}"/>
 
-                <a tal:attributes="href difference/fmt:url" class="toggle-extra"
-                   tal:content="src_name">Foo</a>
+                  <a tal:attributes="href difference/fmt:url" class="toggle-extra"
+                     tal:content="src_name">Foo</a>
+                </tal:checkbox>
+                <tal:clockface condition="python: view.hasPendingSync(difference)">
+                  <span class="sprite build_depwait"></span>
+                  <i>(Synchronizing)</i>
+                </tal:clockface>
               </td>
               <td tal:condition="python: not(view.has_unique_parent) and view.show_parent_version">
                 <a tal:attributes="href difference/parent_series/fmt:url"

=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py	2011-05-13 08:21:38 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py	2011-05-19 17:24:21 +0000
@@ -103,6 +103,21 @@
     def getActiveJobs(target_archive):
         """Retrieve all active sync jobs for an archive."""
 
+    def getPendingJobsPerPackage(target_series):
+        """Find pending jobs for each package in `target_series`.
+
+        This is meant for finding jobs that will resolve specific
+        `DistroSeriesDifference`s, so see also `specify_dsd_package`.
+
+        :param target_series: Target `DistroSeries`; this corresponds to
+            `DistroSeriesDifference.derived_series`.
+        :return: A dict containing as keys the (name, version) tuples for
+            each `DistroSeriesDifference` that has a resolving
+            `PlainPackageCopyJob` pending.  Each of these DSDs maps to its
+            oldest pending job.  The `version` corresponds to
+            `DistroSeriesDifference.parent_source_version`.
+        """
+
 
 class IPlainPackageCopyJob(IRunnableJob):
     """A no-frills job to copy packages between `IArchive`s."""

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-19 17:24:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,6 +6,7 @@
 __all__ = [
     "PackageCopyJob",
     "PlainPackageCopyJob",
+    "specify_dsd_package",
 ]
 
 from lazr.delegates import delegates
@@ -33,6 +34,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
 from lp.services.database.stormbase import StormBase
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
 from lp.soyuz.interfaces.archive import CannotCopy
@@ -46,6 +48,17 @@
 from lp.soyuz.scripts.packagecopier import do_copy
 
 
+def specify_dsd_package(dsd):
+    """Return (name, parent version) for `dsd`'s package.
+
+    This describes the package that `dsd` is for in a format suitable for
+    `PlainPackageCopyJobSource`.
+
+    :param dsd: A `DistroSeriesDifference`.
+    """
+    return (dsd.source_package_name.name, dsd.parent_source_version)
+
+
 class PackageCopyJob(StormBase):
     """Base class for package copying jobs."""
 
@@ -171,9 +184,37 @@
         jobs = IStore(PackageCopyJob).find(
             PackageCopyJob,
             PackageCopyJob.job_type == cls.class_job_type,
-            PackageCopyJob.target_archive == target_archive)
+            PackageCopyJob.target_archive == target_archive,
+            Job.id == PackageCopyJob.job_id,
+            Job._status == JobStatus.WAITING)
+        jobs = jobs.order_by(PackageCopyJob.id)
         return DecoratedResultSet(jobs, cls)
 
+    @classmethod
+    def getPendingJobsForTargetSeries(cls, target_series):
+        """Get upcoming jobs for `target_series`, ordered by age."""
+        raw_jobs = IStore(PackageCopyJob).find(
+            PackageCopyJob,
+            Job.id == PackageCopyJob.job_id,
+            PackageCopyJob.job_type == cls.class_job_type,
+            PackageCopyJob.target_distroseries == target_series,
+            Job._status.is_in(Job.PENDING_STATUSES))
+        raw_jobs = raw_jobs.order_by(PackageCopyJob.id)
+        return DecoratedResultSet(raw_jobs, cls)
+
+    @classmethod
+    def getPendingJobsPerPackage(cls, target_series):
+        """See `IPlainPackageCopyJobSource`."""
+        result = {}
+        # Go through jobs in-order, picking the first matching job for
+        # any (package, version) tuple.  Because of how
+        # getPendingJobsForTargetSeries orders its results, the first
+        # will be the oldest and thus presumably the first to finish.
+        for job in cls.getPendingJobsForTargetSeries(target_series):
+            for package in job.metadata["source_packages"]:
+                result.setdefault(tuple(package), job)
+        return result
+
     @property
     def source_packages(self):
         getPublishedSources = self.source_archive.getPublishedSources

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-19 17:24:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for sync package jobs."""
@@ -10,12 +10,14 @@
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.archive import CannotCopy
 from lp.soyuz.interfaces.packagecopyjob import (
     IPackageCopyJob,
     IPlainPackageCopyJobSource,
     )
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.model.packagecopyjob import specify_dsd_package
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
     run_script,
@@ -28,6 +30,17 @@
 
     layer = LaunchpadZopelessLayer
 
+    def makeJob(self, dsd):
+        """Create a `PlainPackageCopyJob` that would resolve `dsd`."""
+        source_packages = [specify_dsd_package(dsd)]
+        source_archive = dsd.parent_series.main_archive
+        target_archive = dsd.derived_series.main_archive
+        target_distroseries = dsd.derived_series
+        target_pocket = self.factory.getAnyPocket()
+        return getUtility(IPlainPackageCopyJobSource).create(
+            source_packages, source_archive, target_archive,
+            target_distroseries, target_pocket)
+
     def test_create(self):
         # A PackageCopyJob can be created and stores its arguments.
         distroseries = self.factory.makeDistroSeries()
@@ -65,6 +78,21 @@
             include_binaries=False)
         self.assertContentEqual([job], source.getActiveJobs(archive2))
 
+    def test_getActiveJobs_gets_oldest_first(self):
+        # getActiveJobs returns the oldest available job first.
+        dsd = self.factory.makeDistroSeriesDifference()
+        target_archive = dsd.derived_series.main_archive
+        jobs = [self.makeJob(dsd) for counter in xrange(2)]
+        source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(jobs[0], source.getActiveJobs(target_archive)[0])
+
+    def test_getActiveJobs_only_returns_waiting_jobs(self):
+        # getActiveJobs ignores jobs that aren't in the WAITING state.
+        job = self.makeJob(self.factory.makeDistroSeriesDifference())
+        removeSecurityProxy(job).job._status = JobStatus.RUNNING
+        source = getUtility(IPlainPackageCopyJobSource)
+        self.assertContentEqual([], source.getActiveJobs(job.target_archive))
+
     def test_run_unknown_package(self):
         # A job properly records failure.
         distroseries = self.factory.makeDistroSeries()
@@ -200,3 +228,75 @@
                 distroseries=distroseries, archive1=archive1,
                 archive2=archive2),
             repr(job))
+
+    def test_getPendingJobsPerPackage_finds_jobs(self):
+        # getPendingJobsPerPackage finds jobs, and the packages they
+        # belong to.
+        dsd = self.factory.makeDistroSeriesDifference()
+        job = self.makeJob(dsd)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {specify_dsd_package(dsd): job},
+            job_source.getPendingJobsPerPackage(dsd.derived_series))
+
+    def test_getPendingJobsPerPackage_ignores_other_distroseries(self):
+        # getPendingJobsPerPackage only looks for jobs on the indicated
+        # distroseries.
+        dsd = self.factory.makeDistroSeriesDifference()
+        self.makeJob(dsd)
+        other_series = self.factory.makeDistroSeries()
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {}, job_source.getPendingJobsPerPackage(other_series))
+
+    def test_getPendingJobsPerPackage_only_returns_pending_jobs(self):
+        # getPendingJobsPerPackage ignores jobs that have already been
+        # run.
+        dsd = self.factory.makeDistroSeriesDifference()
+        package = specify_dsd_package(dsd)
+        job = self.makeJob(dsd)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        found_by_state = {}
+        for status in JobStatus.items:
+            removeSecurityProxy(job).job._status = status
+            result = job_source.getPendingJobsPerPackage(dsd.derived_series)
+            if len(result) > 0:
+                found_by_state[status] = result[package]
+        expected = {
+            JobStatus.WAITING: job,
+            JobStatus.RUNNING: job,
+            JobStatus.SUSPENDED: job,
+        }
+        self.assertEqual(expected, found_by_state)
+
+    def test_getPendingJobsPerPackage_distinguishes_jobs(self):
+        # getPendingJobsPerPackage associates the right job with the
+        # right package.
+        derived_series = self.factory.makeDistroSeries()
+        dsds = [
+            self.factory.makeDistroSeriesDifference(
+                derived_series=derived_series)
+            for counter in xrange(2)]
+        jobs = map(self.makeJob, dsds)
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            dict(zip(map(specify_dsd_package, dsds), jobs)),
+            job_source.getPendingJobsPerPackage(derived_series))
+
+    def test_getPendingJobsPerPackage_picks_oldest_job_for_dsd(self):
+        # If there are multiple jobs for one package,
+        # getPendingJobsPerPackage picks the oldest.
+        dsd = self.factory.makeDistroSeriesDifference()
+        jobs = [self.makeJob(dsd) for counter in xrange(2)]
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {specify_dsd_package(dsd): jobs[0]},
+            job_source.getPendingJobsPerPackage(dsd.derived_series))
+
+    def test_getPendingJobsPerPackage_ignores_dsds_without_jobs(self):
+        # getPendingJobsPerPackage produces no dict entry for packages
+        # that have no pending jobs, even if they do have DSDs.
+        dsd = self.factory.makeDistroSeriesDifference()
+        job_source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(
+            {}, job_source.getPendingJobsPerPackage(dsd.derived_series))