launchpad-reviewers team mailing list archive

[Colin Watson <mp+414421@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:librarian-gc-log-delete-failures into launchpad:master.

Commit message:
Make librarian GC log delete failures rather than crashing

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

librarian-gc has been crashing on every run since at least the start of 2022.  A debug run showed that most of the deletions it was attempting from Swift were succeeding, but occasionally one timed out.  Since future runs will retry the deletion, it makes sense to log such events rather than crashing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:librarian-gc-log-delete-failures into launchpad:master.

diff --git a/lib/lp/services/librarianserver/librariangc.py b/lib/lp/services/librarianserver/librariangc.py
index aa3429f..26b0462 100644
--- a/lib/lp/services/librarianserver/librariangc.py
+++ b/lib/lp/services/librarianserver/librariangc.py
@@ -553,7 +553,9 @@ class UnreferencedContentPruner:
                             'Swift {}'.format(connection_pool.os_auth_url))
                     except swiftclient.ClientException as x:
                         if x.http_status != 404:
-                            raise
+                            log.exception(
+                                "Failed to delete (%s, %s) from Swift (%s)",
+                                container, name, connection_pool.os_auth_url)
 
         if removed:
             log.debug3(
@@ -868,12 +870,14 @@ def delete_unwanted_swift_files(con):
                 with swift.connection(connection_pool) as swift_connection:
                     try:
                         swift_connection.delete_object(container, name)
+                        log.debug3(
+                            'Deleted (%s, %s) from Swift (%s)',
+                            container, name, connection_pool.os_auth_url)
                     except swiftclient.ClientException as e:
                         if e.http_status != 404:
-                            raise
-                log.debug3(
-                    'Deleted (%s, %s) from Swift (%s)',
-                    container, name, connection_pool.os_auth_url)
+                            log.exception(
+                                "Failed to delete (%s, %s) from Swift (%s)",
+                                container, name, connection_pool.os_auth_url)
                 removed_count += 1
 
     if next_wanted_content_id == content_id:
diff --git a/lib/lp/services/librarianserver/tests/test_gc.py b/lib/lp/services/librarianserver/tests/test_gc.py
index 716bfa3..7e1eedb 100644
--- a/lib/lp/services/librarianserver/tests/test_gc.py
+++ b/lib/lp/services/librarianserver/tests/test_gc.py
@@ -27,8 +27,10 @@ from six.moves.urllib.parse import urljoin
 from storm.store import Store
 from swiftclient import client as swiftclient
 from testtools.matchers import (
+    AnyMatch,
     Equals,
     MatchesListwise,
+    MatchesRegex,
     )
 import transaction
 
@@ -759,6 +761,28 @@ class TestSwiftLibrarianGarbageCollection(
                     if x.http_status != 404:
                         raise
 
+    def test_DeleteUnreferencedContent_handles_timeout(self):
+        # If delete_unreferenced_content times out trying to delete a file
+        # from Swift, it logs an error and moves on.
+
+        # Merge the duplicates. This creates an
+        # unreferenced LibraryFileContent.
+        librariangc.merge_duplicates(self.con)
+
+        # Force a timeout from the Swift fixture.
+        self.pushConfig(
+            'librarian_server', os_username='timeout', swift_timeout=0.1)
+        swift.reconfigure_connection_pools()
+
+        # Delete unreferenced content.
+        librariangc.delete_unreferenced_content(self.con)
+
+        # We logged an error message.
+        self.assertThat(
+            librariangc.log.getLogBuffer().splitlines(),
+            AnyMatch(
+                MatchesRegex(r'^ERROR Failed to delete .* from Swift .*')))
+
     def test_delete_unwanted_files_handles_segments(self):
         # Large files are handled by Swift as multiple segments joined
         # by a manifest. GC treats the segments like the original file.