Merge lp:~cjwatson/launchpad/contents-race into lp:launchpad

[Colin Watson]

Update Contents files in a way that doesn't suffer from races between generate-contents-files and publish-ftpmaster.

Update Contents files in a way that doesn't suffer from races between generate-contents-files and publish-ftpmaster.

[William Grant (wgrant)]

getSupportedSeries is probably not an ideal name, given that SeriesStatus.SUPPORTED exists.

Also, we might want to make the method itself less nonsensical. Experimental is an active series status, so it probably wants Contents files.

review: Approve (code)

=== modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
--- lib/lp/archivepublisher/scripts/generate_contents_files.py	2012-11-10 02:25:07 +0000
+++ lib/lp/archivepublisher/scripts/generate_contents_files.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Archive Contents files generator."""
@@ -16,7 +16,6 @@
 from lp.archivepublisher.config import getPubConfig
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.interfaces.series import SeriesStatus
 from lp.services.command_spawner import (
     CommandSpawner,
     OutputLineHandler,
@@ -132,20 +131,9 @@
             if not file_exists(path):
                 os.makedirs(path)
 
-    def getSupportedSeries(self):
-        """Return suites that are supported in this distribution.
-
-        "Supported" means not EXPERIMENTAL or OBSOLETE.
-        """
-        unsupported_status = (SeriesStatus.EXPERIMENTAL,
-                              SeriesStatus.OBSOLETE)
-        for series in self.distribution:
-            if series.status not in unsupported_status:
-                yield series
-
     def getSuites(self):
-        """Return suites that are actually supported in this distribution."""
-        for series in self.getSupportedSeries():
+        """Return suites that need Contents files."""
+        for series in self.distribution.getNonObsoleteSeries():
             for pocket in PackagePublishingPocket.items:
                 suite = series.getSuite(pocket)
                 if file_exists(os.path.join(self.config.distsroot, suite)):
@@ -269,12 +257,12 @@
         # re-fetch them unnecessarily.
         if differ_in_content(current_contents, last_contents):
             self.logger.debug(
-                "Installing new Contents file for %s/%s.", suite, arch)
+                "Staging new Contents file for %s/%s.", suite, arch)
 
             new_contents = os.path.join(
                 contents_dir, "%s.gz" % contents_filename)
             contents_dest = os.path.join(
-                self.config.distsroot, suite, "%s.gz" % contents_filename)
+                contents_dir, "%s-staged.gz" % contents_filename)
 
             os.rename(current_contents, last_contents)
             os.rename(new_contents, contents_dest)

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2014-08-09 19:45:00 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Master distro publishing script."""
@@ -10,6 +10,7 @@
 
 from datetime import datetime
 import os
+import shutil
 
 from pytz import utc
 from zope.component import getUtility
@@ -156,6 +157,19 @@
         for purpose, config in candidates if config is not None)
 
 
+def newer_mtime(one_file, other_file):
+    """Is one_file newer than other_file, or is other_file missing?"""
+    try:
+        one_mtime = os.stat(one_file).st_mtime
+    except OSError:
+        return False
+    try:
+        other_mtime = os.stat(other_file).st_mtime
+    except OSError:
+        return True
+    return one_mtime > other_mtime
+
+
 class PublishFTPMaster(LaunchpadCronScript):
     """Publish a distro (update).
 
@@ -539,6 +553,32 @@
                 # most of its time.
                 self.publishDistroArchive(distribution, archive)
 
+    def updateContentsFile(self, distribution, suite, arch):
+        """Update a single Contents file if necessary."""
+        config = self.configs[distribution][ArchivePurpose.PRIMARY]
+        backup_dists = get_backup_dists(config)
+        content_dists = os.path.join(
+            config.distroroot, "contents-generation", distribution.name,
+            "dists")
+        contents_filename = "Contents-%s" % arch.architecturetag
+        current_contents = os.path.join(
+            backup_dists, suite, "%s.gz" % contents_filename)
+        new_contents = os.path.join(
+            content_dists, suite, "%s-staged.gz" % contents_filename)
+        if newer_mtime(new_contents, current_contents):
+            self.logger.debug(
+                "Installing new Contents file for %s/%s.", suite,
+                arch.architecturetag)
+            shutil.copy2(new_contents, current_contents)
+
+    def updateContentsFiles(self, distribution):
+        """Pick up updated Contents files if necessary."""
+        for series in distribution.getNonObsoleteSeries():
+            for pocket in PackagePublishingPocket.items:
+                suite = series.getSuite(pocket)
+                for arch in series.enabled_architectures:
+                    self.updateContentsFile(distribution, suite, arch)
+
     def publish(self, distribution, security_only=False):
         """Do the main publishing work.
 
