Merge pull request #16144 from jmchilton/caching_refactor

Refactor caching object stores ahead of adding task-based store.

lib/galaxy/config/schemas/config_schema.yml

@@ -974,6 +974,23 @@ mapping:
           Configuration file for the object store
           If this is set and exists, it overrides any other objectstore settings.
 
+      object_store_cache_path:
+        type: str
+        default: object_store_cache
+        path_resolves_to: cache_dir
+        required: false
+        desc: |
+          Default cache path for caching object stores if cache not configured for
+          that object store entry.
+
+      object_store_cache_size:
+        type: int
+        default: -1
+        required: false
+        desc: |
+          Default cache size for caching object stores if cache not configured for
+          that object store entry.
+
       object_store_store_by:
         type: str
         required: false

lib/galaxy/objectstore/azure_blob.py

@@ -30,6 +30,7 @@ from . import ConcreteObjectStore
 from .caching import (
     CacheTarget,
     InProcessCacheMonitor,
+    parse_caching_config_dict_from_xml,
 )
 
 NO_BLOBSERVICE_ERROR_MESSAGE = (
@@ -51,9 +52,7 @@ def parse_config_xml(config_xml):
         container_name = container_xml.get("name")
         max_chunk_size = int(container_xml.get("max_chunk_size", 250))  # currently unused
 
-        c_xml = config_xml.findall("cache")[0]
-        cache_size = float(c_xml.get("size", -1))
-        staging_path = c_xml.get("path", None)
+        cache_dict = parse_caching_config_dict_from_xml(config_xml)
 
         tag, attrs = "extra_dir", ("type", "path")
         extra_dirs = config_xml.findall(tag)
@@ -71,10 +70,7 @@ def parse_config_xml(config_xml):
                 "name": container_name,
                 "max_chunk_size": max_chunk_size,
             },
-            "cache": {
-                "size": cache_size,
-                "path": staging_path,
-            },
+            "cache": cache_dict,
             "extra_dirs": extra_dirs,
             "private": ConcreteObjectStore.parse_private_from_config_xml(config_xml),
         }
@@ -101,7 +97,7 @@ class AzureBlobObjectStore(ConcreteObjectStore):
 
         auth_dict = config_dict["auth"]
         container_dict = config_dict["container"]
-        cache_dict = config_dict["cache"]
+        cache_dict = config_dict.get("cache") or {}
         self.enable_cache_monitor = config_dict.get("enable_cache_monitor", True)
 
         self.account_name = auth_dict.get("account_name")
@@ -110,7 +106,7 @@ class AzureBlobObjectStore(ConcreteObjectStore):
         self.container_name = container_dict.get("name")
         self.max_chunk_size = container_dict.get("max_chunk_size", 250)  # currently unused
 
-        self.cache_size = cache_dict.get("size", -1)
+        self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size
         self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path
 
         self._initialize()

lib/galaxy/objectstore/caching.py

@@ -98,6 +98,23 @@ def _get_cache_size_files(cache_path) -> Tuple[int, FileListT]:
     return cache_size, file_list
 
 
+def parse_caching_config_dict_from_xml(config_xml):
+    cache_els = config_xml.findall("cache")
+    if len(cache_els) > 0:
+        c_xml = config_xml.findall("cache")[0]
+        cache_size = float(c_xml.get("size", -1))
+
+        staging_path = c_xml.get("path", None)
+
+        cache_dict = {
+            "size": cache_size,
+            "path": staging_path,
+        }
+    else:
+        cache_dict = {}
+    return cache_dict
+
+
 class InProcessCacheMonitor:
     def __init__(self, cache_target: CacheTarget, interval: int = 30, initial_sleep: Optional[int] = 2):
         # This Event object is initialized to False

lib/galaxy/objectstore/cloud.py

@@ -86,7 +86,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin):
 
         bucket_dict = config_dict["bucket"]
         connection_dict = config_dict.get("connection", {})
