Merge lp:~cjwatson/launchpad/germinate-all-dev-series into lp:launchpad

[Colin Watson]

[r=wgrant][bug=904538] Let cron.germinate handle multiple dev series.

== Summary ==

My recent refactoring of cron.germinate caused some dogfood-specific problems, because dogfood has multiple Ubuntu distroseries in the DEVELOPMENT or FROZEN states.

== Proposed fix ==

Generate extra overrides for all DEVELOPMENT and FROZEN series, rather than for just one. Log but otherwise ignore any cases where missing seeds would previously have caused us to abort.

This seems like logically the right thing to do, and removes the obstacle to testing this easily on dogfood.

== Implementation details ==

I introduced a simple new CachedDistroSeries class to preserve the current property that most of generate-extra-overrides runs without a database transaction, without having to pass around more clumsy piles of arguments (otherwise, I was going to have to add a list of lists of components somewhere).

== Tests ==

bin/test -vvct generate_extra_overrides

== Demo and Q/A ==

Run cron.germinate on mawson. Given the current state of dogfood's database, it should emit log entries about the lack of seeds for rusty, and generate updated extra override files (in /srv/launchpad.net/ubuntu-archive/ubuntu-misc/more-extra.override.{oneiric,precise}.main) for precise and oneiric. Previously, it would fail due to the lack of seeds for rusty.

== lint ==

None.

[Robert Collins (lifeless)]

It would be better I think to cache those values in the main
DistroSeries, using e.g. @cachedproperty. Its a little more work to
validate, but centralises the caching.

[Colin Watson (cjwatson)]

Would it be better to just go back to having multiple lists and
iterating over them with zip() (my personal threshold for when I start
thinking about consolidating this kind of thing into a list of classes
seems to be about three, which is why I did it at this point), or is it
worth caching the property for other reasons anyway?

[Colin Watson (cjwatson)]

I played with @cachedproperty and friends a bit. I eventually decided that I wasn't massively keen on relying on the generational cache for correctness rather than just performance, and switching to the stupid cache started to seem like overengineering for the problem at hand. Instead, I just dropped the CachedDistroSeries business and fetched the relevant properties inside the loop over self.series, so we end up with one transaction per series; this should be fine and is IMO much more obviously correct at sight, which I think is valuable.

The point of the DatabaseBlockedPolicy was just to avoid holding a transaction open while doing lengthy computations. Doing a few queries at the start of the loop body for each series shouldn't be a problem; and in any case that loop will only iterate over a single series in production.

[William Grant (wgrant)]

review: Approve (codwe)

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/scripts/generate_extra_overrides.py'
--- lib/lp/archivepublisher/scripts/generate_extra_overrides.py	2012-01-01 02:58:52 +0000
+++ lib/lp/archivepublisher/scripts/generate_extra_overrides.py	2012-01-03 01:35:27 +0000
@@ -17,7 +17,10 @@
 from germinate.archive import TagFile
 from germinate.germinator import Germinator
 from germinate.log import GerminateFormatter
-from germinate.seeds import SeedStructure
+from germinate.seeds import (
+    SeedError,
+    SeedStructure,
+    )
 from zope.component import getUtility
 
 from lp.archivepublisher.config import getPubConfig
@@ -51,16 +54,14 @@
 
 
 def find_operable_series(distribution):
