From 06ff5adfc099dc63629f11604a2dc4597fe0c745 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 8 Jun 2023 09:30:15 -0400 Subject: [PATCH] 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 | 11 ++++------- lib/galaxy/objectstore/caching.py | 23 +++++++++++++++++++++++ lib/galaxy/objectstore/cloud.py | 11 ++++------- lib/galaxy/objectstore/s3.py | 11 ++++------- test/unit/objectstore/test_objectstore.py | 10 ++++++++++ 5 files changed, 45 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/objectstore/azure_blob.py b/lib/galaxy/objectstore/azure_blob.py index e7be7f835c2..3b6d13e0a9c 100644 --- a/lib/galaxy/objectstore/azure_blob.py +++ b/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: diff --git a/lib/galaxy/objectstore/caching.py b/lib/galaxy/objectstore/caching.py index 0278f1bf3c1..2097da2c680 100644 --- a/lib/galaxy/objectstore/caching.py +++ b/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) diff --git a/lib/galaxy/objectstore/cloud.py b/lib/galaxy/objectstore/cloud.py index ece9e440117..6584b59b3f6 100644 --- a/lib/galaxy/objectstore/cloud.py +++ b/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: diff --git a/lib/galaxy/objectstore/s3.py b/lib/galaxy/objectstore/s3.py index 8c10c6e6856..b600e6f4cb9 100644 --- a/lib/galaxy/objectstore/s3.py +++ b/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: diff --git a/test/unit/objectstore/test_objectstore.py b/test/unit/objectstore/test_objectstore.py index 409ee89aeb5..ae922e986ff 100644 --- a/test/unit/objectstore/test_objectstore.py +++ b/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 = """