mirror of
https://github.com/frappe/gunicorn.git
synced 2026-07-07 21:21:30 +08:00
fix(companion): Apply dead config settings and validate specs
Three companion settings were documented and configurable but never had any effect. companion_restart_delay was ignored because CompanionProcess hardcoded a 5s delay; it is now read from config and kept out of config_hash, since it does not affect the spawned process and so must not trigger a restart on reread. companion_config_file was never read; the manager now loads its companion settings from that dedicated file when set, instead of always reading the main gunicorn config. companion_manager_stop_timeout was unused, so shutdown waited only graceful_timeout before SIGKILLing the manager and cut short long-draining companions; stop now waits the larger of graceful_timeout and the manager stop timeout, derived from the slowest companion stop_timeout plus the buffer when not set explicitly. Worker specs now reject unknown keys so a typo fails loudly instead of silently falling back to a default. Also correct the spawn_companion_manager docstring, drop its unused return value, and fix the README config-file description. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
79c8e58429
commit
642387dd0e
@ -395,7 +395,9 @@ class Arbiter:
|
|||||||
sig = signal.SIGTERM
|
sig = signal.SIGTERM
|
||||||
if not graceful:
|
if not graceful:
|
||||||
sig = signal.SIGQUIT
|
sig = signal.SIGQUIT
|
||||||
limit = time.time() + self.cfg.graceful_timeout
|
# The manager may need longer than a worker to drain its companions.
|
||||||
|
limit = time.time() + max(
|
||||||
|
self.cfg.graceful_timeout, self.companion_manager_stop_timeout())
|
||||||
# instruct the workers and the companion manager to exit
|
# instruct the workers and the companion manager to exit
|
||||||
self.kill_workers(sig)
|
self.kill_workers(sig)
|
||||||
self.stop_companion_manager(sig)
|
self.stop_companion_manager(sig)
|
||||||
@ -675,9 +677,10 @@ class Arbiter:
|
|||||||
def spawn_companion_manager(self):
|
def spawn_companion_manager(self):
|
||||||
"""Fork the companion manager process.
|
"""Fork the companion manager process.
|
||||||
|
|
||||||
The parent records the manager pid and returns. The child builds the
|
The configs are built in the parent before the fork; the parent then
|
||||||
configs, runs the manager's supervision loop, and exits when the loop
|
records the manager pid and returns, while the child runs the manager's
|
||||||
returns. The manager forks the individual companions itself.
|
supervision loop and exits when the loop returns. The manager forks the
|
||||||
|
individual companions itself.
|
||||||
"""
|
"""
|
||||||
configs = build_companion_configs(self.cfg)
|
configs = build_companion_configs(self.cfg)
|
||||||
if not configs:
|
if not configs:
|
||||||
@ -695,7 +698,7 @@ class Arbiter:
|
|||||||
if pid != 0:
|
if pid != 0:
|
||||||
self.companion_manager_pid = pid
|
self.companion_manager_pid = pid
|
||||||
self.log.info("Companion manager started (pid:%s)", pid)
|
self.log.info("Companion manager started (pid:%s)", pid)
|
||||||
return pid
|
return
|
||||||
|
|
||||||
# Process Child
|
# Process Child
|
||||||
try:
|
try:
|
||||||
@ -759,6 +762,19 @@ class Arbiter:
|
|||||||
return
|
return
|
||||||
raise
|
raise
|
||||||
|
|
||||||
|
def companion_manager_stop_timeout(self):
|
||||||
|
"""Seconds to wait for the companion manager during shutdown: the
|
||||||
|
explicit setting, else the slowest companion stop_timeout plus the
|
||||||
|
shutdown buffer, else 0 when no companions are configured.
|
||||||
|
"""
|
||||||
|
if self.cfg.companion_manager_stop_timeout is not None:
|
||||||
|
return self.cfg.companion_manager_stop_timeout
|
||||||
|
configs = build_companion_configs(self.cfg)
|
||||||
|
if not configs:
|
||||||
|
return 0
|
||||||
|
slowest = max(config.stop_timeout for config in configs)
|
||||||
|
return slowest + self.cfg.companion_manager_shutdown_buffer
|
||||||
|
|
||||||
def kill_workers(self, sig):
|
def kill_workers(self, sig):
|
||||||
"""\
|
"""\
|
||||||
Kill all workers with the signal `sig`
|
Kill all workers with the signal `sig`
|
||||||
|
|||||||
@ -53,9 +53,7 @@ STOPPED ──start──▶ STARTING ──(survives startsecs)──▶ RUNNIN
|
|||||||
|
|
||||||
## Configuration
|
## Configuration
|
||||||
|
|
||||||
Companions live in the normal Gunicorn config — a Python file you pass with
|
Companions live in the normal Gunicorn config — a Python file you pass with `-c`. To reload companions independently of the web config, put them in a dedicated file and point `companion_config_file` (or `--companion-config`) at it; the manager then reads its companion settings from there instead.
|
||||||
`-c`. There is no separate companion config file or CLI flag; if you already run
|
|
||||||
Gunicorn with a config, add the companion settings to it.
|
|
||||||
|
|
||||||
Save a `gunicorn.conf.py`:
|
Save a `gunicorn.conf.py`:
|
||||||
|
|
||||||
|
|||||||
@ -8,7 +8,7 @@ import json
|
|||||||
|
|
||||||
# Maps each optional companion field to the global setting build_companion_configs
|
# Maps each optional companion field to the global setting build_companion_configs
|
||||||
# reads when a spec omits it. ``name`` and ``target`` are required per spec and
|
# reads when a spec omits it. ``name`` and ``target`` are required per spec and
|
||||||
# have no global default, so they are filled directly instead of through here.
|
# have no global default. ``restart_delay`` is global-only, so it is absent here.
|
||||||
FIELD_DEFAULTS = {
|
FIELD_DEFAULTS = {
|
||||||
"cwd": "companion_cwd",
|
"cwd": "companion_cwd",
|
||||||
"env": "companion_env",
|
"env": "companion_env",
|
||||||
@ -41,6 +41,7 @@ class CompanionConfig:
|
|||||||
stdout=None,
|
stdout=None,
|
||||||
stderr=None,
|
stderr=None,
|
||||||
startsecs=1,
|
startsecs=1,
|
||||||
|
restart_delay=5,
|
||||||
):
|
):
|
||||||
self.name = name
|
self.name = name
|
||||||
self.target = target
|
self.target = target
|
||||||
@ -52,6 +53,7 @@ class CompanionConfig:
|
|||||||
self.stdout = stdout
|
self.stdout = stdout
|
||||||
self.stderr = stderr
|
self.stderr = stderr
|
||||||
self.startsecs = startsecs
|
self.startsecs = startsecs
|
||||||
|
self.restart_delay = restart_delay
|
||||||
|
|
||||||
def to_dict(self):
|
def to_dict(self):
|
||||||
return {
|
return {
|
||||||
@ -88,19 +90,47 @@ class CompanionConfig:
|
|||||||
return "<CompanionConfig %s>" % self.name
|
return "<CompanionConfig %s>" % self.name
|
||||||
|
|
||||||
|
|
||||||
def build_companion_configs(cfg):
|
ALLOWED_SPEC_KEYS = {"name", "target"} | set(FIELD_DEFAULTS)
|
||||||
"""Build a CompanionConfig list from ``cfg.companion_workers``.
|
|
||||||
|
|
||||||
A spec missing ``name`` or ``target`` is rejected, since the manager has
|
|
||||||
nothing to supervise without both.
|
def _load_companion_settings(cfg):
|
||||||
|
"""Return the ``companion_*`` settings from ``companion_config_file``, or
|
||||||
|
``{}`` when no dedicated file is configured."""
|
||||||
|
path = getattr(cfg, "companion_config_file", None)
|
||||||
|
if not path:
|
||||||
|
return {}
|
||||||
|
namespace = {}
|
||||||
|
with open(path) as config_file:
|
||||||
|
exec(compile(config_file.read(), path, "exec"), namespace)
|
||||||
|
return {name: value for name, value in namespace.items()
|
||||||
|
if name.startswith("companion_")}
|
||||||
|
|
||||||
|
|
||||||
|
def build_companion_configs(cfg):
|
||||||
|
"""Build a CompanionConfig list from the companion settings.
|
||||||
|
|
||||||
|
Settings come from ``companion_config_file`` when set, otherwise ``cfg``. A
|
||||||
|
spec is rejected if it is missing ``name``/``target`` or carries an unknown
|
||||||
|
key.
|
||||||
"""
|
"""
|
||||||
|
overrides = _load_companion_settings(cfg)
|
||||||
|
|
||||||
|
def setting(name):
|
||||||
|
return overrides.get(name, getattr(cfg, name))
|
||||||
|
|
||||||
configs = []
|
configs = []
|
||||||
for spec in cfg.companion_workers:
|
for spec in setting("companion_workers"):
|
||||||
if "name" not in spec or "target" not in spec:
|
if "name" not in spec or "target" not in spec:
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
"each companion worker needs 'name' and 'target': %s" % spec)
|
"each companion worker needs 'name' and 'target': %s" % spec)
|
||||||
fields = {field: spec.get(field, getattr(cfg, setting))
|
unknown = set(spec) - ALLOWED_SPEC_KEYS
|
||||||
for field, setting in FIELD_DEFAULTS.items()}
|
if unknown:
|
||||||
configs.append(
|
raise ValueError(
|
||||||
CompanionConfig(name=spec["name"], target=spec["target"], **fields))
|
"unknown companion worker key(s) %s in %s"
|
||||||
|
% (sorted(unknown), spec))
|
||||||
|
fields = {field: spec.get(field, setting(global_setting))
|
||||||
|
for field, global_setting in FIELD_DEFAULTS.items()}
|
||||||
|
configs.append(CompanionConfig(
|
||||||
|
name=spec["name"], target=spec["target"],
|
||||||
|
restart_delay=setting("companion_restart_delay"), **fields))
|
||||||
return configs
|
return configs
|
||||||
|
|||||||
@ -36,7 +36,7 @@ class CompanionProcess:
|
|||||||
self.config = config
|
self.config = config
|
||||||
self.state = State.STOPPED
|
self.state = State.STOPPED
|
||||||
self.pid = None
|
self.pid = None
|
||||||
self.restart_delay = 5
|
self.restart_delay = config.restart_delay
|
||||||
|
|
||||||
self.started_at = None
|
self.started_at = None
|
||||||
self.exited_at = None
|
self.exited_at = None
|
||||||
|
|||||||
@ -282,6 +282,27 @@ def test_arbiter_stop_signals_companion_manager(close_sockets):
|
|||||||
assert signal.SIGKILL in signals
|
assert signal.SIGKILL in signals
|
||||||
|
|
||||||
|
|
||||||
|
def test_companion_manager_stop_timeout_uses_explicit():
|
||||||
|
arbiter = gunicorn.arbiter.Arbiter(DummyApplication())
|
||||||
|
arbiter.cfg.set("companion_manager_stop_timeout", 120)
|
||||||
|
assert arbiter.companion_manager_stop_timeout() == 120
|
||||||
|
|
||||||
|
|
||||||
|
def test_companion_manager_stop_timeout_derives_from_slowest():
|
||||||
|
arbiter = gunicorn.arbiter.Arbiter(DummyApplication())
|
||||||
|
arbiter.cfg.set("companion_workers", [
|
||||||
|
{"name": "rq", "target": "pkg:run", "stop_timeout": 300},
|
||||||
|
{"name": "scheduler", "target": "pkg:sched", "stop_timeout": 30},
|
||||||
|
])
|
||||||
|
arbiter.cfg.set("companion_manager_shutdown_buffer", 10)
|
||||||
|
assert arbiter.companion_manager_stop_timeout() == 310
|
||||||
|
|
||||||
|
|
||||||
|
def test_companion_manager_stop_timeout_zero_without_companions():
|
||||||
|
arbiter = gunicorn.arbiter.Arbiter(DummyApplication())
|
||||||
|
assert arbiter.companion_manager_stop_timeout() == 0
|
||||||
|
|
||||||
|
|
||||||
class PreloadedAppWithEnvSettings(DummyApplication):
|
class PreloadedAppWithEnvSettings(DummyApplication):
|
||||||
"""
|
"""
|
||||||
Simple application that makes use of the 'preload' feature to
|
Simple application that makes use of the 'preload' feature to
|
||||||
|
|||||||
@ -36,6 +36,31 @@ def test_build_per_spec_overrides_global():
|
|||||||
assert config.stop_signal == "SIGTERM"
|
assert config.stop_signal == "SIGTERM"
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_applies_global_restart_delay():
|
||||||
|
cfg = make_config(
|
||||||
|
[{"name": "rq", "target": "pkg:run"}],
|
||||||
|
companion_restart_delay=42)
|
||||||
|
config, = build_companion_configs(cfg)
|
||||||
|
assert config.restart_delay == 42
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_rejects_unknown_spec_key():
|
||||||
|
with pytest.raises(ValueError):
|
||||||
|
build_companion_configs(
|
||||||
|
make_config([{"name": "rq", "target": "pkg:run", "stop_sigal": "X"}]))
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_reads_companion_config_file(tmp_path):
|
||||||
|
config_file = tmp_path / "companion.conf.py"
|
||||||
|
config_file.write_text(
|
||||||
|
'companion_workers = [{"name": "scheduler", "target": "app:run"}]\n'
|
||||||
|
'companion_stop_signal = "SIGINT"\n')
|
||||||
|
cfg = make_config([], companion_config_file=str(config_file))
|
||||||
|
config, = build_companion_configs(cfg)
|
||||||
|
assert config.name == "scheduler"
|
||||||
|
assert config.stop_signal == "SIGINT"
|
||||||
|
|
||||||
|
|
||||||
def test_build_empty_when_none_configured():
|
def test_build_empty_when_none_configured():
|
||||||
assert build_companion_configs(make_config([])) == []
|
assert build_companion_configs(make_config([])) == []
|
||||||
|
|
||||||
@ -71,6 +96,14 @@ def test_config_hash_stable_and_field_sensitive():
|
|||||||
assert base.config_hash != changed.config_hash
|
assert base.config_hash != changed.config_hash
|
||||||
|
|
||||||
|
|
||||||
|
def test_config_hash_ignores_restart_delay():
|
||||||
|
# restart_delay does not affect the spawned process, so changing it must not
|
||||||
|
# change the hash (and so must not trigger a restart on reread).
|
||||||
|
base = CompanionConfig(name="rq", target="pkg:run", restart_delay=5)
|
||||||
|
changed = CompanionConfig(name="rq", target="pkg:run", restart_delay=30)
|
||||||
|
assert base.config_hash == changed.config_hash
|
||||||
|
|
||||||
|
|
||||||
def test_config_hash_keys_callable_target_by_qualified_name():
|
def test_config_hash_keys_callable_target_by_qualified_name():
|
||||||
def run():
|
def run():
|
||||||
pass
|
pass
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user