launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/launchpad/initialize-distroseries-retry into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/launchpad/initialize-distroseries-retry/+merge/64738

When trying to create a new InitializeDistroSeriesJob an InitializationPending will be raised if a pending job already exists. If a completed job already exists, InitializationCompleted will be raised. Both of these exceptions carry a "job" instance variable that callers can use to aid in reporting.

If an existing failed job exists, it is deleted (there is a database constraint to prevent two initialization jobs existing for a single distroseries) and a new job is scheduled.

-- 
https://code.launchpad.net/~allenap/launchpad/initialize-distroseries-retry/+merge/64738
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/initialize-distroseries-retry into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-06-13 09:43:22 +0000
+++ database/schema/security.cfg	2011-06-15 19:23:30 +0000
@@ -167,7 +167,7 @@
 public.databasereplicationlag           = SELECT
 public.diff                             = SELECT, INSERT, UPDATE
 public.distributionbounty               = SELECT, INSERT, UPDATE
-public.distributionjob                  = SELECT, INSERT
+public.distributionjob                  = SELECT, INSERT, DELETE
 public.distributionmirror               = SELECT, INSERT, UPDATE, DELETE
 public.distributionsourcepackage        = SELECT, INSERT, UPDATE, DELETE
 public.distributionsourcepackagecache   = SELECT

=== modified file 'lib/lp/soyuz/interfaces/distributionjob.py'
--- lib/lp/soyuz/interfaces/distributionjob.py	2011-06-09 10:50:25 +0000
+++ lib/lp/soyuz/interfaces/distributionjob.py	2011-06-15 19:23:30 +0000
@@ -10,6 +10,8 @@
     "IDistroSeriesDifferenceJobSource",
     "IInitializeDistroSeriesJob",
     "IInitializeDistroSeriesJobSource",
