Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Reload cache factors from disk on SIGHUP #12673

Merged
merged 17 commits into from
May 11, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12673.feature
Original file line number Diff line number Diff line change
@@ -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.
19 changes: 19 additions & 0 deletions synapse/app/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring pwease?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# 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"""

Expand Down
36 changes: 2 additions & 34 deletions synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
67 changes: 60 additions & 7 deletions synapse/config/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@

import argparse
import errno
import logging
import os
from collections import OrderedDict
from hashlib import sha256
from textwrap import dedent
from typing import (
Any,
ClassVar,
Collection,
Dict,
Iterable,
Iterator,
List,
MutableMapping,
Optional,
Expand All @@ -40,6 +44,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
Expand All @@ -55,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 = """\
Expand Down Expand Up @@ -119,7 +157,7 @@ class Config:
defined in subclasses.
"""

section: str
section: ClassVar[str]

def __init__(self, root_config: "RootConfig" = None):
self.root = root_config
Expand Down Expand Up @@ -309,9 +347,12 @@ class RootConfig:
class, lower-cased and with "Config" removed.
"""

config_classes = []
config_classes: List[Type[Config]] = []

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]

def __init__(self):
for config_class in self.config_classes:
if config_class.section is None:
raise ValueError("%r requires a section name" % (config_class,))
Expand Down Expand Up @@ -512,12 +553,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.")

Expand Down Expand Up @@ -627,7 +666,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:
Expand Down Expand Up @@ -727,6 +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:
"""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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we error half way through reading the config? I.e. if someone has put an invalid value in there? Do we end up in a messed up state where we've reloaded some of the config?

I guess ideally we should be reading the config into a new config class and then replacing the existing config with that once we've successfully read it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we end up in a messed up state where we've reloaded some of the config?

We will, yes. I don't think that's the end of the world, because you can correct and re-reload the config. But it would be nice for this to be an atomic operation. I'll do this shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note however that the CacheConfig in particular is naughty. When it reads in config it adjusts global state in its enclosing module. And it automatically re-evaluates cache sizes. I'll have to change this too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's annoying :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted four commits which have a go at this. Could use somebody to sanity check that it works though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use somebody to sanity check that it works though.

The reloading seems to work fine. Not sure how to check that the caches themselves have the appropriate size from the diffs?



def read_config_files(config_files: Iterable[str]) -> Dict[str, Any]:
"""Read the config files into a dict
Expand Down
8 changes: 7 additions & 1 deletion synapse/config/_base.pyi
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import argparse
from typing import (
Any,
Collection,
Dict,
Iterable,
Iterator,
List,
MutableMapping,
Optional,
Expand Down Expand Up @@ -64,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
Expand Down Expand Up @@ -117,7 +121,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]: ...
Expand Down Expand Up @@ -157,6 +162,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
Expand Down
8 changes: 3 additions & 5 deletions synapse/config/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.")

Expand Down