Less conditional state around cache size.

We were updated the configured cache size only if the cache was configured - that is less than ideal. Fixed that and added some abstractions to make things a little cleaner in my eyes.

lib/galaxy/objectstore/azure_blob.py

@@ -118,10 +118,7 @@ class AzureBlobObjectStore(ConcreteObjectStore):
 
         self._configure_connection()
 
-        # Clean cache only if value is set in galaxy.ini
-        if self.cache_size != -1 and self.enable_cache_monitor:
-            # Convert GBs to bytes for comparison
-            self.cache_size = self.cache_size * 1073741824
+        if self.enable_cache_monitor:
             self.cache_monitor = InProcessCacheMonitor(self.cache_target, self.cache_monitor_interval)
 
     def to_dict(self):
@@ -269,12 +266,12 @@ class AzureBlobObjectStore(ConcreteObjectStore):
         local_destination = self._get_cache_path(rel_path)
         try:
             log.debug("Pulling '%s' into cache to %s", rel_path, local_destination)
-            if self.cache_size > 0 and self._get_size_in_azure(rel_path) > self.cache_size:
+            if not self.cache_target.fits_in_cache(self._get_size_in_azure(rel_path)):
                 log.critical(
-                    "File %s is larger (%s) than the cache size (%s). Cannot download.",
+                    "File %s is larger (%s bytes) than the configured cache allows (%s). Cannot download.",
                     rel_path,
                     self._get_size_in_azure(rel_path),
-                    self.cache_size,
+                    self.cache_target.log_description,
                 )
                 return False
             else:

lib/galaxy/objectstore/caching.py

@@ -29,6 +29,20 @@ class CacheTarget(NamedTuple):
     size: int  # cache size in gigabytes
     limit: float  # cache limit as a percent
 
+    def fits_in_cache(self, bytes: int) -> bool:
+        # if we don't have a positive cache size - interpret it as an unbounded
+        # object store
+        if not (self.size > 0):
+            return True
+
+        if bytes > (self.size * ONE_GIGA_BYTE * self.limit):
+            return False
+        return True
+
+    @property
+    def log_description(self) -> str:
+        return f"{self.limit} percent of {self.size} gigabytes"
+
 
 def check_caches(targets: List[CacheTarget]):
     for target in targets:
@@ -121,6 +135,15 @@ def parse_caching_config_dict_from_xml(config_xml):
     return cache_dict
 
 
+def configured_cache_size(config, config_dict) -> int:
+    cache_config_dict = config_dict.get("cache") or {}
+    cache_size = cache_config_dict.get("size") or config.object_store_cache_size
+    if cache_size != -1:
+        # Convert admin-set GBs to bytes internally for quick comparison
+        cache_size = cache_size * ONE_GIGA_BYTE
+    return cache_size
+
+
 def enable_cache_monitor(config, config_dict) -> Tuple[bool, int]:
     cache_config_dict = config_dict.get("cache") or {}
     default_interval = getattr(config, "object_store_cache_monitor_interval", 600)

lib/galaxy/objectstore/cloud.py

@@ -121,10 +121,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin):
             self.use_axel = False
 
     def start_cache_monitor(self):
-        # Clean cache only if value is set in galaxy.ini
-        if self.cache_size != -1 and self.enable_cache_monitor:
-            # Convert GBs to bytes for comparison
-            self.cache_size = self.cache_size * 1073741824
+        if self.enable_cache_monitor:
             self.cache_monitor = InProcessCacheMonitor(self.cache_target, self.cache_monitor_interval)
 
     @staticmethod
@@ -384,12 +381,12 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin):
             log.debug("Pulling key '%s' into cache to %s", rel_path, self._get_cache_path(rel_path))
             key = self.bucket.objects.get(rel_path)
             # Test if cache is large enough to hold the new file
-            if self.cache_size > 0 and key.size > self.cache_size:
+            if not self.cache_target.fits_in_cache(key.size):
                 log.critical(
-                    "File %s is larger (%s) than the cache size (%s). Cannot download.",
+                    "File %s is larger (%s) than the configured cache allows (%s). Cannot download.",
                     rel_path,
                     key.size,
-                    self.cache_size,
+                    self.cache_target.log_description,
                 )
                 return False
             if self.use_axel:

lib/galaxy/objectstore/s3.py

@@ -203,10 +203,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin):
             self.use_axel = False
 
     def start_cache_monitor(self):
-        # Clean cache only if value is set in galaxy.ini
-        if self.cache_size != -1 and self.enable_cache_monitor:
-            # Convert GBs to bytes for comparison
-            self.cache_size = self.cache_size * 1073741824
+        if self.enable_cache_monitor:
             self.cache_monitor = InProcessCacheMonitor(self.cache_target, self.cache_monitor_interval)
 
     def _configure_connection(self):
@@ -394,12 +391,12 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin):
                 log.critical(message)
                 raise Exception(message)
             # Test if cache is large enough to hold the new file
-            if self.cache_size > 0 and key.size > self.cache_size:
+            if not self.cache_target.fits_in_cache(key.size):
                 log.critical(
-                    "File %s is larger (%s) than the cache size (%s). Cannot download.",
+                    "File %s is larger (%s) than the configured cache allows (%s). Cannot download.",
                     rel_path,
                     key.size,
-                    self.cache_size,
+                    self.cache_target.log_description,
                 )
                 return False
             if self.use_axel:

test/unit/objectstore/test_objectstore.py

@@ -1212,6 +1212,16 @@ def test_check_cache_sanity(tmp_path):
     assert not path.exists()
 
 
+def test_fits_in_cache_check(tmp_path):
+    cache_dir = tmp_path
+    big_cache_target = CacheTarget(cache_dir, 1, 0.2)
+    assert not big_cache_target.fits_in_cache(int(1024 * 1024 * 1024 * 0.3))
+    assert big_cache_target.fits_in_cache(int(1024 * 1024 * 1024 * 0.1))
+
+    noop_cache_target = CacheTarget(cache_dir, -1, 0.2)
+    assert noop_cache_target.fits_in_cache(1024 * 1024 * 1024 * 100)
+
+
 AZURE_BLOB_NO_CACHE_TEST_CONFIG = """<object_store type="azure_blob">
     <auth account_name="azureact" account_key="password123" />
     <container name="unique_container_name" max_chunk_size="250"/>