-        cache_dict = config_dict["cache"]
+        cache_dict = config_dict.get("cache") or {}
         self.enable_cache_monitor = config_dict.get("enable_cache_monitor", True)
 
         self.provider = config_dict["provider"]
@@ -101,7 +101,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin):
         self.is_secure = connection_dict.get("is_secure", True)
         self.conn_path = connection_dict.get("conn_path", "/")
 
-        self.cache_size = cache_dict.get("size", -1)
+        self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size
         self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path
 
         self._initialize()

lib/galaxy/objectstore/irods.py

@@ -202,10 +202,8 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin):
         if self.connection_pool_monitor_interval is None:
             _config_dict_error("connection->connection_pool_monitor_interval")
 
-        cache_dict = config_dict["cache"]
-        if cache_dict is None:
-            _config_dict_error("cache")
-        self.cache_size = cache_dict.get("size", -1)
+        cache_dict = config_dict.get("cache") or {}
+        self.cache_size = cache_dict.get("size") or self.config.object_store_cache_path
         if self.cache_size is None:
             _config_dict_error("cache->size")
         self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path

lib/galaxy/objectstore/s3.py

@@ -35,6 +35,7 @@ from . import ConcreteObjectStore
 from .caching import (
     CacheTarget,
     InProcessCacheMonitor,
+    parse_caching_config_dict_from_xml,
 )
 from .s3_multipart_upload import multipart_upload
 
@@ -70,10 +71,7 @@ def parse_config_xml(config_xml):
         is_secure = string_as_bool(cn_xml.get("is_secure", "True"))
         conn_path = cn_xml.get("conn_path", "/")
 
-        c_xml = config_xml.findall("cache")[0]
-        cache_size = float(c_xml.get("size", -1))
-
-        staging_path = c_xml.get("path", None)
+        cache_dict = parse_caching_config_dict_from_xml(config_xml)
 
         tag, attrs = "extra_dir", ("type", "path")
         extra_dirs = config_xml.findall(tag)
@@ -100,10 +98,7 @@ def parse_config_xml(config_xml):
                 "is_secure": is_secure,
                 "conn_path": conn_path,
             },
-            "cache": {
-                "size": cache_size,
-                "path": staging_path,
-            },
+            "cache": cache_dict,
             "extra_dirs": extra_dirs,
             "private": ConcreteObjectStore.parse_private_from_config_xml(config_xml),
         }
@@ -158,7 +153,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin):
         auth_dict = config_dict["auth"]
         bucket_dict = config_dict["bucket"]
         connection_dict = config_dict.get("connection", {})
-        cache_dict = config_dict["cache"]
+        cache_dict = config_dict.get("cache") or {}
         self.enable_cache_monitor = config_dict.get("enable_cache_monitor", True)
 
         self.access_key = auth_dict.get("access_key")
@@ -174,7 +169,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin):
         self.is_secure = connection_dict.get("is_secure", True)
         self.conn_path = connection_dict.get("conn_path", "/")
 
-        self.cache_size = cache_dict.get("size", -1)
+        self.cache_size = cache_dict.get("size") or self.config.object_store_cache_size
         self.staging_path = cache_dict.get("path") or self.config.object_store_cache_path
 
         extra_dirs = {e["type"]: e["path"] for e in config_dict.get("extra_dirs", [])}

lib/galaxy/objectstore/unittest_utils/__init__.py

@@ -39,6 +39,7 @@ class Config:
             config_file = "store.yaml"
         self.write(config_str, config_file)
         config = MockConfig(self.temp_directory, config_file, store_by=store_by)
+        self.global_config = config
         if clazz is None:
             self.object_store = objectstore.build_object_store_from_config(config)
         elif config_file == "store.xml":
@@ -68,6 +69,7 @@ class MockConfig:
         self.file_path = temp_directory
         self.object_store_config_file = os.path.join(temp_directory, config_file)
         self.object_store_check_old_style = False
+        self.object_store_cache_size = -1
         self.object_store_cache_path = os.path.join(temp_directory, "staging")
         self.object_store_store_by = store_by
         self.jobs_directory = temp_directory

