launchpad-reviewers team mailing list archive

[Colin Watson <mp+430821@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-librarian-swift-multiple-instances into launchpad:master.

Commit message:
Fix librarian-feed-swift's multiple-instance mode

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

The multiple-instance mode that I added in https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/404970 turned out not to work quite right when we actually tried to use it on production, because the instances raced with each other to list directories and remove files from those directories in a way that could result in exceptions while checking for recently-modified files.

Based on a change already applied to production by William Grant; I added a corresponding test.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-librarian-swift-multiple-instances into launchpad:master.

diff --git a/lib/lp/services/librarianserver/swift.py b/lib/lp/services/librarianserver/swift.py
index 53e85ba..4fb79e6 100644
--- a/lib/lp/services/librarianserver/swift.py
+++ b/lib/lp/services/librarianserver/swift.py
@@ -131,12 +131,6 @@ def to_swift(
             if fs_path > end_fs_path:
                 break
 
-            # Skip files which have been modified recently, as they
-            # may be uploads still in progress.
-            if os.path.getmtime(fs_path) > time.time() - ONE_DAY:
-                log.debug("Skipping recent upload %s" % fs_path)
-                continue
-
             # Reverse engineer the LibraryFileContent.id from the
             # file's path. Warn about and skip bad filenames.
             rel_fs_path = fs_path[len(fs_root) + 1 :]
@@ -155,6 +149,12 @@ def to_swift(
                 if (lfc % num_instances) != instance_id:
                     continue
 
+            # Skip files which have been modified recently, as they
+            # may be uploads still in progress.
+            if os.path.getmtime(fs_path) > time.time() - ONE_DAY:
+                log.debug("Skipping recent upload %s" % fs_path)
+                continue
+
             log.debug("Found {} ({})".format(lfc, filename))
 
             if (
diff --git a/lib/lp/services/librarianserver/tests/test_swift.py b/lib/lp/services/librarianserver/tests/test_swift.py
index 64f8546..9e095dd 100644
--- a/lib/lp/services/librarianserver/tests/test_swift.py
+++ b/lib/lp/services/librarianserver/tests/test_swift.py
@@ -380,6 +380,60 @@ class TestFeedSwift(TestCase):
             finally:
                 swift_client.close()
 
+    def test_multiple_feed_instances_race(self):
+        # The feed process copes with one instance removing its files in
+        # parallel with another instance running.  Simulate this with a
+        # remove_func that removes other instances' files as well.
+        def remove_this_and_others(path):
+            os.unlink(path)
+            for lfc in self.lfcs:
+                if (lfc.id % 3) != 0:
+                    other_path = swift.filesystem_path(lfc.id)
+                    try:
+                        os.unlink(other_path)
+                    except FileNotFoundError:
+                        pass
+
+        log = BufferLogger()
+
+        # Confirm that files exist on disk where we expect to find them.
+        for lfc in self.lfcs:
+            path = swift.filesystem_path(lfc.id)
+            self.assertTrue(os.path.exists(path))
+
+        # Migrate all the files for the first instance into Swift,
+        # pretending that other instances are running in parallel.
+        swift.to_swift(
+            log,
+            instance_id=0,
+            num_instances=3,
+            remove_func=remove_this_and_others,
+        )
+
+        # All the matching files are gone from disk (some removed by the
+        # first instance, others by our remove_func).
+        for lfc in self.lfcs:
+            self.assertFalse(os.path.exists(swift.filesystem_path(lfc.id)))
+
+        # All the files migrated by this instance are in Swift.
+        swift_client = self.swift_fixture.connect()
+        try:
+            for lfc, contents in zip(self.lfcs, self.contents):
+                container, name = swift.swift_location(lfc.id)
+                if (lfc.id % 3) == 0:
+                    headers, obj = swift_client.get_object(container, name)
+                    self.assertEqual(contents, obj, "Did not round trip")
+                else:
+                    self.assertRaises(
+                        swiftclient.ClientException,
+                        swift.quiet_swiftclient,
+                        swift_client.get_object,
+                        container,
+                        name,
+                    )
+        finally:
+            swift_client.close()
+
     def test_swift_timeout(self):
         # The librarian's Swift connections honour the configured timeout.
         self.pushConfig(