launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:ftparchive-avoid-empty-overrides into launchpad:master.

Commit message:
Don't overwrite existing files in createEmptyPocketRequests

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1871920 in Launchpad itself: "Contents files for releases are sometimes empty"
  https://bugs.launchpad.net/launchpad/+bug/1871920

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/399940

The job of `FTPArchive.createEmptyPocketRequests` is to make sure that override and file list files for suites/components that we're publishing exist even if they're going to end up empty.  It did this by making them empty, which was OK when nothing was reading from those files in parallel.  However, that hasn't been true for some time due to Contents file generation, and the current implementation means that it's possible for Contents file generation to see temporarily-empty versions of these files and thus generate empty Contents files.

To fix this, only create these files if they don't already exist.  This should complete the fix in https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/374930.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:ftparchive-avoid-empty-overrides into launchpad:master.

diff --git a/lib/lp/archivepublisher/model/ftparchive.py b/lib/lp/archivepublisher/model/ftparchive.py
index 3203771..a13269e 100644
--- a/lib/lp/archivepublisher/model/ftparchive.py
+++ b/lib/lp/archivepublisher/model/ftparchive.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from collections import defaultdict
@@ -242,7 +242,7 @@ class FTPArchiveHandler:
         """Creates empty files for a release pocket and distroseries"""
         suite = distroseries.getSuite(pocket)
 
-        # Create empty override lists.
+        # Create empty override lists if they don't already exist.
         needed_paths = [
             (comp,),
             ("extra", comp),
@@ -252,15 +252,19 @@ class FTPArchiveHandler:
             needed_paths.append((comp, sub_comp))
 
         for path in needed_paths:
-            write_file(os.path.join(
+            full_path = os.path.join(
                 self._config.overrideroot,
-                ".".join(("override", suite) + path)), b"")
+                ".".join(("override", suite) + path))
+            if not os.path.exists(full_path):
+                write_file(full_path, b"")
 
-        # Create empty file lists.
+        # Create empty file lists if they don't already exist.
         def touch_list(*parts):
-            write_file(os.path.join(
+            full_path = os.path.join(
                 self._config.overrideroot,
-                "_".join((suite, ) + parts)), b"")
+                "_".join((suite, ) + parts))
+            if not os.path.exists(full_path):
+                write_file(full_path, b"")
         touch_list(comp, "source")
 
         arch_tags = [
diff --git a/lib/lp/archivepublisher/tests/test_ftparchive.py b/lib/lp/archivepublisher/tests/test_ftparchive.py
index 7209f53..3a9665a 100755
--- a/lib/lp/archivepublisher/tests/test_ftparchive.py
+++ b/lib/lp/archivepublisher/tests/test_ftparchive.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for ftparchive.py"""
@@ -22,6 +22,7 @@ from debian.deb822 import (
     )
 from testtools.matchers import (
     Equals,
+    FileContains,
     LessThan,
     MatchesListwise,
     )
@@ -42,6 +43,7 @@ from lp.services.log.logger import (
     BufferLogger,
     DevNullLogger,
     )
+from lp.services.osutils import write_file
 from lp.soyuz.enums import (
     BinaryPackageFormat,
     IndexCompressionType,
@@ -192,6 +194,36 @@ class TestFTPArchive(TestCaseWithFactory):
             [('tiny', 'tiny_0.1_i386.deb', component, 'binary-i386')])
         fa.publishFileLists('hoary-test', source_files, binary_files)
 
+    def test_createEmptyPocketRequests_preserves_existing(self):
+        # createEmptyPocketRequests leaves existing override and file list
+        # files alone, in order to avoid race conditions with other
+        # processes (e.g. generate-contents-files) reading those files in
+        # parallel.
+        publisher = Publisher(
+            self._logger, self._config, self._dp, self._archive)
+        fa = FTPArchiveHandler(
+            self._logger, self._config, self._dp, self._distribution,
+            publisher)
+        lists = (
+            'hoary-test-updates_main_source',
+            'hoary-test-updates_main_binary-i386',
+            'hoary-test-updates_main_debian-installer_binary-i386',
+            'override.hoary-test-updates.main',
+            'override.hoary-test-updates.extra.main',
+            'override.hoary-test-updates.main.src',
+            )
+        for listname in lists:
+            write_file(
+                os.path.join(self._config.overrideroot, listname),
+                b'previous contents\n')
+
+        fa.createEmptyPocketRequests(fullpublish=True)
+
+        for listname in lists:
+            self.assertThat(
+                os.path.join(self._config.overrideroot, listname),
+                FileContains('previous contents\n'))
+
     def test_getSourcesForOverrides(self):
         # getSourcesForOverrides returns a list of tuples containing:
         # (sourcename, component, section)