-    """Find a series we can operate on in this distribution.
+    """Find all the series we can operate on in this distribution.
 
     We are allowed to modify DEVELOPMENT or FROZEN series, but should leave
     series with any other status alone.
     """
-    series = distribution.currentseries
-    if series.status in (SeriesStatus.DEVELOPMENT, SeriesStatus.FROZEN):
-        return series
-    else:
-        return None
+    return [
+        series for series in distribution.series
+        if series.status in (SeriesStatus.DEVELOPMENT, SeriesStatus.FROZEN)]
 
 
 class GenerateExtraOverrides(LaunchpadScript):
@@ -96,17 +97,11 @@
                 "Distribution '%s' not found." % self.options.distribution)
 
         self.series = find_operable_series(self.distribution)
-        if self.series is None:
+        if not self.series:
             raise LaunchpadScriptFailure(
-                "There is no DEVELOPMENT distroseries for %s." %
+                "There is no DEVELOPMENT or FROZEN distroseries for %s." %
                 self.options.distribution)
 
-        # Even if DistroSeries.component_names starts including partner, we
-        # don't want it; this applies to the primary archive only.
-        self.components = [component
-                           for component in self.series.component_names
-                           if component != "partner"]
-
     def getConfig(self):
         """Set up a configuration object for this archive."""
         archive = self.distribution.main_archive
@@ -148,11 +143,32 @@
         self.setUpDirs()
         self.addLogHandler()
 
+    def getComponents(self, series):
+        """Get the list of components to process for a given distroseries.
+
+        Even if DistroSeries.component_names starts including partner,
+        we don't want it; this applies to the primary archive only.
+        """
+        return [component
+                for component in series.component_names
+                if component != "partner"]
+
     def makeSeedStructures(self, series_name, flavours, seed_bases=None):
         structures = {}
         for flavour in flavours:
-            structures[flavour] = SeedStructure(
-                "%s.%s" % (flavour, series_name), seed_bases=seed_bases)
+            try:
+                structure = SeedStructure(
+                    "%s.%s" % (flavour, series_name), seed_bases=seed_bases)
+                if len(structure):
+                    structures[flavour] = structure
+                else:
+                    self.logger.warning(
+                        "Skipping empty seed structure for %s.%s",
+                        flavour, series_name)
+            except SeedError, e:
+                self.logger.warning(
+                    "Failed to fetch seeds for %s.%s: %s",
+                    flavour, series_name, e)
         return structures
 
     def logGerminateProgress(self, *args):
@@ -278,14 +294,14 @@
         if "build-essential" in structure.names and primary_flavour:
             write_overrides("build-essential", "Build-Essential", "yes")
 
-    def germinateArch(self, override_file, series_name, arch, flavours,
-                      structures):
+    def germinateArch(self, override_file, series_name, components, arch,
+                      flavours, structures):
         """Germinate seeds on all flavours for a single architecture."""
         germinator = Germinator(arch)
 
         # Read archive metadata.
         archive = TagFile(
-            series_name, self.components, arch,
+            series_name, components, arch,
             "file://%s" % self.config.archiveroot, cleanup=True)
         germinator.parse_archive(archive)
 
@@ -302,34 +318,38 @@
                 override_file, germinator, series_name, arch, flavour,
                 structures[flavour], flavour == flavours[0])
 
-    def generateExtraOverrides(self, series_name, series_architectures,
+    def generateExtraOverrides(self, series_name, components, architectures,
                                flavours, seed_bases=None):
         structures = self.makeSeedStructures(
             series_name, flavours, seed_bases=seed_bases)
 
-        override_path = os.path.join(
-            self.config.miscroot,
-            "more-extra.override.%s.main" % series_name)
-        with AtomicFile(override_path) as override_file:
-            for arch in series_architectures:
-                self.germinateArch(
-                    override_file, series_name, arch, flavours, structures)
+        if structures:
+            override_path = os.path.join(
+                self.config.miscroot,
+                "more-extra.override.%s.main" % series_name)
+            with AtomicFile(override_path) as override_file:
+                for arch in architectures:
+                    self.germinateArch(
+                        override_file, series_name, components, arch,
+                        flavours, structures)
 
     def process(self, seed_bases=None):
         """Do the bulk of the work."""
         self.setUp()
 
-        series_name = self.series.name
-        series_architectures = sorted(
-            [arch.architecturetag for arch in self.series.architectures])
+        for series in self.series:
+            series_name = series.name
+            components = self.getComponents(series)
+            architectures = sorted(
+                [arch.architecturetag for arch in series.architectures])
 
-        # This takes a while.  Ensure that we do it without keeping a
-        # database transaction open.
-        self.txn.commit()
-        with DatabaseBlockedPolicy():
-            self.generateExtraOverrides(
-                series_name, series_architectures, self.args,
-                seed_bases=seed_bases)
+            # This takes a while.  Ensure that we do it without keeping a
+            # database transaction open.
+            self.txn.commit()
+            with DatabaseBlockedPolicy():
+                self.generateExtraOverrides(
+                    series_name, components, architectures, self.args,
+                    seed_bases=seed_bases)
 
     def main(self):
         """See `LaunchpadScript`."""

=== modified file 'lib/lp/archivepublisher/tests/test_generate_extra_overrides.py'
--- lib/lp/archivepublisher/tests/test_generate_extra_overrides.py	2012-01-01 02:58:52 +0000
+++ lib/lp/archivepublisher/tests/test_generate_extra_overrides.py	2012-01-03 01:35:27 +0000
@@ -260,7 +260,8 @@
         development_distroseries = self.factory.makeDistroSeries(
             distro, status=SeriesStatus.DEVELOPMENT)
         script = self.makeScript(distro)
-        self.assertEqual(development_distroseries, script.series)
+        observed_series = [series.name for series in script.series]
+        self.assertEqual([development_distroseries.name], observed_series)
 
     def test_permits_frozen_distro_series(self):
         # If there is no DEVELOPMENT series, a FROZEN one will do.
@@ -270,7 +271,8 @@
         frozen_distroseries = self.factory.makeDistroSeries(
             distro, status=SeriesStatus.FROZEN)
         script = self.makeScript(distro)
-        self.assertEqual(frozen_distroseries, script.series)
+        observed_series = [series.name for series in script.series]
+        self.assertEqual([frozen_distroseries.name], observed_series)
 
     def test_requires_development_frozen_distro_series(self):
         # If there is no DEVELOPMENT or FROZEN series, the script fails.
@@ -280,6 +282,28 @@
         script = self.makeScript(distro, run_setup=False)
         self.assertRaises(LaunchpadScriptFailure, script.processOptions)
 
+    def test_multiple_development_frozen_distro_series(self):
+        # If there are multiple DEVELOPMENT or FROZEN series, they are all
+        # used.
+        distro = self.makeDistro()
+        development_distroseries_one = self.factory.makeDistroSeries(
+            distro, status=SeriesStatus.DEVELOPMENT)
+        development_distroseries_two = self.factory.makeDistroSeries(
+            distro, status=SeriesStatus.DEVELOPMENT)
+        frozen_distroseries_one = self.factory.makeDistroSeries(
+            distro, status=SeriesStatus.FROZEN)
+        frozen_distroseries_two = self.factory.makeDistroSeries(
+            distro, status=SeriesStatus.FROZEN)
+        script = self.makeScript(distro)
+        expected_series = [
+            development_distroseries_one.name,
+            development_distroseries_two.name,
+            frozen_distroseries_one.name,
+            frozen_distroseries_two.name,
+            ]
+        observed_series = [series.name for series in script.series]
+        self.assertContentEqual(expected_series, observed_series)
+
     def test_components_exclude_partner(self):
         # If a 'partner' component exists, it is excluded.
         distro = self.makeDistro()
@@ -289,7 +313,8 @@
         self.factory.makeComponentSelection(
             distroseries=distroseries, component="partner")
         script = self.makeScript(distro)
-        self.assertEqual(["main"], script.components)
+        self.assertEqual(1, len(script.series))
+        self.assertEqual(["main"], script.getComponents(script.series[0]))
 
     def test_compose_output_path_in_germinateroot(self):
         # Output files are written to the correct locations under
@@ -308,15 +333,58 @@
                 arch),
             output)
 
-    def fetchGerminatedOverrides(self, script, series_name, arch, flavours):
+    def test_make_seed_structures_missing_seeds(self):
+        # makeSeedStructures ignores missing seeds.
+        distro = self.makeDistro()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        series_name = distroseries.name
+        script = self.makeScript(distro)
+        flavour = self.factory.getUniqueString()
+
+        structures = script.makeSeedStructures(
+            series_name, [flavour], seed_bases=["file://%s" % self.seeddir])
+        self.assertEqual({}, structures)
+
+    def test_make_seed_structures_empty_seed_structure(self):
+        # makeSeedStructures ignores an empty seed structure.
+        distro = self.makeDistro()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        series_name = distroseries.name
+        script = self.makeScript(distro)
+        flavour = self.factory.getUniqueString()
+        self.makeSeedStructure(flavour, series_name, [])
+
+        structures = script.makeSeedStructures(
+            series_name, [flavour], seed_bases=["file://%s" % self.seeddir])
+        self.assertEqual({}, structures)
+
+    def test_make_seed_structures_valid_seeds(self):
+        # makeSeedStructures reads valid seeds successfully.
+        distro = self.makeDistro()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        series_name = distroseries.name
+        script = self.makeScript(distro)
+        flavour = self.factory.getUniqueString()
+        seed = self.factory.getUniqueString()
+        self.makeSeedStructure(flavour, series_name, [seed])
+        self.makeSeed(flavour, series_name, seed, [])
+
+        structures = script.makeSeedStructures(
+            series_name, [flavour], seed_bases=["file://%s" % self.seeddir])
+        self.assertIn(flavour, structures)
+
+    def fetchGerminatedOverrides(self, script, distroseries, arch, flavours):
         """Helper to call script.germinateArch and return overrides."""
         structures = script.makeSeedStructures(
-            series_name, flavours, seed_bases=["file://%s" % self.seeddir])
+            distroseries.name, flavours,
+            seed_bases=["file://%s" % self.seeddir])
 
         override_fd, override_path = tempfile.mkstemp()
         with os.fdopen(override_fd, "w") as override_file:
             script.germinateArch(
-                override_file, series_name, arch, flavours, structures)
+                override_file, distroseries.name,
+                script.getComponents(distroseries), arch, flavours,
+                structures)
         return file_contents(override_path).splitlines()
 
     def test_germinate_output(self):
@@ -344,7 +412,7 @@
         self.makeSeed(flavour_two, series_name, seed, [two.name])
 
         overrides = self.fetchGerminatedOverrides(
-            script, series_name, arch, [flavour_one, flavour_two])
+            script, distroseries, arch, [flavour_one, flavour_two])
         self.assertEqual([], overrides)
 
         seed_dir_one = os.path.join(
@@ -402,7 +470,7 @@
             headers=["Task-Description: two"])
 
         overrides = self.fetchGerminatedOverrides(
-            script, series_name, arch, [flavour])
+            script, distroseries, arch, [flavour])
         expected_overrides = [
             "%s/%s  Task  %s" % (one.name, arch, seed_one),
             "%s/%s  Task  %s" % (two.name, arch, seed_one),
@@ -510,10 +578,37 @@
         self.makeSeed(flavour, series_name, seed, [package.name])
 
         overrides = self.fetchGerminatedOverrides(
-            script, series_name, arch, [flavour])
+            script, distroseries, arch, [flavour])
         self.assertContentEqual(
             ["%s/%s  Build-Essential  yes" % (package.name, arch)], overrides)
 
+    def test_process_missing_seeds(self):
+        # The script ignores series with no seed structures.
+        distro = self.makeDistro()
+        distroseries_one = self.factory.makeDistroSeries(distribution=distro)
+        distroseries_two = self.factory.makeDistroSeries(distribution=distro)
+        component = self.factory.makeComponent()
+        self.factory.makeComponentSelection(
+            distroseries=distroseries_one, component=component)
+        self.factory.makeComponentSelection(
+            distroseries=distroseries_two, component=component)
+        self.factory.makeDistroArchSeries(distroseries=distroseries_one)
+        self.factory.makeDistroArchSeries(distroseries=distroseries_two)
+        flavour = self.factory.getUniqueString()
+        script = self.makeScript(distro, extra_args=[flavour])
+        self.makeIndexFiles(script, distroseries_two)
+        seed = self.factory.getUniqueString()
+        self.makeSeedStructure(flavour, distroseries_two.name, [seed])
+        self.makeSeed(flavour, distroseries_two.name, seed, [])
+
+        script.process(seed_bases=["file://%s" % self.seeddir])
+        self.assertFalse(os.path.exists(os.path.join(
+            script.config.miscroot,
+            "more-extra.override.%s.main" % distroseries_one.name)))
+        self.assertTrue(os.path.exists(os.path.join(
+            script.config.miscroot,
+            "more-extra.override.%s.main" % distroseries_two.name)))
+
     def test_main(self):
         # If run end-to-end, the script generates override files containing
         # output for all architectures, and sends germinate's log output to