Try to load config_name_mapping from disk just once

tron/config/config_parse.py

@@ -11,6 +11,7 @@
 import itertools
 import logging
 import os
+from copy import deepcopy
 from typing import Dict
 from typing import List
 from typing import Optional
@@ -1066,11 +1067,14 @@ def validate_config_mapping(config_mapping):
         msg = "A config mapping requires a %s namespace"
         raise ConfigError(msg % MASTER_NAMESPACE)
 
-    master = valid_config(config_mapping.pop(MASTER_NAMESPACE))
+    # we mutate this mapping - so let's make sure that we're making a copy
+    # in case the passed-in mapping is used elsewhere
+    config_mapping_to_validate = deepcopy(config_mapping)
+    master = valid_config(config_mapping_to_validate.pop(MASTER_NAMESPACE))
     nodes = get_nodes_from_master_namespace(master)
     yield MASTER_NAMESPACE, master
 
-    for name, content in config_mapping.items():
+    for name, content in config_mapping_to_validate.items():
         context = ConfigContext(
             name,
             nodes,

tron/config/manager.py

@@ -1,6 +1,7 @@
 import hashlib
 import logging
 import os
+from copy import deepcopy
 
 from tron import yaml
 from tron.config import config_parse
@@ -97,6 +98,7 @@ class ConfigManager:
     def __init__(self, config_path, manifest=None):
         self.config_path = config_path
         self.manifest = manifest or ManifestFile(config_path)
+        self.name_mapping = None
 
     def build_file_path(self, name):
         name = name.replace(".", "_").replace(os.path.sep, "_")
@@ -107,23 +109,32 @@ def read_raw_config(self, name=schema.MASTER_NAMESPACE) -> str:
         filename = self.manifest.get_file_name(name)
         return read_raw(filename)
 
-    def write_config(self, name, content):
+    def write_config(self, name: str, content: str) -> None:
+        loaded_content = from_string(content)
         self.validate_with_fragment(
             name,
-            from_string(content),
+            content=loaded_content,
             # TODO: remove this constraint after tron triggers across clusters are supported.
             should_validate_missing_dependency=False,
         )
+        # validate_with_fragment throws if the updated content is invalid - so if we get here
+        # we know it's safe to reflect the update in our config store
+        self.get_config_name_mapping()[name] = loaded_content
+
+        # ...and then let's also persist the update to disk since memory is temporary, but disk is forever™
         filename = self.get_filename_from_manifest(name)
         write_raw(filename, content)
 
-    def delete_config(self, name):
+    def delete_config(self, name: str) -> None:
         filename = self.manifest.get_file_name(name)
         if not filename:
             msg = "Namespace %s does not exist in manifest, cannot delete."
             log.info(msg % name)
             return
 
+        # to avoid needing to reload from disk on every config load - we need to ensure that
+        # we also persist config deletions into our cache
+        self.get_config_name_mapping().pop(name, None)
         self.manifest.delete(name)
         os.remove(filename)
 
@@ -141,7 +152,11 @@ def validate_with_fragment(
         content,
         should_validate_missing_dependency=True,
     ):
-        name_mapping = self.get_config_name_mapping()
+        # NOTE: we deepcopy rather than swap values to keep this a pure function
+        # get_config_name_mapping() returns a shared dict, so this would otherwise
+        # actually update the mapping - which would be unwanted/need to be rolled-back
+        # should validation fail.
+        name_mapping = deepcopy(self.get_config_name_mapping())
         name_mapping[name] = content
         try:
             JobGraph(
@@ -152,8 +167,11 @@ def validate_with_fragment(
             raise ConfigError(str(e))
 
     def get_config_name_mapping(self):
-        seq = self.manifest.get_file_mapping().items()
-        return {name: read(filename) for name, filename in seq}
+        if self.name_mapping is None:
+            log.info("Creating config mapping cache...")
+            seq = self.manifest.get_file_mapping().items()
+            self.name_mapping = {name: read(filename) for name, filename in seq}
+        return self.name_mapping
 
     def load(self):
         """Return the fully constructed configuration."""