Merge lp:~cjwatson/launchpad/dep11-mtime into lp:launchpad

[Colin Watson]

Fine-tune publisher timestamp syncing to avoid interfering with files that are fetched from other sources rather than generated internally.

Fine-tune publisher timestamp syncing to avoid interfering with files that are fetched from other sources rather than generated internally.

[William Grant (wgrant)]

This will fix the out of dateness problem, but will it not reintroduce the timestamp-based cache header desync thing for non-core files?

review: Approve (code)

[Colin Watson (cjwatson)]

The relevant cache headers are only set for a very limited set of files.
E.g. in ubuntu-mirror:

    <Files ~ "Release(\.gpg)?|InRelease|Packages\.(xz|bz2|gz)|Sources\.(xz|bz2|gz)?|Translation-.*?$">
        ExpiresActive On
        ExpiresDefault "modification plus 1800 seconds"
        Header append Cache-Control "proxy-revalidate"
    </Files>

In general this seems to equate to the core set of files produced by
apt-ftparchive and other necessary publisher functions, rather than the
ones I'm calling "extra" files that are synced in from elsewhere; the
latter would never have had special Expires applied, so all we were
achieving was to bump its Last-Modified, which probably didn't achieve
much except causing unnecessary redownloads. curl against the current
archive confirms:

  $ curl -Is http://archive.ubuntu.com/ubuntu/dists/xenial/InRelease | egrep '^(Cache-Control|Expires):'
  Cache-Control: max-age=1264, proxy-revalidate
  Expires: Tue, 15 Mar 2016 02:48:00 GMT
  $ curl -Is http://archive.ubuntu.com/ubuntu/dists/xenial/main/dep11/Components-amd64.yml.gz | egrep '^(Cache-Control|Expires):'
  $

Of course this is all fairly weird and ought to be replaced by by-hash,
but in the interim I don't think this change breaks anything to do with
cache headers.

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2016-02-12 01:29:49 +0000
+++ lib/lp/archivepublisher/publishing.py	2016-03-11 12:14:23 +0000
@@ -838,15 +838,24 @@
             self.archive.getComponentsForSeries(distroseries)]
         all_architectures = [
             a.architecturetag for a in distroseries.enabled_architectures]
-        all_files = set()
+        # Core files are those that are normally updated when a suite
+        # changes, and which therefore receive special treatment with
+        # caching headers on mirrors.
+        core_files = set()
+        # Extra files are updated occasionally from other sources.  They are
+        # still checksummed and indexed, but they do not receive special
+        # treatment with caching headers on mirrors.  We must not play any
+        # special games with timestamps here, as it will interfere with the
+        # "staging" mechanism used to update these files.
+        extra_files = set()
         for component in all_components:
             self._writeSuiteSource(
-                distroseries, pocket, component, all_files)
+                distroseries, pocket, component, core_files)
             for architecture in all_architectures:
                 self._writeSuiteArch(
-                    distroseries, pocket, component, architecture, all_files)
+                    distroseries, pocket, component, architecture, core_files)
             self._writeSuiteI18n(
-                distroseries, pocket, component, all_files)
+                distroseries, pocket, component, core_files)
             dep11_dir = os.path.join(
                 self._config.distsroot, suite, component, "dep11")
             try:
@@ -855,15 +864,16 @@
                             dep11_file.startswith("icons-")):
                         dep11_path = os.path.join(
                             component, "dep11", dep11_file)
-                        all_files.add(remove_suffix(dep11_path))
-                        all_files.add(dep11_path)
+                        extra_files.add(remove_suffix(dep11_path))
+                        extra_files.add(dep11_path)
             except OSError as e:
                 if e.errno != errno.ENOENT:
                     raise
         for architecture in all_architectures:
             for contents_path in get_suffixed_indices(
                     'Contents-' + architecture):
-                all_files.add(contents_path)
+                extra_files.add(contents_path)
+        all_files = core_files | extra_files
 
         drsummary = "%s %s " % (self.distro.displayname,
                                 distroseries.displayname)
@@ -898,20 +908,20 @@
             release_file.setdefault("SHA256", []).append(hashes["sha256"])
 
         self._writeReleaseFile(suite, release_file)
-        all_files.add("Release")
+        core_files.add("Release")
 
         if self.archive.signing_key is not None:
             # Sign the repository.
             IArchiveSigningKey(self.archive).signRepository(suite)
-            all_files.add("Release.gpg")
-            all_files.add("InRelease")
+            core_files.add("Release.gpg")
+            core_files.add("InRelease")
         else:
             # Skip signature if the archive signing key is undefined.
             self.log.debug("No signing key available, skipping signature.")
 
         # Make sure all the timestamps match, to make it easier to insert
         # caching headers on mirrors.
-        self._syncTimestamps(suite, all_files)
+        self._syncTimestamps(suite, core_files)
 
     def _writeSuiteArchOrSource(self, distroseries, pocket, component,
                                 file_stub, arch_name, arch_path,

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2016-02-12 01:29:49 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2016-03-11 12:14:23 +0000
@@ -22,7 +22,10 @@
     import lzma
 except ImportError:
     from backports import lzma
-from testtools.matchers import ContainsAll
+from testtools.matchers import (
+    ContainsAll,
+    LessThan,
+    )
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -1863,7 +1866,7 @@
                     release, os.path.join('main', 'dep11', name), f.read())
 
     def testReleaseFileTimestamps(self):
-        # The timestamps of Release and all its entries match.
+        # The timestamps of Release and all its core entries match.
         publisher = Publisher(
             self.logger, self.config, self.disk_pool,
             self.ubuntutest.main_archive)
@@ -1878,16 +1881,33 @@
         sources = suite_path('main', 'source', 'Sources.gz')
         sources_timestamp = os.stat(sources).st_mtime - 60
         os.utime(sources, (sources_timestamp, sources_timestamp))
+        dep11_path = suite_path('main', 'dep11')
+        dep11_names = ('Components-amd64.yml.gz', 'Components-i386.yml.gz',
+                       'icons-64x64.tar.gz', 'icons-128x128.tar.gz')
+        os.makedirs(dep11_path)
+        now = time.time()
+        for name in dep11_names:
+            with gzip.GzipFile(os.path.join(dep11_path, name), 'wb') as f:
+                f.write(name)
+            os.utime(os.path.join(dep11_path, name), (now - 60, now - 60))
 
         publisher.D_writeReleaseFiles(False)
 
         release = self.parseRelease(suite_path('Release'))
         paths = ['Release'] + [entry['name'] for entry in release['md5sum']]
         timestamps = set(
-            os.stat(suite_path(path)).st_mtime
-            for path in paths if os.path.exists(suite_path(path)))
+            os.stat(suite_path(path)).st_mtime for path in paths
+            if '/dep11/' not in path and os.path.exists(suite_path(path)))
         self.assertEqual(1, len(timestamps))
 
+        # Non-core files preserve their original timestamps.
+        # (Due to https://bugs.python.org/issue12904, there is some loss of
+        # accuracy in the test.)
+        for name in dep11_names:
+            self.assertThat(
+                os.stat(os.path.join(dep11_path, name)).st_mtime,
+                LessThan(now - 59))
+
     def testCreateSeriesAliasesNoAlias(self):
         """createSeriesAliases has nothing to do by default."""
         publisher = Publisher(