From d942823750067673413a2ca79168e6cff6714021 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 11:51:41 +0100 Subject: [PATCH 01/17] Fix minor typos in cache config docstring --- synapse/config/cache.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 94d852f413d9..1d472e6c54da 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -69,11 +69,11 @@ def _canonicalise_cache_name(cache_name: str) -> str: def add_resizable_cache( cache_name: str, cache_resize_callback: Callable[[float], None] ) -> None: - """Register a cache that's size can dynamically change + """Register a cache whose size can dynamically change Args: cache_name: A reference to the cache - cache_resize_callback: A callback function that will be ran whenever + cache_resize_callback: A callback function that will run whenever the cache needs to be resized """ # Some caches have '*' in them which we strip out. From 077ce90f2e594fd858f8fe05ebb73920922245ea Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 12:42:18 +0100 Subject: [PATCH 02/17] Add drive-by annotations in s.config._base --- synapse/config/_base.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 179aa7ff887e..5816cb2dc499 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -22,6 +22,7 @@ from textwrap import dedent from typing import ( Any, + ClassVar, Dict, Iterable, List, @@ -119,7 +120,7 @@ class Config: defined in subclasses. """ - section: str + section: ClassVar[str] def __init__(self, root_config: "RootConfig" = None): self.root = root_config @@ -309,7 +310,7 @@ class RootConfig: class, lower-cased and with "Config" removed. """ - config_classes = [] + config_classes: List[Type[Config]] = [] def __init__(self): for config_class in self.config_classes: From 1f668be1d51e84bf15ee490b7fba42f10ac81915 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 12:42:54 +0100 Subject: [PATCH 03/17] Define `RootConfig.reload_config_section` --- synapse/config/_base.py | 29 ++++++++++++++++++++++++----- synapse/config/_base.pyi | 5 ++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 5816cb2dc499..f694b2e6bd68 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -16,6 +16,7 @@ import argparse import errno +import logging import os from collections import OrderedDict from hashlib import sha256 @@ -23,6 +24,7 @@ from typing import ( Any, ClassVar, + Collection, Dict, Iterable, List, @@ -41,6 +43,8 @@ from synapse.util.templates import _create_mxc_to_http_filter, _format_ts_filter +logger = logging.getLogger(__name__) + class ConfigError(Exception): """Represents a problem parsing the configuration @@ -312,7 +316,10 @@ class RootConfig: config_classes: List[Type[Config]] = [] - def __init__(self): + def __init__(self, config_files: Collection[str] = ()): + # Capture absolute paths here, so we can reload config after we daemonize. + self.config_files = [os.path.abspath(path) for path in config_files] + for config_class in self.config_classes: if config_class.section is None: raise ValueError("%r requires a section name" % (config_class,)) @@ -513,12 +520,10 @@ def load_config_with_parser( object from parser.parse_args(..)` """ - obj = cls() - config_args = parser.parse_args(argv) config_files = find_config_files(search_paths=config_args.config_path) - + obj = cls(config_files) if not config_files: parser.error("Must supply a config file.") @@ -628,7 +633,7 @@ def load_or_generate_config( generate_missing_configs = config_args.generate_missing_configs - obj = cls() + obj = cls(config_files) if config_args.generate_config: if config_args.report_stats is None: @@ -728,6 +733,20 @@ def generate_missing_files( ) -> None: self.invoke_all("generate_files", config_dict, config_dir_path) + def reload_config_section(self, section_name: str) -> None: + """Reconstruct the given config section, leaving all others unchanged. + + :raises ValueError: if the given `section` does not exist. + :raises ConfigError: for any other problems reloading config. + """ + existing_config: Optional[Config] = getattr(self, section_name, None) + if existing_config is None: + raise ValueError(f"Unknown config section '{section_name}'") + logger.info("Reloading config section '%s'", section_name) + + new_config_data = read_config_files(self.config_files) + existing_config.read_config(new_config_data) + def read_config_files(config_files: Iterable[str]) -> Dict[str, Any]: """Read the config files into a dict diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index bd092f956dde..4ef9c18f1c66 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -1,6 +1,7 @@ import argparse from typing import ( Any, + Collection, Dict, Iterable, List, @@ -117,7 +118,8 @@ class RootConfig: background_updates: background_updates.BackgroundUpdateConfig config_classes: List[Type["Config"]] = ... - def __init__(self) -> None: ... + config_files: List[str] + def __init__(self, config_files: Collection[str] = ...) -> None: ... def invoke_all( self, func_name: str, *args: Any, **kwargs: Any ) -> MutableMapping[str, Any]: ... @@ -157,6 +159,7 @@ class RootConfig: def generate_missing_files( self, config_dict: dict, config_dir_path: str ) -> None: ... + def reload_config_section(self, section_name: str) -> None: ... class Config: root: RootConfig From 2a5ce2f5fc659948edeb982749deb1937e0a3b91 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 14:08:23 +0100 Subject: [PATCH 04/17] Move `format_config_error` to s.config._base right below the definition of ConfigError. --- synapse/app/homeserver.py | 36 ++---------------------------------- synapse/config/_base.py | 33 +++++++++++++++++++++++++++++++++ synapse/config/_base.pyi | 3 +++ 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 0f75e7b9d491..4c6c0658ab14 100644 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -16,7 +16,7 @@ import logging import os import sys -from typing import Dict, Iterable, Iterator, List +from typing import Dict, Iterable, List from matrix_common.versionstring import get_distribution_version_string @@ -45,7 +45,7 @@ redirect_stdio_to_logs, register_start, ) -from synapse.config._base import ConfigError +from synapse.config._base import ConfigError, format_config_error from synapse.config.emailconfig import ThreepidBehaviour from synapse.config.homeserver import HomeServerConfig from synapse.config.server import ListenerConfig @@ -399,38 +399,6 @@ async def start() -> None: return hs -def format_config_error(e: ConfigError) -> Iterator[str]: - """ - Formats a config error neatly - - The idea is to format the immediate error, plus the "causes" of those errors, - hopefully in a way that makes sense to the user. For example: - - Error in configuration at 'oidc_config.user_mapping_provider.config.display_name_template': - Failed to parse config for module 'JinjaOidcMappingProvider': - invalid jinja template: - unexpected end of template, expected 'end of print statement'. - - Args: - e: the error to be formatted - - Returns: An iterator which yields string fragments to be formatted - """ - yield "Error in configuration" - - if e.path: - yield " at '%s'" % (".".join(e.path),) - - yield ":\n %s" % (e.msg,) - - parent_e = e.__cause__ - indent = 1 - while parent_e: - indent += 1 - yield ":\n%s%s" % (" " * indent, str(parent_e)) - parent_e = parent_e.__cause__ - - def run(hs: HomeServer) -> None: _base.start_reactor( "synapse-homeserver", diff --git a/synapse/config/_base.py b/synapse/config/_base.py index f694b2e6bd68..d78641ca898e 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -27,6 +27,7 @@ Collection, Dict, Iterable, + Iterator, List, MutableMapping, Optional, @@ -60,6 +61,38 @@ def __init__(self, msg: str, path: Optional[Iterable[str]] = None): self.path = path +def format_config_error(e: ConfigError) -> Iterator[str]: + """ + Formats a config error neatly + + The idea is to format the immediate error, plus the "causes" of those errors, + hopefully in a way that makes sense to the user. For example: + + Error in configuration at 'oidc_config.user_mapping_provider.config.display_name_template': + Failed to parse config for module 'JinjaOidcMappingProvider': + invalid jinja template: + unexpected end of template, expected 'end of print statement'. + + Args: + e: the error to be formatted + + Returns: An iterator which yields string fragments to be formatted + """ + yield "Error in configuration" + + if e.path: + yield " at '%s'" % (".".join(e.path),) + + yield ":\n %s" % (e.msg,) + + parent_e = e.__cause__ + indent = 1 + while parent_e: + indent += 1 + yield ":\n%s%s" % (" " * indent, str(parent_e)) + parent_e = parent_e.__cause__ + + # We split these messages out to allow packages to override with package # specific instructions. MISSING_REPORT_STATS_CONFIG_INSTRUCTIONS = """\ diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index 4ef9c18f1c66..b1b3f302d96a 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -4,6 +4,7 @@ from typing import ( Collection, Dict, Iterable, + Iterator, List, MutableMapping, Optional, @@ -65,6 +66,8 @@ class ConfigError(Exception): self.msg = msg self.path = path +def format_config_error(e: ConfigError) -> Iterator[str]: ... + MISSING_REPORT_STATS_CONFIG_INSTRUCTIONS: str MISSING_REPORT_STATS_SPIEL: str MISSING_SERVER_NAME: str From 69da4ca454fcc636451f37b947503e968db81825 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 12:45:36 +0100 Subject: [PATCH 05/17] Reload cache config on sighup --- synapse/app/_base.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index d28b87a3f4d8..553ca25136b3 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -51,6 +51,8 @@ from synapse.api.constants import MAX_PDU_SIZE from synapse.app import check_bind_error from synapse.app.phone_stats_home import start_phone_stats_home +from synapse.config import ConfigError +from synapse.config._base import format_config_error from synapse.config.homeserver import HomeServerConfig from synapse.config.server import ManholeConfig from synapse.crypto import context_factory @@ -426,6 +428,7 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: signal.signal(signal.SIGHUP, run_sighup) register_sighup(refresh_certificate, hs) + register_sighup(reload_cache_config, hs.config) # Load the certificate from disk. refresh_certificate(hs) @@ -480,6 +483,22 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: atexit.register(gc.freeze) +def reload_cache_config(config: HomeServerConfig) -> None: + # For mypy's benefit. It can't know that we haven't altered `config` by the time + # we call this closure. + assert config is not None + try: + # This will call CacheConfig.read_config, which will automatically call + # CacheConfig.resize_all_caches. + config.reload_config_section("caches") + except ConfigError as e: + logger.warning("Failed to reload cache config") + for f in format_config_error(e): + logger.warning(f) + else: + logger.debug("New cache config: %s", config.caches.__dict__) + + def setup_sentry(hs: "HomeServer") -> None: """Enable sentry integration, if enabled in configuration""" From 85f4385875d8044522746c8f550c408b2bad3b53 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 13:08:51 +0100 Subject: [PATCH 06/17] Changelog --- changelog.d/12673.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12673.feature diff --git a/changelog.d/12673.feature b/changelog.d/12673.feature new file mode 100644 index 000000000000..f2bddd6e1c27 --- /dev/null +++ b/changelog.d/12673.feature @@ -0,0 +1 @@ +Synapse will now reload [cache config](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#caching) when it receives a [SIGHUP](https://en.wikipedia.org/wiki/SIGHUP) signal. From 1febfd6c8132a53ce0ccd1b409d8a9ee6363f335 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 9 May 2022 14:44:26 +0100 Subject: [PATCH 07/17] If `global_factor` is omitted, use 0.5 on reload Consider the following scenario: 1. Start synapse with config that omits a `caches.global_factor`. 2. `CacheProperties.default_factor_size` is now 0.5. 3. Edit config to set `caches.global_factor = 2`. 4. Reload config. 5. Read `CacheProperties.default_factor_size`. - Without patch: still 2 - With patch: backt to 0.5 Without this commit, when we reload config we will compute `self.global_factor` to be the _previous_ global cache factor. I think this is surprising: I would that factor to reflect what the config file says. --- synapse/config/cache.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 1d472e6c54da..983de5a56f34 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -180,9 +180,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.cache_factors: Dict[str, float] = {} cache_config = config.get("caches") or {} - self.global_factor = cache_config.get( - "global_factor", properties.default_factor_size - ) + self.global_factor = cache_config.get("global_factor", _DEFAULT_FACTOR_SIZE) if not isinstance(self.global_factor, (int, float)): raise ConfigError("caches.global_factor must be a number.") From 4039b2d5c8a50d011b09086c1e573b3032df2b0a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 13:58:45 +0100 Subject: [PATCH 08/17] Annotate CacheConfig's instance vars at the top so I can see what it actually contains --- synapse/config/cache.py | 11 +++++++++-- synapse/http/client.py | 2 +- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 983de5a56f34..1c400092e4ba 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -96,6 +96,13 @@ class CacheConfig(Config): section = "caches" _environ = os.environ + event_cache_size: int + cache_factors: Dict[str, float] + global_factor: float + track_memory_usage: bool + expiry_time_msec: Optional[int] + sync_response_cache_duration: int + @staticmethod def reset() -> None: """Resets the caches to their defaults. Used for tests.""" @@ -177,7 +184,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: self.event_cache_size = self.parse_size( config.get("event_cache_size", _DEFAULT_EVENT_CACHE_SIZE) ) - self.cache_factors: Dict[str, float] = {} + self.cache_factors = {} cache_config = config.get("caches") or {} self.global_factor = cache_config.get("global_factor", _DEFAULT_FACTOR_SIZE) @@ -228,7 +235,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: cache_entry_ttl = cache_config.get("cache_entry_ttl", "30m") if expire_caches: - self.expiry_time_msec: Optional[int] = self.parse_duration(cache_entry_ttl) + self.expiry_time_msec = self.parse_duration(cache_entry_ttl) else: self.expiry_time_msec = None diff --git a/synapse/http/client.py b/synapse/http/client.py index 8310fb466ac5..b2c9a7c67090 100644 --- a/synapse/http/client.py +++ b/synapse/http/client.py @@ -348,7 +348,7 @@ def __init__( # XXX: The justification for using the cache factor here is that larger instances # will need both more cache and more connections. # Still, this should probably be a separate dial - pool.maxPersistentPerHost = max((100 * hs.config.caches.global_factor, 5)) + pool.maxPersistentPerHost = max(int(100 * hs.config.caches.global_factor), 5) pool.cachedConnectionTimeout = 2 * 60 self.agent: IAgent = ProxyAgent( From b7a6ec5ecc1000bf3bd8e76c595090a67d93bfb4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 14:29:00 +0100 Subject: [PATCH 09/17] Separate reading and applying cache config --- synapse/app/_base.py | 3 +-- synapse/app/generic_worker.py | 1 + synapse/config/cache.py | 24 +++++++++++++----------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 553ca25136b3..809ff3019ee0 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -488,8 +488,6 @@ def reload_cache_config(config: HomeServerConfig) -> None: # we call this closure. assert config is not None try: - # This will call CacheConfig.read_config, which will automatically call - # CacheConfig.resize_all_caches. config.reload_config_section("caches") except ConfigError as e: logger.warning("Failed to reload cache config") @@ -497,6 +495,7 @@ def reload_cache_config(config: HomeServerConfig) -> None: logger.warning(f) else: logger.debug("New cache config: %s", config.caches.__dict__) + config.caches.resize_all_caches() def setup_sentry(hs: "HomeServer") -> None: diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index 07dddc0b1326..1a0a7406cc30 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -459,6 +459,7 @@ def start(config_options: List[str]) -> None: synapse.events.USE_FROZEN_DICTS = config.server.use_frozen_dicts synapse.util.caches.TRACK_MEMORY_USAGE = config.caches.track_memory_usage + config.caches.resize_all_caches() if config.server.gc_seconds: synapse.metrics.MIN_TIME_BETWEEN_GCS = config.server.gc_seconds diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 1c400092e4ba..350227ab8421 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -181,6 +181,11 @@ def generate_config_section(self, **kwargs: Any) -> str: """ def read_config(self, config: JsonDict, **kwargs: Any) -> None: + """Populate this config object with values from `config`. + + This method does NOT resize existing or future caches: use `resize_all_caches`. + We use two separate methods so that we can reject bad config before applying it. + """ self.event_cache_size = self.parse_size( config.get("event_cache_size", _DEFAULT_EVENT_CACHE_SIZE) ) @@ -191,9 +196,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: if not isinstance(self.global_factor, (int, float)): raise ConfigError("caches.global_factor must be a number.") - # Set the global one so that it's reflected in new caches - properties.default_factor_size = self.global_factor - # Load cache factors from the config individual_factors = cache_config.get("per_cache_factors") or {} if not isinstance(individual_factors, dict): @@ -259,19 +261,19 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: cache_config.get("sync_response_cache_duration", 0) ) - # Resize all caches (if necessary) with the new factors we've loaded - self.resize_all_caches() - - # Store this function so that it can be called from other classes without - # needing an instance of Config - properties.resize_all_caches_func = self.resize_all_caches - def resize_all_caches(self) -> None: - """Ensure all cache sizes are up to date + """Ensure all cache sizes are up-to-date. For each cache, run the mapped callback function with either a specific cache factor or the default, global one. """ + # Set the global factor size, so that new caches are appropriately sized. + properties.default_factor_size = self.global_factor + + # Store this function so that it can be called from other classes without + # needing an instance of CacheConfig + properties.resize_all_caches_func = self.resize_all_caches + # block other threads from modifying _CACHES while we iterate it. with _CACHES_LOCK: for cache_name, callback in _CACHES.items(): From 519f98b8d6b472d923db8b734b1395c7d4b407ed Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 14:29:40 +0100 Subject: [PATCH 10/17] Remove redundant assert --- synapse/app/_base.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 809ff3019ee0..da4398b2c2d6 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -484,9 +484,6 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: def reload_cache_config(config: HomeServerConfig) -> None: - # For mypy's benefit. It can't know that we haven't altered `config` by the time - # we call this closure. - assert config is not None try: config.reload_config_section("caches") except ConfigError as e: From b9595adcd4a79b500904cd39755e8555f075df1d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 14:35:59 +0100 Subject: [PATCH 11/17] Create a new config object when reloading This ensures we don't partially overwrite the existing object if we encounter an exception during `read_config()`. --- synapse/app/_base.py | 8 ++++++-- synapse/config/_base.py | 18 ++++++++++++++++-- synapse/config/_base.pyi | 9 ++++++++- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index da4398b2c2d6..a800f1e43ac6 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -485,13 +485,17 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: def reload_cache_config(config: HomeServerConfig) -> None: try: - config.reload_config_section("caches") + previous_cache_config = config.reload_config_section("caches") except ConfigError as e: logger.warning("Failed to reload cache config") for f in format_config_error(e): logger.warning(f) else: - logger.debug("New cache config: %s", config.caches.__dict__) + logger.debug( + "New cache config. Was:\n %s\nNow:\n", + previous_cache_config.__dict__, + config.caches.__dict__, + ) config.caches.resize_all_caches() diff --git a/synapse/config/_base.py b/synapse/config/_base.py index d78641ca898e..42364fc133f1 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -766,11 +766,20 @@ def generate_missing_files( ) -> None: self.invoke_all("generate_files", config_dict, config_dir_path) - def reload_config_section(self, section_name: str) -> None: + def reload_config_section(self, section_name: str) -> Config: """Reconstruct the given config section, leaving all others unchanged. + This works in three steps: + + 1. Create a new instance of the relevant `Config` subclass. + 2. Call `read_config` on that instance to parse the new config. + 3. Replace the existing config instance with the new one. + :raises ValueError: if the given `section` does not exist. :raises ConfigError: for any other problems reloading config. + + :returns: the previous config object, which no longer has a reference to this + RootConfig. """ existing_config: Optional[Config] = getattr(self, section_name, None) if existing_config is None: @@ -778,7 +787,12 @@ def reload_config_section(self, section_name: str) -> None: logger.info("Reloading config section '%s'", section_name) new_config_data = read_config_files(self.config_files) - existing_config.read_config(new_config_data) + new_config = type(existing_config)(self) + new_config.read_config(new_config_data) + setattr(self, section_name, new_config) + + existing_config.root = None + return existing_config def read_config_files(config_files: Iterable[str]) -> Dict[str, Any]: diff --git a/synapse/config/_base.pyi b/synapse/config/_base.pyi index b1b3f302d96a..71d6655fda4e 100644 --- a/synapse/config/_base.pyi +++ b/synapse/config/_base.pyi @@ -6,12 +6,14 @@ from typing import ( Iterable, Iterator, List, + Literal, MutableMapping, Optional, Tuple, Type, TypeVar, Union, + overload, ) import jinja2 @@ -162,7 +164,12 @@ class RootConfig: def generate_missing_files( self, config_dict: dict, config_dir_path: str ) -> None: ... - def reload_config_section(self, section_name: str) -> None: ... + @overload + def reload_config_section( + self, section_name: Literal["caches"] + ) -> cache.CacheConfig: ... + @overload + def reload_config_section(self, section_name: str) -> Config: ... class Config: root: RootConfig From 1118b9f974b6ad94f1731519c6fb621952b96964 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 17:06:03 +0100 Subject: [PATCH 12/17] Fix tests oops --- tests/config/test_cache.py | 8 ++++++++ tests/server.py | 1 + 2 files changed, 9 insertions(+) diff --git a/tests/config/test_cache.py b/tests/config/test_cache.py index 4bb82e810e0c..d2b3c299e354 100644 --- a/tests/config/test_cache.py +++ b/tests/config/test_cache.py @@ -38,6 +38,7 @@ def test_individual_caches_from_environ(self): "SYNAPSE_NOT_CACHE": "BLAH", } self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() self.assertEqual(dict(self.config.cache_factors), {"something_or_other": 2.0}) @@ -52,6 +53,7 @@ def test_config_overrides_environ(self): "SYNAPSE_CACHE_FACTOR_FOO": 1, } self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() self.assertEqual( dict(self.config.cache_factors), @@ -71,6 +73,7 @@ def test_individual_instantiated_before_config_load(self): config = {"caches": {"per_cache_factors": {"foo": 3}}} self.config.read_config(config) + self.config.resize_all_caches() self.assertEqual(cache.max_size, 300) @@ -82,6 +85,7 @@ def test_individual_instantiated_after_config_load(self): """ config = {"caches": {"per_cache_factors": {"foo": 2}}} self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() cache = LruCache(100) add_resizable_cache("foo", cache_resize_callback=cache.set_cache_factor) @@ -99,6 +103,7 @@ def test_global_instantiated_before_config_load(self): config = {"caches": {"global_factor": 4}} self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() self.assertEqual(cache.max_size, 400) @@ -110,6 +115,7 @@ def test_global_instantiated_after_config_load(self): """ config = {"caches": {"global_factor": 1.5}} self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() cache = LruCache(100) add_resizable_cache("foo", cache_resize_callback=cache.set_cache_factor) @@ -128,6 +134,7 @@ def test_cache_with_asterisk_in_name(self): "SYNAPSE_CACHE_FACTOR_CACHE_B": 3, } self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() cache_a = LruCache(100) add_resizable_cache("*cache_a*", cache_resize_callback=cache_a.set_cache_factor) @@ -148,6 +155,7 @@ def test_apply_cache_factor_from_config(self): config = {"caches": {"event_cache_size": "10k"}} self.config.read_config(config, config_dir_path="", data_dir_path="") + self.config.resize_all_caches() cache = LruCache( max_size=self.config.event_cache_size, diff --git a/tests/server.py b/tests/server.py index 8f30e250c83c..a22aae41b957 100644 --- a/tests/server.py +++ b/tests/server.py @@ -736,6 +736,7 @@ def setup_test_homeserver( if config is None: config = default_config(name, parse=True) + config.caches.resize_all_caches() config.ldap_enabled = False if "clock" not in kwargs: From f6e1394037525e888e8c030db9589cefa00275bc Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 10 May 2022 18:02:35 +0100 Subject: [PATCH 13/17] Document the ability to reload cache config --- docs/sample_config.yaml | 2 ++ .../configuration/config_documentation.md | 18 ++++++++++++++++++ synapse/config/cache.py | 2 ++ 3 files changed, 22 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index a803b8261dcd..10871e8073fa 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -726,6 +726,8 @@ retention: ## Caching ## # Caching can be configured through the following options. +# This configuration can be reloaded while the application +# is running, by sending a SIGHUP signal to the Synapse process. # # A cache 'factor' is a multiplier that can be applied to each of # Synapse's caches in order to increase or decrease the maximum diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 21dad0ac41e2..2411b7bbfead 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1130,6 +1130,24 @@ caches: expire_caches: false sync_response_cache_duration: 2m ``` + +### Reloading cache config + +The cache config may be reloaded at any time by sending a +[`SIGHUP`](https://en.wikipedia.org/wiki/SIGHUP) signal to Synapse using e.g. + +```commandline +kill -HUP [PID_OF_SYNAPSE_PROCESS] +``` + +If you are running multiple workers, you must send this signal to each worker +process individually; otherwise different workers will have different caching +config. + +If you're using the [example systemd service](https://github.com/matrix-org/synapse/blob/develop/contrib/systemd/matrix-synapse.service) +file in Synapse's `contrib` directory, you can send a `SIGHUP` signal by using +`systemctl reload matrix-synapse`. + --- ## Database ## Config options related to database settings. diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 350227ab8421..80d1bbd855ea 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -118,6 +118,8 @@ def generate_config_section(self, **kwargs: Any) -> str: ## Caching ## # Caching can be configured through the following options. + # This configuration can be reloaded while the application + # is running, by sending a SIGHUP signal to the Synapse process. # # A cache 'factor' is a multiplier that can be applied to each of # Synapse's caches in order to increase or decrease the maximum From 55963acf102f544479fb2d5c87d7c47f8648e576 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 11 May 2022 12:21:48 +0100 Subject: [PATCH 14/17] Master process needs to apply cache config too. --- synapse/app/_base.py | 3 +++ synapse/app/generic_worker.py | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index a800f1e43ac6..75956a3b2a5b 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -430,6 +430,9 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: register_sighup(refresh_certificate, hs) register_sighup(reload_cache_config, hs.config) + # Apply the cache config. + hs.config.caches.resize_all_caches() + # Load the certificate from disk. refresh_certificate(hs) diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index 1a0a7406cc30..07dddc0b1326 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -459,7 +459,6 @@ def start(config_options: List[str]) -> None: synapse.events.USE_FROZEN_DICTS = config.server.use_frozen_dicts synapse.util.caches.TRACK_MEMORY_USAGE = config.caches.track_memory_usage - config.caches.resize_all_caches() if config.server.gc_seconds: synapse.metrics.MIN_TIME_BETWEEN_GCS = config.server.gc_seconds From f199d82be29c42aa8986f454b209dda9b8bfcbe6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 11 May 2022 12:36:36 +0100 Subject: [PATCH 15/17] Update TRACK_MEMORY_USAGE on cache config reload --- synapse/app/_base.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 75956a3b2a5b..8753553387f3 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -48,6 +48,7 @@ from twisted.protocols.tls import TLSMemoryBIOFactory from twisted.python.threadpool import ThreadPool +import synapse.util.caches from synapse.api.constants import MAX_PDU_SIZE from synapse.app import check_bind_error from synapse.app.phone_stats_home import start_phone_stats_home @@ -499,6 +500,7 @@ def reload_cache_config(config: HomeServerConfig) -> None: previous_cache_config.__dict__, config.caches.__dict__, ) + synapse.util.caches.TRACK_MEMORY_USAGE = config.caches.track_memory_usage config.caches.resize_all_caches() From 412414d4be95283a0b86ab5891d760bec87b8fd1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 11 May 2022 13:06:13 +0100 Subject: [PATCH 16/17] Overly comprehensive docstring --- synapse/app/_base.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 8753553387f3..21e9847ee10e 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -488,6 +488,25 @@ def run_sighup(*args: Any, **kwargs: Any) -> None: def reload_cache_config(config: HomeServerConfig) -> None: + """Reload cache config from disk and immediately apply it.resize caches accordingly. + + If the config is invalid, a `ConfigError` is logged and no changes are made. + + Otherwise, this: + - replaces the `caches` section on the given `config` object, + - resizes all caches according to the new cache factors, and + - updates synapse.util.caches.TRACK_MEMORY_USAGE. + + Note that the following cache config keys are read, but not applied: + - event_cache_size: used to set a max_size and _original_max_size on + EventsWorkerStore._get_event_cache when it is created. We'd have to update + the _original_max_size (and maybe + - sync_response_cache_duration: would have to update the timeout_sec attribute on + HomeServer -> SyncHandler -> ResponseCache. + + Also, the HTTPConnectionPool in SimpleHTTPClient sets its maxPersistentPerHost + parameter based on the global_factor. This won't be applied on a config reload. + """ try: previous_cache_config = config.reload_config_section("caches") except ConfigError as e: From a7418c06fd41d356fe74bb559acd7294435ce4ad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 11 May 2022 14:13:10 +0100 Subject: [PATCH 17/17] Don't update TRACK_MEMORY_USAGE; update docs --- docs/sample_config.yaml | 8 ++++++-- docs/usage/configuration/config_documentation.md | 9 ++++----- synapse/app/_base.py | 3 ++- synapse/config/cache.py | 8 ++++++-- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 10871e8073fa..e7b57f5a0bdf 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -726,12 +726,16 @@ retention: ## Caching ## # Caching can be configured through the following options. -# This configuration can be reloaded while the application -# is running, by sending a SIGHUP signal to the Synapse process. # # A cache 'factor' is a multiplier that can be applied to each of # Synapse's caches in order to increase or decrease the maximum # number of entries that can be stored. +# +# The configuration for cache factors (caches.global_factor and +# caches.per_cache_factors) can be reloaded while the application is running, +# by sending a SIGHUP signal to the Synapse process. Changes to other parts of +# the caching config will NOT be applied after a SIGHUP is received; a restart +# is necessary. # The number of events to cache in memory. Not affected by # caches.global_factor. diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 2411b7bbfead..f292b94fb0cd 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1131,18 +1131,17 @@ caches: sync_response_cache_duration: 2m ``` -### Reloading cache config +### Reloading cache factors -The cache config may be reloaded at any time by sending a +The cache factors (i.e. `caches.global_factor` and `caches.per_cache_factors`) may be reloaded at any time by sending a [`SIGHUP`](https://en.wikipedia.org/wiki/SIGHUP) signal to Synapse using e.g. ```commandline kill -HUP [PID_OF_SYNAPSE_PROCESS] ``` -If you are running multiple workers, you must send this signal to each worker -process individually; otherwise different workers will have different caching -config. +If you are running multiple workers, you must individually update the worker +config file and send this signal to each worker process. If you're using the [example systemd service](https://github.com/matrix-org/synapse/blob/develop/contrib/systemd/matrix-synapse.service) file in Synapse's `contrib` directory, you can send a `SIGHUP` signal by using diff --git a/synapse/app/_base.py b/synapse/app/_base.py index 21e9847ee10e..5946da97b266 100644 --- a/synapse/app/_base.py +++ b/synapse/app/_base.py @@ -495,7 +495,6 @@ def reload_cache_config(config: HomeServerConfig) -> None: Otherwise, this: - replaces the `caches` section on the given `config` object, - resizes all caches according to the new cache factors, and - - updates synapse.util.caches.TRACK_MEMORY_USAGE. Note that the following cache config keys are read, but not applied: - event_cache_size: used to set a max_size and _original_max_size on @@ -503,6 +502,8 @@ def reload_cache_config(config: HomeServerConfig) -> None: the _original_max_size (and maybe - sync_response_cache_duration: would have to update the timeout_sec attribute on HomeServer -> SyncHandler -> ResponseCache. + - track_memory_usage. This affects synapse.util.caches.TRACK_MEMORY_USAGE which + influences Synapse's self-reported metrics. Also, the HTTPConnectionPool in SimpleHTTPClient sets its maxPersistentPerHost parameter based on the global_factor. This won't be applied on a config reload. diff --git a/synapse/config/cache.py b/synapse/config/cache.py index 80d1bbd855ea..58b2fe55193c 100644 --- a/synapse/config/cache.py +++ b/synapse/config/cache.py @@ -118,12 +118,16 @@ def generate_config_section(self, **kwargs: Any) -> str: ## Caching ## # Caching can be configured through the following options. - # This configuration can be reloaded while the application - # is running, by sending a SIGHUP signal to the Synapse process. # # A cache 'factor' is a multiplier that can be applied to each of # Synapse's caches in order to increase or decrease the maximum # number of entries that can be stored. + # + # The configuration for cache factors (caches.global_factor and + # caches.per_cache_factors) can be reloaded while the application is running, + # by sending a SIGHUP signal to the Synapse process. Changes to other parts of + # the caching config will NOT be applied after a SIGHUP is received; a restart + # is necessary. # The number of events to cache in memory. Not affected by # caches.global_factor.