test/unit/objectstore/test_objectstore.py

@@ -600,22 +600,22 @@ def test_hiercachical_backend_must_share_quota_source():
 # Unit testing the cloud and advanced infrastructure object stores is difficult, but
 # we can at least stub out initializing and test the configuration of these things from
 # XML and dicts.
-class UnitializedPithosObjectStore(PithosObjectStore):
+class UninitializedPithosObjectStore(PithosObjectStore):
     def _initialize(self):
         pass
 
 
-class UnitializeS3ObjectStore(S3ObjectStore):
+class UninitializedS3ObjectStore(S3ObjectStore):
     def _initialize(self):
         pass
 
 
-class UnitializedAzureBlobObjectStore(AzureBlobObjectStore):
+class UninitializedAzureBlobObjectStore(AzureBlobObjectStore):
     def _initialize(self):
         pass
 
 
-class UnitializedCloudObjectStore(Cloud):
+class UninitializedCloudObjectStore(Cloud):
     def _initialize(self):
         pass
 
@@ -650,7 +650,7 @@ extra_dirs:
 
 def test_config_parse_pithos():
     for config_str in [PITHOS_TEST_CONFIG, PITHOS_TEST_CONFIG_YAML]:
-        with TestConfig(config_str, clazz=UnitializedPithosObjectStore) as (directory, object_store):
+        with TestConfig(config_str, clazz=UninitializedPithosObjectStore) as (directory, object_store):
             configured_config_dict = object_store.config_dict
             _assert_has_keys(configured_config_dict, ["auth", "container", "extra_dirs"])
 
@@ -719,7 +719,7 @@ extra_dirs:
 
 def test_config_parse_s3():
     for config_str in [S3_TEST_CONFIG, S3_TEST_CONFIG_YAML]:
-        with TestConfig(config_str, clazz=UnitializeS3ObjectStore) as (directory, object_store):
+        with TestConfig(config_str, clazz=UninitializedS3ObjectStore) as (directory, object_store):
             assert object_store.private
             assert object_store.access_key == "access_moo"
             assert object_store.secret_key == "secret_cow"
@@ -767,6 +767,41 @@ def test_config_parse_s3():
             assert len(extra_dirs) == 2
 
 
+S3_DEFAULT_CACHE_TEST_CONFIG = """<object_store type="s3" private="true">
+     <auth access_key="access_moo" secret_key="secret_cow" />
+     <bucket name="unique_bucket_name_all_lowercase" use_reduced_redundancy="False" />
+     <extra_dir type="job_work" path="database/job_working_directory_s3"/>
+     <extra_dir type="temp" path="database/tmp_s3"/>
+</object_store>
+"""
+
+
+S3_DEFAULT_CACHE_TEST_CONFIG_YAML = """
+type: s3
+private: true
+auth:
+  access_key: access_moo
+  secret_key: secret_cow
+
+bucket:
+  name: unique_bucket_name_all_lowercase
+  use_reduced_redundancy: false
+
+extra_dirs:
+- type: job_work
+  path: database/job_working_directory_s3
+- type: temp
+  path: database/tmp_s3
+"""
+
+
+def test_config_parse_s3_with_default_cache():
+    for config_str in [S3_DEFAULT_CACHE_TEST_CONFIG, S3_DEFAULT_CACHE_TEST_CONFIG_YAML]:
+        with TestConfig(config_str, clazz=UninitializedS3ObjectStore) as (directory, object_store):
+            assert object_store.cache_size == -1
+            assert object_store.staging_path == directory.global_config.object_store_cache_path
+
+
 CLOUD_AWS_TEST_CONFIG = """<object_store type="cloud" provider="aws">
      <auth access_key="access_moo" secret_key="secret_cow" />
      <bucket name="unique_bucket_name_all_lowercase" use_reduced_redundancy="False" />
@@ -882,7 +917,7 @@ def test_config_parse_cloud():
             path = os.path.join(tmpdir, "gcp.config")
             open(path, "w").write("some_gcp_config")
             config_str = config_str.replace("gcp.config", path)
-        with TestConfig(config_str, clazz=UnitializedCloudObjectStore) as (directory, object_store):
+        with TestConfig(config_str, clazz=UninitializedCloudObjectStore) as (directory, object_store):
             assert object_store.bucket_name == "unique_bucket_name_all_lowercase"
             assert object_store.use_rr is False
 
@@ -949,7 +984,7 @@ CLOUD_AWS_NO_AUTH_TEST_CONFIG = """<object_store type="cloud" provider="aws">
 
 def test_config_parse_cloud_noauth_for_aws():
     for config_str in [CLOUD_AWS_NO_AUTH_TEST_CONFIG]:
