Merge lp:~jtv/launchpad/bug-181368-parallelize into lp:launchpad

[Jeroen T. Vermeulen]

[r=gmb,leonardr][bug=181368] Parallel apt-ftparchive.

= Summary =

The archive publisher takes far longer to run than anyone would like.

It spends most of its time running apt-ftparchive, which writes index files for each CPU architecture in turn.

== Proposed fix ==

This branch parallelizes the apt-ftparchive process into concurrent invocations, one for each architecture. It's made easy by the earlier introduction of the CommandSpawner, a helper class that lets you issue commands in parallel and then wait for them all to complete.

== Pre-implementation notes ==

The idea was wgrant's. Since this is a little scary, I also discussed the idea with bigjools. To validate the operational side, mthaddon observed the memory footprint of a full apt-ftparchive run. It took up less than a percent of available memory.

== Implementation details ==

The command-line option that lets us limit apt-ftparchive to a single architecture was introduced in Maverick. Previous versions allowed the same thing through the configuration file, but using it that way would have involved rewriting configuration files. Backpatching the new option was considered the lesser risk.

For the purposes of apt-ftparchive, "source" is just another architecture.

== Tests ==

To exercise the change:
{{{
./bin/test -vvc lp.archivepublisher.tests.test_ftparchive
}}}

I removed a few assertions on runApt's return value. The method used to return apt-ftparchive's exit code, but it would raise an exception first if that exit code was not zero. Therefore it always returned zero. I removed it. A new test verifies that runApt does indeed raise an exception if the command fails.

== Demo and Q/A ==

This is not something we can easily reproduce without Soyuz skills. I rely entirely on Julian there.

He has already tested the change on dogfood, though only with one architecture (plus the "source" one, of course). We'll have to try a larger run with multiple architectures as part of regular Q/A.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/tests/test_ftparchive.py
  lib/lp/archivepublisher/ftparchive.py

So there!

[Graham Binns (gmb)]

From IRC:

[13:57] gmb: jtv: Most of the tests in TestFTPArchiveRunApt need comments, if you please. Otherwise I'm happy with the code, but I'm not convinced that I actually have enough domain knowledge to review this - my brain seems to keep skipping off it.
[13:58] gmb: jtv: I'll give it r=me, but you might want to get a review from someone who knows more about it than I do.

review: Approve (code)

[Leonard Richardson (leonardr)]

I echo Graham's comments. I found test_getArchitectureTags_contains_no_duplicates and test_getArchitectureTags_counts_any_architecture_enabled_once confusing to read, and we discussed on IRC a way to improve them with a helper method.

I also suspect that test_runApt_reports_failure proves too much. Can you really break this code by naming a distro arch series "--something"? If so, that's its own problem and should be fixed. Or does the error happen because the string "bogus-config" is passed in instead of a real config? In that case, the command might not be run at all, and you'll need another test to show what happens when the command runs and fails.

[Leonard Richardson (leonardr)]

r=me with these (hopefully) minor changes.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Discussed on IRC, but for the record: any way of breaking the script will do, and the bogus-config is enough.

Ordinary users are not in a position to set a bogus architecture tag, and the shell is not invoked to do clever/risky things with the script arguments. So the damage from setting one is limited to archive publication being prevented, by a user who has privileges to do that anyway.

=== modified file 'lib/lp/archivepublisher/model/ftparchive.py'
--- lib/lp/archivepublisher/model/ftparchive.py	2011-02-04 09:07:36 +0000
+++ lib/lp/archivepublisher/model/ftparchive.py	2011-02-25 12:00:23 +0000
@@ -102,6 +102,10 @@
 """
 
 
+class AptFTPArchiveFailure(Exception):
+    """Failure while running apt-ftparchive."""
+
+
 class FTPArchiveHandler:
     """Produces Sources and Packages files via apt-ftparchive.
 
@@ -135,34 +139,54 @@
         apt_config_filename = self.generateConfig(is_careful)
         self.runApt(apt_config_filename)
 
+    def _getArchitectureTags(self):
+        """List tags of all architectures enabled in this distro."""
+        archs = set()
+        for series in self.distro.series:
+            archs.update(set([
+                distroarchseries.architecturetag
+                for distroarchseries in series.enabled_architectures]))
+        return archs
+
     def runApt(self, apt_config_filename):
-        """Run apt in a subprocess and verify its return value. """
+        """Run apt-ftparchive in subprocesses.
+
+        :raise: AptFTPArchiveFailure if any of the apt-ftparchive
+            commands failed.
+        """
         self.log.debug("Filepath: %s" % apt_config_filename)