@@ -558,6 +598,8 @@
                 # Let's assume the main archive is always modified
                 has_published = True
 
+            self.updateContentsFiles(distribution)
+
             # Swizzle the now-updated backup dists and the current dists
             # around.
             self.installDists(distribution)

=== modified file 'lib/lp/archivepublisher/tests/test_generate_contents_files.py'
--- lib/lp/archivepublisher/tests/test_generate_contents_files.py	2013-09-11 08:17:34 +0000
+++ lib/lp/archivepublisher/tests/test_generate_contents_files.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test for the `generate-contents-files` script."""
@@ -15,6 +15,7 @@
     execute,
     GenerateContentsFiles,
     )
+from lp.archivepublisher.scripts.publish_ftpmaster import PublishFTPMaster
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.log.logger import DevNullLogger
 from lp.services.osutils import write_file
@@ -183,14 +184,6 @@
         self.assertEqual(
             [das.architecturetag], script.getArchs(distroseries.name))
 
-    def test_getSupportedSeries(self):
-        # getSupportedSeries returns the supported distroseries in the
-        # distribution.
-        script = self.makeScript()
-        distroseries = self.factory.makeDistroSeries(
-            distribution=script.distribution)
-        self.assertIn(distroseries, script.getSupportedSeries())
-
     def test_getSuites(self):
         # getSuites returns the full names (distroseries-pocket) of the
         # pockets that have packages to publish.
@@ -268,7 +261,8 @@
             script.content_archive, StartsWith(script.config.distroroot))
 
     def test_main(self):
-        # If run end-to-end, the script generates Contents.gz files.
+        # If run end-to-end, the script generates Contents.gz files, and a
+        # following publisher run will put those files in their final place.
         distro = self.makeDistro()
         distroseries = self.factory.makeDistroSeries(distribution=distro)
         processor = self.factory.makeProcessor()
@@ -287,6 +281,13 @@
         fake_overrides(script, distroseries)
         script.process()
         self.assertTrue(file_exists(os.path.join(
+            script.content_archive, distro.name, "dists", suite,
+            "Contents-%s-staged.gz" % das.architecturetag)))
+        publisher_script = PublishFTPMaster(test_args=["-d", distro.name])
+        publisher_script.txn = self.layer.txn
+        publisher_script.logger = DevNullLogger()
+        publisher_script.main()
+        self.assertTrue(file_exists(os.path.join(
             script.config.distsroot, suite,
             "Contents-%s.gz" % das.architecturetag)))
 

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2014-04-15 16:32:03 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test publish-ftpmaster cron script."""
@@ -8,6 +8,7 @@
 import logging
 import os
 from textwrap import dedent
+import time
 
 from apt_pkg import TagFile
 from testtools.matchers import (
@@ -24,6 +25,7 @@
     compose_shell_boolean,
     find_run_parts_dir,
     get_working_dists,
+    newer_mtime,
     PublishFTPMaster,
     shell_quote,
     )
@@ -38,6 +40,7 @@
     BufferLogger,
     DevNullLogger,
     )
+from lp.services.osutils import write_file
 from lp.services.scripts.base import LaunchpadScriptFailure
 from lp.services.utils import file_exists
 from lp.soyuz.enums import (
@@ -208,6 +211,46 @@
         self.assertEqual("no", compose_shell_boolean(False))
 
 
+class TestNewerMtime(TestCase):
+
+    def setUp(self):
+        super(TestCase, self).setUp()
+        tempdir = self.useTempDir()
+        self.a = os.path.join(tempdir, "a")
+        self.b = os.path.join(tempdir, "b")
+
+    def test_both_missing(self):
+        self.assertFalse(newer_mtime(self.a, self.b))
+
+    def test_one_missing(self):
+        write_file(self.b, "")
+        self.assertFalse(newer_mtime(self.a, self.b))
+
+    def test_other_missing(self):
+        write_file(self.a, "")
+        self.assertTrue(newer_mtime(self.a, self.b))
+
+    def test_older(self):
+        write_file(self.a, "")
+        os.utime(self.a, (0, 0))
+        write_file(self.b, "")
+        self.assertFalse(newer_mtime(self.a, self.b))
+
+    def test_equal(self):
+        now = time.time()
+        write_file(self.a, "")
+        os.utime(self.a, (now, now))
+        write_file(self.b, "")
+        os.utime(self.b, (now, now))
+        self.assertFalse(newer_mtime(self.a, self.b))
+
+    def test_newer(self):
+        write_file(self.a, "")
+        write_file(self.b, "")
+        os.utime(self.b, (0, 0))
+        self.assertTrue(newer_mtime(self.a, self.b))
+
+
 class TestFindRunPartsDir(TestCaseWithFactory, HelpersMixin):
     layer = ZopelessDatabaseLayer
 
@@ -796,6 +839,34 @@
                 "Did not find expected marker for %s."
                 % archive.purpose.title)
 