-        with TestConfig(config_str, clazz=UnitializedCloudObjectStore) as (directory, object_store):
+        with TestConfig(config_str, clazz=UninitializedCloudObjectStore) as (directory, object_store):
             assert object_store.bucket_name == "unique_bucket_name_all_lowercase"
             assert object_store.use_rr is False
 
@@ -996,6 +1031,22 @@ def test_config_parse_cloud_noauth_for_aws():
             assert len(extra_dirs) == 2
 
 
+CLOUD_AWS_NO_CACHE_TEST_CONFIG = """<object_store type="cloud" provider="aws">
+     <auth />
+     <bucket name="unique_bucket_name_all_lowercase" use_reduced_redundancy="False" />
+     <extra_dir type="job_work" path="database/job_working_directory_cloud"/>
+     <extra_dir type="temp" path="database/tmp_cloud"/>
+</object_store>
+"""
+
+
+def test_config_parse_cloud_no_cache_for_aws():
+    for config_str in [CLOUD_AWS_NO_CACHE_TEST_CONFIG]:
+        with TestConfig(config_str, clazz=UninitializedCloudObjectStore) as (directory, object_store):
+            assert object_store.staging_path == directory.global_config.object_store_cache_path
+            assert object_store.cache_size == -1
+
+
 AZURE_BLOB_TEST_CONFIG = """<object_store type="azure_blob">
     <auth account_name="azureact" account_key="password123" />
     <container name="unique_container_name" max_chunk_size="250"/>
@@ -1030,7 +1081,7 @@ extra_dirs:
 
 def test_config_parse_azure():
     for config_str in [AZURE_BLOB_TEST_CONFIG, AZURE_BLOB_TEST_CONFIG_YAML]:
-        with TestConfig(config_str, clazz=UnitializedAzureBlobObjectStore) as (directory, object_store):
+        with TestConfig(config_str, clazz=UninitializedAzureBlobObjectStore) as (directory, object_store):
             assert object_store.account_name == "azureact"
             assert object_store.account_key == "password123"
 
@@ -1103,6 +1154,40 @@ def test_check_cache_sanity(tmp_path):
     assert not path.exists()
 
 
+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"/>
+    <extra_dir type="job_work" path="database/job_working_directory_azure"/>
+    <extra_dir type="temp" path="database/tmp_azure"/>
+</object_store>
+"""
+
+
+AZURE_BLOB_NO_CACHE_TEST_CONFIG_YAML = """
+type: azure_blob
+auth:
+  account_name: azureact
+  account_key: password123
+
+container:
+  name: unique_container_name
+  max_chunk_size: 250
+
+extra_dirs:
+- type: job_work
+  path: database/job_working_directory_azure
+- type: temp
+  path: database/tmp_azure
+"""
+
+
+def test_config_parse_azure_no_cache():
+    for config_str in [AZURE_BLOB_NO_CACHE_TEST_CONFIG, AZURE_BLOB_NO_CACHE_TEST_CONFIG_YAML]:
+        with TestConfig(config_str, clazz=UninitializedAzureBlobObjectStore) as (directory, object_store):
+            assert object_store.cache_size == -1
+            assert object_store.staging_path == directory.global_config.object_store_cache_path
+
+
 class MockDataset:
     def __init__(self, id):
         self.id = id