Improve DCS cleanup job error messages (#117)

Author: mephenor

services/dcs/README.md

@@ -43,21 +43,21 @@ We recommend using the provided Docker container.
 
 A pre-built version is available at [docker hub](https://hub.docker.com/repository/docker/ghga/download-controller-service):
 ```bash
-docker pull ghga/download-controller-service:6.1.1
+docker pull ghga/download-controller-service:6.1.2
 ```
 
 Or you can build the container yourself from the [`./Dockerfile`](./Dockerfile):
 ```bash
 # Execute in the repo's root dir:
-docker build -t ghga/download-controller-service:6.1.1 .
+docker build -t ghga/download-controller-service:6.1.2 .
 ```
 
 For production-ready deployment, we recommend using Kubernetes, however,
 for simple use cases, you could execute the service using docker
 on a single server:
 ```bash
 # The entrypoint is preconfigured:
-docker run -p 8080:8080 ghga/download-controller-service:6.1.1 --help
+docker run -p 8080:8080 ghga/download-controller-service:6.1.2 --help
 ```
 
 If you prefer not to use containers, you may install the service from source:

services/dcs/openapi.yaml

@@ -206,7 +206,7 @@ info:
     \ Object Storage. \n\nThis is an implementation of the DRS standard from the Global\
     \ Alliance for Genomics and Health, please find more information at: https://github.com/ga4gh/data-repository-service-schemas"
   title: Download Controller Service
-  version: 6.1.1
+  version: 6.1.2
 openapi: 3.1.0
 paths:
   /health:

services/dcs/pyproject.toml

@@ -21,7 +21,7 @@ classifiers = [
     "Intended Audience :: Developers",
 ]
 name = "dcs"
-version = "6.1.1"
+version = "6.1.2"
 description = "Download Controller Service - a GA4GH DRS-compliant service for delivering files from S3 encrypted according to the GA4GH Crypt4GH standard."
 
 

services/dcs/src/dcs/core/data_repository.py

@@ -249,7 +249,9 @@ async def access_drs_object(self, *, drs_id: str) -> models.DrsObjectResponseMod
         )
 
     async def cleanup_outbox_buckets(
-        self, *, object_storages_config: S3ObjectStoragesConfig
+        self,
+        *,
+        object_storages_config: S3ObjectStoragesConfig,
     ):
         """Run cleanup task for all outbox buckets configured in the service config."""
         for storage_alias in object_storages_config.object_storages:
@@ -265,16 +267,19 @@ async def cleanup_outbox(self, *, storage_alias: str):
         """
         # Run on demand through CLI, so crashing should be ok if the alias is not configured
         log.info(
-            f"Starting outbox cleanup for storage identified by alias '{storage_alias}'."
+            f"Starting outbox cleanup for storage identified by alias {storage_alias}."
         )
         try:
             bucket_id, object_storage = self._object_storages.for_alias(storage_alias)
-        except KeyError as exc:
+        except KeyError:
             storage_alias_not_configured = self.StorageAliasNotConfiguredError(
                 alias=storage_alias
             )
             log.critical(storage_alias_not_configured)
-            raise storage_alias_not_configured from exc
+            log.warning(
+                f"Skipping outbox cleanup for storage {storage_alias} as it is not configured."
+            )
+            return
 
         threshold = utc_dates.now_as_utc() - timedelta(
             days=self._config.outbox_cache_timeout
@@ -294,13 +299,14 @@ async def cleanup_outbox(self, *, storage_alias: str):
             except ResourceNotFoundError as error:
                 cleanup_error = self.CleanupError(object_id=object_id, from_error=error)
                 log.critical(cleanup_error)
-                raise cleanup_error from error
+                log.warning(
+                    f"Object with id {object_id} in storage {storage_alias} not found in database, skipping."
+                )
+                continue
 
             # only remove file if last access is later than oubtox_cache_timeout days ago
             if drs_object.last_accessed <= threshold:
-                log.info(
-                    f"Deleting object '{object_id}' from storage '{storage_alias}'."
-                )
+                log.info(f"Deleting object {object_id} from storage {storage_alias}.")
                 try:
                     await object_storage.delete_object(
                         bucket_id=bucket_id, object_id=object_id
@@ -313,7 +319,10 @@ async def cleanup_outbox(self, *, storage_alias: str):
                         object_id=object_id, from_error=error
                     )
                     log.critical(cleanup_error)
-                    raise cleanup_error from error
+                    log.warning(
+                        f"Could not delete object with id {object_id} from storage {storage_alias}."
+                    )
+                    continue
 
     async def register_new_file(self, *, file: models.DrsObjectBase):
         """Register a file as a new DRS Object."""

services/dcs/src/dcs/ports/inbound/data_repository.py

@@ -39,9 +39,7 @@ class CleanupError(RuntimeError):
         """
 
         def __init__(self, *, object_id: str, from_error: Exception):
-            message = (
-                f"Could not remove object {object_id} from outbox: {str(from_error)}"
-            )
+            message = f"Could not remove object {object_id} from outbox bucket: {str(from_error)}"
             super().__init__(message)
 
     class DrsObjectNotFoundError(RuntimeError):

services/dcs/tests_dcs/test_typical_journey.py

@@ -16,6 +16,7 @@
 """Tests typical user journeys"""
 
 import json
+import logging
 import re
 
 import httpx
@@ -220,7 +221,7 @@ async def test_happy_deletion(
     )
 
 
-async def test_bucket_cleanup(cleanup_fixture: CleanupFixture):
+async def test_bucket_cleanup(cleanup_fixture: CleanupFixture, caplog):
     """Test multiple outbox bucket cleanup handling."""
     data_repository = cleanup_fixture.joint.data_repository
 
@@ -246,7 +247,15 @@ async def test_bucket_cleanup(cleanup_fixture: CleanupFixture):
         object_id=expired_object.object_id,
     )
 
-    with pytest.raises(data_repository.StorageAliasNotConfiguredError):
+    with caplog.at_level(logging.ERROR):
         await data_repository.cleanup_outbox(
             storage_alias=cleanup_fixture.joint.endpoint_aliases.fake
         )
+
+    expected_message = str(
+        data_repository.StorageAliasNotConfiguredError(
+            alias=cleanup_fixture.joint.endpoint_aliases.fake
+        )
+    )
+
+    assert expected_message in caplog.records[0].message