+    def test_updateContentsFile_installs_changed(self):
+        distro = self.makeDistroWithPublishDirectory()
+        distroseries = self.factory.makeDistroSeries(distribution=distro)
+        das = self.factory.makeDistroArchSeries(distroseries=distroseries)
+        script = self.makeScript(distro)
+        script.setUp()
+        script.setUpDirs()
+        archive_config = getPubConfig(distro.main_archive)
+        contents_filename = "Contents-%s" % das.architecturetag
+        backup_suite = os.path.join(
+            archive_config.archiveroot + "-distscopy", "dists",
+            distroseries.name)
+        os.makedirs(backup_suite)
+        write_marker_file(
+            [backup_suite, "%s.gz" % contents_filename], "Old Contents")
+        os.utime(
+            os.path.join(backup_suite, "%s.gz" % contents_filename), (0, 0))
+        content_suite = os.path.join(
+            archive_config.distroroot, "contents-generation", distro.name,
+            "dists", distroseries.name)
+        os.makedirs(content_suite)
+        write_marker_file(
+            [content_suite, "%s-staged.gz" % contents_filename], "Contents")
+        script.updateContentsFile(distro, distroseries.name, das)
+        self.assertEqual(
+            "Contents",
+            read_marker_file([backup_suite, "%s.gz" % contents_filename]))
+
     def test_publish_always_returns_true_for_primary(self):
         script = self.makeScript()
         script.publishDistroUploads = FakeMethod()

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2014-11-17 18:36:16 +0000
+++ lib/lp/registry/interfaces/distribution.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interfaces including and related to IDistribution."""
@@ -408,6 +408,9 @@
     def getDevelopmentSeries():
         """Return the DistroSeries which are marked as in development."""
 
+    def getNonObsoleteSeries():
+        """Return the non-OBSOLETE DistroSeries in this distribution."""
+
     def resolveSeriesAlias(name):
         """Resolve a series alias.
 

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2014-12-08 04:20:17 +0000
+++ lib/lp/registry/model/distribution.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database classes for implementing distribution items."""
@@ -785,6 +785,12 @@
             distribution=self,
             status=SeriesStatus.DEVELOPMENT)
 
+    def getNonObsoleteSeries(self):
+        """See `IDistribution`."""
+        for series in self.series:
+            if series.status != SeriesStatus.OBSOLETE:
+                yield series
+
     def getMilestone(self, name):
         """See `IDistribution`."""
         return Milestone.selectOne("""

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2013-08-01 14:09:45 +0000
+++ lib/lp/registry/tests/test_distribution.py	2015-02-13 10:45:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Distribution."""
@@ -383,7 +383,7 @@
 
 
 class SeriesTests(TestCaseWithFactory):
-    """Test IDistribution.getSeries().
+    """Test IDistribution.getSeries() and friends.
     """
 
     layer = LaunchpadFunctionalLayer
@@ -416,6 +416,20 @@
         self.assertEqual(
             series, distro.getSeries("devel", follow_aliases=True))
 
+    def test_getNonObsoleteSeries(self):
+        distro = self.factory.makeDistribution()
+        self.factory.makeDistroSeries(
+            distribution=distro, status=SeriesStatus.OBSOLETE)
+        current = self.factory.makeDistroSeries(
+            distribution=distro, status=SeriesStatus.CURRENT)
+        development = self.factory.makeDistroSeries(
+            distribution=distro, status=SeriesStatus.DEVELOPMENT)
+        experimental = self.factory.makeDistroSeries(
+            distribution=distro, status=SeriesStatus.EXPERIMENTAL)
+        self.assertContentEqual(
+            [current, development, experimental],
+            list(distro.getNonObsoleteSeries()))
+
 
 class DerivativesTests(TestCaseWithFactory):
     """Test IDistribution.derivatives.