-        # XXX JeroenVermeulen 2011-02-01 bug=181368: Run parallel
-        # apt-ftparchive processes for the various architectures (plus
-        # source).
+
         stdout_handler = OutputLineHandler(self.log.debug, "a-f: ")
         stderr_handler = OutputLineHandler(self.log.warning, "a-f: ")
-        completion_handler = ReturnCodeReceiver()
-        command = [
+        base_command = [
             "apt-ftparchive",
             "--no-contents",
             "generate",
             apt_config_filename,
             ]
         spawner = CommandSpawner()
-        spawner.start(
-            command, stdout_handler=stdout_handler,
-            stderr_handler=stderr_handler,
-            completion_handler=completion_handler)
+
+        returncodes = {}
+        for arch_tag in self._getArchitectureTags().union(set(["source"])):
+            completion_handler = ReturnCodeReceiver()
+            returncodes[arch_tag] = completion_handler
+            command = base_command + ["--arch", arch_tag]
+            spawner.start(
+                command, stdout_handler=stdout_handler,
+                stderr_handler=stderr_handler,
+                completion_handler=completion_handler)
+
         spawner.complete()
         stdout_handler.finalize()
         stderr_handler.finalize()
-        if completion_handler.returncode != 0:
-            raise AssertionError(
-                "Failure from apt-ftparchive. Return code %s"
-                % completion_handler.returncode)
-        return completion_handler.returncode
+        failures = sorted([
+            (tag, receiver.returncode)
+            for tag, receiver in returncodes.iteritems()
+                if receiver.returncode != 0])
+        if len(failures) > 0:
+            by_arch = ["%s (returned %d)" % failure for failure in failures]
+            raise AptFTPArchiveFailure(
+                "Failure(s) from apt-ftparchive: %s" % ", ".join(by_arch))
 
     #
     # Empty Pocket Requests

=== modified file 'lib/lp/archivepublisher/tests/test_ftparchive.py'
--- lib/lp/archivepublisher/tests/test_ftparchive.py	2011-02-04 09:07:36 +0000
+++ lib/lp/archivepublisher/tests/test_ftparchive.py	2011-02-25 12:00:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for ftparchive.py"""
@@ -15,11 +15,18 @@
 from zope.component import getUtility
 
 from canonical.config import config
-from lp.services.log.logger import BufferLogger
-from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.services.log.logger import (
+    BufferLogger,
+    DevNullLogger,
+    )
+from canonical.testing.layers import (
+    LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.diskpool import DiskPool
 from lp.archivepublisher.model.ftparchive import (
+    AptFTPArchiveFailure,
     f_touch,
     FTPArchiveHandler,
     )
@@ -290,7 +297,7 @@
         # those kind of tests and avoid to run it when performing 'make
         # check'. Although they should remain active in PQM to avoid possible
         # regressions.
-        assert fa.runApt(apt_conf) == 0
+        fa.runApt(apt_conf)
         self._verifyFile("Packages",
             os.path.join(self._distsdir, "hoary-test", "main", "binary-i386"))
         self._verifyFile("Sources",
@@ -305,7 +312,7 @@
         assert len(file(apt_conf).readlines()) == 23
 
         # XXX cprov 2007-03-21: see above, do not run a-f on dev machines.
-        assert fa.runApt(apt_conf) == 0
+        fa.runApt(apt_conf)
 
     def test_generateConfig_empty_and_careful(self):
         # Generate apt-ftparchive config for an specific empty suite.
@@ -359,20 +366,98 @@
         self.assertEqual(apt_conf_content, sample_content)
 
         # XXX cprov 2007-03-21: see above, do not run a-f on dev machines.
-        self.assertEqual(fa.runApt(apt_conf), 0)
-        self.assertTrue(os.path.exists(
-            os.path.join(self._distsdir, "hoary-test-updates", "main",
-                         "binary-i386", "Packages")))
-        self.assertTrue(os.path.exists(
-            os.path.join(self._distsdir, "hoary-test-updates", "main",
-                         "source", "Sources")))
-
-        self.assertFalse(os.path.exists(
-            os.path.join(self._distsdir, "hoary-test", "main",
-                         "binary-i386", "Packages")))
-        self.assertFalse(os.path.exists(
-            os.path.join(self._distsdir, "hoary-test", "main",
-                         "source", "Sources")))
+        fa.runApt(apt_conf)
+        self.assertTrue(os.path.exists(
+            os.path.join(self._distsdir, "hoary-test-updates", "main",
+                         "binary-i386", "Packages")))
+        self.assertTrue(os.path.exists(
+            os.path.join(self._distsdir, "hoary-test-updates", "main",
+                         "source", "Sources")))
+
+        self.assertFalse(os.path.exists(
+            os.path.join(self._distsdir, "hoary-test", "main",
+                         "binary-i386", "Packages")))
+        self.assertFalse(os.path.exists(
+            os.path.join(self._distsdir, "hoary-test", "main",
+                         "source", "Sources")))
+
+
+class TestFTPArchiveRunApt(TestCaseWithFactory):
+    """Test `FTPArchive`'s execution of apt-ftparchive."""
+
+    layer = ZopelessDatabaseLayer
+
+    def _makeMatchingDistroArchSeries(self):
+        """Create two `DistroArchSeries` for the same distro and processor."""
+        distro = self.factory.makeDistribution()
+        processor = self.factory.makeProcessor()
+        return (
+            self.factory.makeDistroArchSeries(
+                distroseries=self.factory.makeDistroSeries(distro),
+                processorfamily=processor.family,
+                architecturetag=processor.name)
+            for counter in (1, 2))
+
+    def test_getArchitectureTags_starts_out_empty(self):
+        fa = FTPArchiveHandler(
+            DevNullLogger(), None, None, self.factory.makeDistribution(),
+            None)
+        self.assertContentEqual([], fa._getArchitectureTags())
+
+    def test_getArchitectureTags_includes_enabled_architectures(self):
+        distroarchseries = self.factory.makeDistroArchSeries()
+        fa = FTPArchiveHandler(
+            DevNullLogger(), None, None,
+            distroarchseries.distroseries.distribution, None)
+        self.assertContentEqual(
+            [distroarchseries.architecturetag], fa._getArchitectureTags())
+
+    def test_getArchitectureTags_considers_all_series(self):
+        distro = self.factory.makeDistribution()
+        affluent_antilope = self.factory.makeDistroSeries(distribution=distro)
+        bilious_baboon = self.factory.makeDistroSeries(distribution=distro)
+        affluent_arch = self.factory.makeDistroArchSeries(
+            distroseries=affluent_antilope)
+        bilious_arch = self.factory.makeDistroArchSeries(
+            distroseries=bilious_baboon)
+        fa = FTPArchiveHandler(DevNullLogger(), None, None, distro, None)
+        self.assertContentEqual(
+            [affluent_arch.architecturetag, bilious_arch.architecturetag],
+            fa._getArchitectureTags())
+
+    def test_getArchitectureTags_ignores_disabled_architectures(self):
+        distroarchseries = self.factory.makeDistroArchSeries()
+        distroarchseries.enabled = False
+        fa = FTPArchiveHandler(
+            DevNullLogger(), None, None,
+            distroarchseries.distroseries.distribution, None)
+        self.assertContentEqual([], fa._getArchitectureTags())
+
+    def test_getArchitectureTags_contains_no_duplicates(self):
+        ominous_okapi, pilfering_puppy = self._makeMatchingDistroArchSeries()
+        fa = FTPArchiveHandler(
+            DevNullLogger(), None, None,
+            ominous_okapi.distroseries.distribution, None)
+        self.assertEqual(1, len(list(fa._getArchitectureTags())))
+        self.assertContentEqual(
+            [ominous_okapi.architecturetag], fa._getArchitectureTags())
+
+    def test_getArchitectureTags_counts_any_architecture_enabled_once(self):
+        manic_mantis, nervous_nit = self._makeMatchingDistroArchSeries()
+        nervous_nit.enabled = False
+        fa = FTPArchiveHandler(
+            DevNullLogger(), None, None,
+            manic_mantis.distroseries.distribution, None)
+        self.assertContentEqual(
+            [manic_mantis.architecturetag], fa._getArchitectureTags())
+
+    def test_runApt_reports_failure(self):
+        # If we sabotage apt-ftparchive, runApt notices that it failed
+        # and raises an exception.
+        distroarchseries = self.factory.makeDistroArchSeries()
+        distro = distroarchseries.distroseries.distribution
+        fa = FTPArchiveHandler(DevNullLogger(), None, None, distro, None)
+        self.assertRaises(AptFTPArchiveFailure, fa.runApt, "bogus-config")
 
 
 class TestFTouch(unittest.TestCase):