+    "InitializationCompleted",
+    "InitializationPending",
 ]
 
 from lazr.enum import (
@@ -76,6 +78,28 @@
         """)
 
 
+class InitializationPending(Exception):
+    """The initialization of the distroseries has already been scheduled.
+
+    :ivar job: The `InitializeDistroSeriesJob` that's already scheduled.
+    """
+
+    def __init__(self, job):
+        super(InitializationPending, self).__init__()
+        self.job = job
+
+
+class InitializationCompleted(Exception):
+    """The initialization of the distroseries has already been done.
+
+    :ivar job: The `InitializeDistroSeriesJob` that's already scheduled.
+    """
+
+    def __init__(self, job):
+        super(InitializationCompleted, self).__init__()
+        self.job = job
+
+
 class IInitializeDistroSeriesJobSource(IJobSource):
     """An interface for acquiring IInitializeDistroSeriesJobs."""
 

=== modified file 'lib/lp/soyuz/model/initializedistroseriesjob.py'
--- lib/lp/soyuz/model/initializedistroseriesjob.py	2011-06-09 10:52:17 +0000
+++ lib/lp/soyuz/model/initializedistroseriesjob.py	2011-06-15 19:23:30 +0000
@@ -16,11 +16,14 @@
     IMasterStore,
     IStore,
     )
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.soyuz.interfaces.distributionjob import (
     DistributionJobType,
     IInitializeDistroSeriesJob,
     IInitializeDistroSeriesJobSource,
+    InitializationCompleted,
+    InitializationPending,
     )
 from lp.soyuz.model.distributionjob import (
     DistributionJob,
@@ -41,6 +44,23 @@
                rebuild=False, overlays=(), overlay_pockets=(),
                overlay_components=()):
         """See `IInitializeDistroSeriesJob`."""
+        store = IMasterStore(DistributionJob)
+        # Only one InitializeDistroSeriesJob can be present at a time.
+        distribution_job = store.find(
+            DistributionJob, DistributionJob.job_id == Job.id,
+            DistributionJob.job_type == cls.class_job_type,
+            DistributionJob.distroseries_id == child.id).one()
+        if distribution_job is not None:
+            if distribution_job.job.status == JobStatus.FAILED:
+                # Delete the failed job to allow initialization of the series
+                # to be rescheduled.
+                store.remove(distribution_job)
+                store.remove(distribution_job.job)
+            elif distribution_job.job.status == JobStatus.COMPLETED:
+                raise InitializationCompleted(cls(distribution_job))
+            else:
+                raise InitializationPending(cls(distribution_job))
+        # Schedule the initialization.
         metadata = {
             'parents': parents,
             'arches': arches,
@@ -50,11 +70,10 @@
             'overlay_pockets': overlay_pockets,
             'overlay_components': overlay_components,
             }
-        job = DistributionJob(
-            child.distribution, child, cls.class_job_type,
-            metadata)
-        IMasterStore(DistributionJob).add(job)
-        return cls(job)
+        distribution_job = DistributionJob(
+            child.distribution, child, cls.class_job_type, metadata)
+        store.add(distribution_job)
+        return cls(distribution_job)
 
     @classmethod
     def getPendingJobsForDistroseries(cls, distroseries):
@@ -62,8 +81,7 @@
         return IStore(DistributionJob).find(
             DistributionJob,
             DistributionJob.job_id == Job.id,
-            DistributionJob.job_type ==
-                DistributionJobType.INITIALIZE_SERIES,
+            DistributionJob.job_type == cls.class_job_type,
             DistributionJob.distroseries_id == distroseries.id,
             Job._status.is_in(Job.PENDING_STATUSES))
 

=== modified file 'lib/lp/soyuz/tests/test_initializedistroseriesjob.py'
--- lib/lp/soyuz/tests/test_initializedistroseriesjob.py	2011-06-09 10:52:17 +0000
+++ lib/lp/soyuz/tests/test_initializedistroseriesjob.py	2011-06-15 19:23:30 +0000
@@ -3,7 +3,6 @@
 
 __metaclass__ = type
 
-from storm.exceptions import IntegrityError
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -17,8 +16,12 @@
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.model.job import Job
 from lp.soyuz.interfaces.distributionjob import (
+    IInitializeDistroSeriesJob,
     IInitializeDistroSeriesJobSource,
+    InitializationCompleted,
+    InitializationPending,
     )
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
@@ -26,6 +29,7 @@
 from lp.soyuz.scripts.initialize_distroseries import InitializationError
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import Provides
 
 
 class InitializeDistroSeriesJobTests(TestCaseWithFactory):
@@ -58,15 +62,44 @@
         queue."""
         return len(self._getJobs())
 
-    def test_create_only_creates_one(self):
-        parent = self.factory.makeDistroSeries()
-        distroseries = self.factory.makeDistroSeries()
-        # If there's already a InitializeDistroSeriesJob for a
-        # DistroSeries, InitializeDistroSeriesJob.create() won't create
-        # a new one.
-        self.job_source.create(distroseries, [parent.id])
-        self.job_source.create(distroseries, [parent.id])
-        self.assertRaises(IntegrityError, flush_database_caches)
+    def test_create_with_existing_job(self):
+        parent = self.factory.makeDistroSeries()
+        distroseries = self.factory.makeDistroSeries()
+        # If there's already a pending or completed InitializeDistroSeriesJob
+        # for a DistroSeries, InitializeDistroSeriesJob.create() raises an
+        # exception.
+        job = self.job_source.create(distroseries, [parent.id])
+        assert_create_fails = lambda exc_type: self.assertRaises(
+            exc_type, self.job_source.create, distroseries, [parent.id])
+        # JobStatus.WAITING -> fails
+        exception = assert_create_fails(InitializationPending)
+        self.assertThat(exception.job, Provides(IInitializeDistroSeriesJob))
+        self.assertEqual(distroseries, exception.job.distroseries)
+        self.assertIn(exception.job.job.status, Job.PENDING_STATUSES)
+        # JobStatus.RUNNING -> fails
+        job.start()
+        assert_create_fails(InitializationPending)
+        # JobStatus.SUSPENDED -> fails
+        job.suspend()
+        assert_create_fails(InitializationPending)
+        # JobStatus.COMPLETED -> fails
+        job.queue()
+        job.start()
+        job.complete()
+        assert_create_fails(InitializationCompleted)
+
+    def test_create_with_existing_failed_job(self):
+        parent = self.factory.makeDistroSeries()
+        distroseries = self.factory.makeDistroSeries()
+        # If there's already a failed InitializeDistroSeriesJob for a
+        # DistroSeries, InitializeDistroSeriesJob.create() schedules a new
+        # job.
+        job = self.job_source.create(distroseries, [parent.id])
+        # JobStatus.FAILED -> succeeds
+        job.start()
+        job.fail()
+        self.job_source.create(distroseries, [parent.id])
+        flush_database_caches()
 
     def test_run_with_previous_series_already_set(self):
         # InitializationError is raised if a parent series already exists