launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/pcj-queue-ordering into lp:launchpad.

Commit message:
Prioritise mass-sync copies below others, and allow others to jump the queue.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1064274 in Launchpad itself: "PCJ runner has no prioritisation facilities"
  https://bugs.launchpad.net/launchpad/+bug/1064274

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/pcj-queue-ordering/+merge/130108

== Summary ==

The PackageCopyJob queue is strictly first-come-first-served.  Package-copy requests can be starved for hours by a long mass-sync queue, of the kind that occurs when auto-syncing packages from Debian into Ubuntu at the start of an Ubuntu release cycle.

== Proposed fix ==

We went back and forth on this quite a bit on IRC (initially trying to increase capacity with more runners, only to find that all the obvious paths to that are blocked for one reason or another) and agreed that a sensible approach would be to allow single copies to pre-empt mass copies.  We generally don't care very much about the response time to mass copies, but people copying individual packages between PPAs expect something close to an interactive response time.

== Pre-implementation notes ==

Simply ordering the queue by copy_policy is the obvious approach, but it occurred to me that we also need to run the query more than once or else it'll simply pick up 3000 mass-sync copies right at the start and we have the same problem.  William Grant suggested that it would probably be reasonable to just select the first job each time round the loop, so I've done that.  Technically I suppose this has something like O(n^2 log n) complexity, but the numbers aren't such that I would expect this to be a serious problem in practice and at the moment the query step at the start of a PCJ run doesn't take enough time to show up in logs; if we turn out to have a problem then we could mitigate it by working in batches of (say) ten at a time.

== LOC Rationale ==

+40, but this should be the last substantial addition before we can get rid of synchronous PPA copies and delayed copies.

== Tests ==

bin/test -vvct lp.soyuz.tests.test_packagecopyjob

== Demo and Q/A ==

Copy a reasonably substantial number of packages (say, 10) from Ubuntu to a dogfood PPA, including binaries, using Archive.copyPackages.  Run the PCJ processor.  While it's running, copy another package between PPAs using the web UI or Archive.copyPackage.  Verify in the logs that the single copy jumps the queue.
-- 
https://code.launchpad.net/~cjwatson/launchpad/pcj-queue-ordering/+merge/130108
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/pcj-queue-ordering into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-10-04 05:50:31 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-10-17 13:01:44 +0000
@@ -206,13 +206,26 @@
 
     @classmethod
     def iterReady(cls):
-        """Iterate through all ready PackageCopyJobs."""
-        jobs = IStore(PackageCopyJob).find(
-            PackageCopyJob,
-            And(PackageCopyJob.job_type == cls.class_job_type,
-                PackageCopyJob.job == Job.id,
-                Job.id.is_in(Job.ready_jobs)))
-        return (cls(job) for job in jobs)
+        """Iterate through all ready PackageCopyJobs.
+
+        Even though it's slower, we repeat the query each time in order that
+        very long queues of mass syncs can be pre-empted by other jobs.
+        """
+        seen = set()
+        while True:
+            jobs = IStore(PackageCopyJob).find(
+                PackageCopyJob,
+                And(PackageCopyJob.job_type == cls.class_job_type,
+                    PackageCopyJob.job == Job.id,
+                    Job.id.is_in(Job.ready_jobs)))
+            jobs.order_by(PackageCopyJob.copy_policy)
+            for job in jobs:
+                if job.id not in seen:
+                    seen.add(job.id)
+                    yield cls(job)
+                    break
+            else:
+                break
 
     def getOopsVars(self):
         """See `IRunnableJob`."""

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-04 07:02:08 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-17 13:01:44 +0000
@@ -478,6 +478,33 @@
         emails = pop_notifications()
         self.assertEqual(len(emails), 1)
 
+    def test_iterReady_orders_by_copy_policy(self):
+        # iterReady prioritises mass-sync copies below anything else.
+        self.makeJob(copy_policy=PackageCopyPolicy.MASS_SYNC)
+        self.makeJob()
+        self.makeJob(copy_policy=PackageCopyPolicy.MASS_SYNC)
+        ready_jobs = list(getUtility(IPlainPackageCopyJobSource).iterReady())
+        self.assertEqual([
+            PackageCopyPolicy.INSECURE,
+            PackageCopyPolicy.MASS_SYNC,
+            PackageCopyPolicy.MASS_SYNC,
+            ], [job.copy_policy for job in ready_jobs])
+
+    def test_iterReady_preempt(self):
+        # Ordinary ("insecure") copy jobs that arrive in the middle of a
+        # long mass-sync run take precedence immediately.
+        for i in range(2):
+            self.makeJob(copy_policy=PackageCopyPolicy.MASS_SYNC)
+        iterator = getUtility(IPlainPackageCopyJobSource).iterReady()
+        self.assertEqual(
+            PackageCopyPolicy.MASS_SYNC, next(iterator).copy_policy)
+        self.makeJob()
+        self.assertEqual(
+            PackageCopyPolicy.INSECURE, next(iterator).copy_policy)
+        self.assertEqual(
+            PackageCopyPolicy.MASS_SYNC, next(iterator).copy_policy)
+        self.assertRaises(StopIteration, next, iterator)
+
     def test_getOopsVars(self):
         distroseries = self.factory.makeDistroSeries()
         archive1 = self.factory.makeArchive(distroseries.distribution)