From 642df0403b294ab5f6b87c87f80bd73ad06f3794 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 10:47:40 -0400 Subject: [PATCH 01/27] WIP systemd process manager implementation --- gravity/process_manager/__init__.py | 22 ++- gravity/process_manager/supervisor.py | 20 --- gravity/process_manager/systemd.py | 244 ++++++++++++++++++++++++++ 3 files changed, 265 insertions(+), 21 deletions(-) create mode 100644 gravity/process_manager/systemd.py diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 7e99bd8..303eec2 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -10,7 +10,7 @@ from functools import partial, wraps from gravity.config_manager import ConfigManager -from gravity.io import debug, exception, warn +from gravity.io import debug, exception, info, warn from gravity.state import VALID_SERVICE_NAMES from gravity.util import which @@ -81,6 +81,26 @@ def _service_environment(self, service, attribs): environment.update(attribs.get(environment_from, {}).get("environment", {})) return environment + def _file_needs_update(self, path, contents): + """Update if contents differ""" + if os.path.exists(path): + # check first whether there are changes + with open(path) as fh: + existing_contents = fh.read() + if existing_contents == contents: + return False + return True + + def _update_file(self, path, contents, name, file_type): + exists = os.path.exists(path) + if (exists and self._file_needs_update(path, contents)) or not exists: + verb = "Updating" if exists else "Adding" + info("%s %s %s", verb, file_type, name) + with open(path, "w") as out: + out.write(contents) + else: + debug("No changes to existing config for %s %s at %s", file_type, name, path) + def follow(self, configs=None, service_names=None, quiet=False): # supervisor has a built-in tail command but it only works on a single log file. `galaxyctl supervisorctl tail # ...` can be used if desired, though diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 7b4e005..7cd0fc2 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -324,26 +324,6 @@ def __update_service(self, config_file, config, attribs, service, instance_conf_ return conf - def _file_needs_update(self, path, contents): - """Update if contents differ""" - if os.path.exists(path): - # check first whether there are changes - with open(path) as fh: - existing_contents = fh.read() - if existing_contents == contents: - return False - return True - - def _update_file(self, path, contents, name, file_type): - exists = os.path.exists(path) - if (exists and self._file_needs_update(path, contents)) or not exists: - verb = "Updating" if exists else "Adding" - info("%s %s %s", verb, file_type, name) - with open(path, "w") as out: - out.write(contents) - else: - debug("No changes to existing config for %s %s at %s", file_type, name, path) - def _process_config(self, config): """Perform necessary supervisor config updates as per current Galaxy/Gravity configuration. diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py new file mode 100644 index 0000000..3224904 --- /dev/null +++ b/gravity/process_manager/systemd.py @@ -0,0 +1,244 @@ +""" +""" +import errno +import os +import shlex +import subprocess + +from gravity.io import debug, info +from gravity.process_manager import BaseProcessManager +from gravity.settings import ProcessManager + +SYSTEMD_SERVICE_TEMPLATES = {} +SYSTEMD_SERVICE_TEMPLATES["gunicorn"] = """; +; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN +; + +[Unit] +Description=Galaxy {program_name} +After=network.target +After=time-sync.target + +[Service] +UMask={galaxy_umask} +Type=simple +{systemd_user_group} +WorkingDirectory={galaxy_root} +TimeoutStartSec=15 +ExecStart={command} +#ExecReload= +#ExecStop= +{environment} +#MemoryLimit= +Restart=always + +MemoryAccounting=yes +CPUAccounting=yes +BlockIOAccounting=yes + +[Install] +WantedBy=multi-user.target +""" + +SYSTEMD_SERVICE_TEMPLATES["celery"] = SYSTEMD_SERVICE_TEMPLATES["gunicorn"] +SYSTEMD_SERVICE_TEMPLATES["celery-beat"] = SYSTEMD_SERVICE_TEMPLATES["gunicorn"] + + +# FIXME: need to document and enforce that gravity_config.virtualenv is required in systemd mode +class SystemdProcessManager(BaseProcessManager): + + name = ProcessManager.systemd + + def __init__(self, state_dir=None, start_daemon=True, foreground=False, **kwargs): + super(SystemdProcessManager, self).__init__(state_dir=state_dir, **kwargs) + self.user_mode = os.geteuid() != 0 + + @property + def __systemd_unit_dir(self): + unit_path = "/etc/systemd/system" if not self.user_mode else os.path.expanduser("~/.config/systemd/user") + return unit_path + + @property + def __use_instance(self): + #return not self.config_manager.single_instance + return False + + def __systemctl(self, *args, **kwargs): + args = list(args) + if self.user_mode: + args = ["--user"] + args + try: + debug("Calling systemctl with args: %s", args) + subprocess.check_call(["systemctl"] + args) + except: + raise + + def terminate(self): + """ """ + debug("TERMINATE") + + def __unit_name(self, instance_name, service): + unit_name = f"{service['config_type']}-" + if self.__use_instance: + unit_name += f"{instance_name}-" + unit_name += f"{service['service_name']}.service" + return unit_name + + def __update_service(self, config_file, config, attribs, service, instance_name): + unit_name = self.__unit_name(instance_name, service) + + # FIXME: refactor + # used by the "standalone" service type + attach_to_pool_opt = "" + server_pools = service.get("server_pools") + if server_pools: + _attach_to_pool_opt = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) + # Insert a single leading space + attach_to_pool_opt = f" {_attach_to_pool_opt}" + + virtualenv_dir = attribs.get("virtualenv") + virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" + gunicorn_options = attribs["gunicorn"].copy() + gunicorn_options["preload"] = "--preload" if gunicorn_options["preload"] else "" + + format_vars = { + #"log_dir": attribs["log_dir"], + #"log_file": self._service_log_file(attribs["log_dir"], program_name), + "program_name": service["service_name"], + "systemd_user_group": "", + "config_type": service["config_type"], + "server_name": service["service_name"], + "attach_to_pool_opt": attach_to_pool_opt, + "gunicorn": gunicorn_options, + "celery": attribs["celery"], + "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], + "tusd": attribs["tusd"], + "gx_it_proxy": attribs["gx_it_proxy"], + "galaxy_umask": service.get("umask", "022"), + "galaxy_conf": config_file, + "galaxy_root": config["galaxy_root"], + "virtualenv_bin": virtualenv_bin, + "state_dir": self.state_dir, + } + format_vars["command"] = service.command_template.format(**format_vars) + if not format_vars["command"].startswith("/"): + # FIXME: bit of a hack + format_vars["command"] = f"{virtualenv_bin}/{format_vars['command']}" + if not self.user_mode: + format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" + if "galaxy_group" in attribs: + format_vars["systemd_user_group"] += f"\nGroup={attribs['galaxy_group']}" + conf = os.path.join(self.__systemd_unit_dir, unit_name) + + template = SYSTEMD_SERVICE_TEMPLATES.get(service["service_type"]) + if not template: + raise Exception(f"Unknown service type: {service['service_type']}") + + environment = self._service_environment(service, attribs) + if virtualenv_bin and service.add_virtualenv_to_path: + # FIXME: what should we use for a default here? + path = environment.get("PATH", "%(ENV_PATH)s") + environment["PATH"] = ":".join([virtualenv_bin, path]) + format_vars["environment"] = "\n".join("Environment={}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) + + contents = template.format(**format_vars) + service_name = self._service_program_name(instance_name, service) + self._update_file(conf, contents, service_name, "service") + + return conf + + def _process_config(self, config, **kwargs): + """ """ + instance_name = config["instance_name"] + attribs = config["attribs"] + config_file = config.__file__ + intended_configs = set() + present_configs = set() + + try: + os.makedirs(self.__systemd_unit_dir) + except OSError as exc: + if exc.errno != errno.EEXIST: + raise + + # FIXME: none of this works for instances + for service in config["services"]: + intended_configs.add(self.__update_service(config_file, config, attribs, service, instance_name)) + + # FIXME: should use config_type, but that's per-service + _present_configs = filter(lambda f: f.startswith("galaxy-"), os.listdir(self.__systemd_unit_dir)) + present_configs.update([os.path.join(self.__systemd_unit_dir, f) for f in _present_configs]) + + for file in (present_configs - intended_configs): + service_name = os.path.basename(os.path.splitext(file)[0]) + info(f"Ensuring service is stopped: {service_name}") + self.__systemctl("stop", service_name) + info("Removing service config %s", file) + os.unlink(file) + + def __unit_names(self, configs, service_names): + unit_names = [] + for config in configs: + services = config.services + if service_names: + services = [s for s in config.services if s["service_name"] in service_names] + unit_names.extend([self.__unit_name(config.instance_name, s) for s in services]) + return unit_names + + def start(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("start", *unit_names) + + def stop(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("stop", *unit_names) + + def restart(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("restart", *unit_names) + + def reload(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("reload", *unit_names) + + def graceful(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("reload", *unit_names) + + def status(self, configs=None, service_names=None): + """ """ + unit_names = self.__unit_names(configs, service_names) + self.__systemctl("status", "--lines=0", *unit_names) + + def update(self, configs=None, force=False, **kwargs): + """ """ + for config in configs: + process_manager = config["process_manager"] + if process_manager == self.name: + self._process_config(config) + else: + pass + """ + # FIXME: refactor + instance_name = config["instance_name"] + instance_conf_dir = join(self.supervisord_conf_dir, f"{instance_name}.d") + group_file = join(self.supervisord_conf_dir, f"group_{instance_name}.conf") + if os.path.exists(instance_conf_dir): + shutil.rmtree(instance_conf_dir) + if os.path.exists(group_file): + os.unlink(group_file) + """ + self.__systemctl("daemon-reload") + + + def shutdown(self): + """ """ + debug(f"SHUTDOWN") + + def pm(self): + """ """ From 9d5d0a224c413401ddc6ed6db2df76d2138ebfa8 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 15:18:16 -0400 Subject: [PATCH 02/27] Don't consider configs controlled by other PMs to be valid supervisor configs when removing "invalid" configs (e.g. when changing a config's PM from supervisor to systemd) --- gravity/process_manager/supervisor.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 7cd0fc2..08f377f 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -360,8 +360,10 @@ def _process_config(self, config): info("Removing service config %s", file) os.unlink(file) - def _remove_invalid_configs(self): - valid_names = [c.instance_name for c in self.config_manager.get_registered_configs()] + def _remove_invalid_configs(self, valid_configs=None): + if not valid_configs: + valid_configs = [c for c in self.config_manager.get_registered_configs() if c.process_manager == self.name] + valid_names = [c.instance_name for c in valid_configs] valid_instance_dirs = [f"{name}.d" for name in valid_names] valid_group_confs = [] if self.use_group: @@ -440,12 +442,14 @@ def shutdown(self): def update(self, configs=None, force=False, **kwargs): """Add newly defined servers, remove any that are no longer present""" - if force: + if force and os.listdir(self.supervisord_conf_dir): + info(f"Removing supervisord conf dir due to --force option: {self.supervisord_conf_dir}") shutil.rmtree(self.supervisord_conf_dir) os.makedirs(self.supervisord_conf_dir) + elif not force: + self._remove_invalid_configs(valid_configs=configs) for config in configs: self._process_config(config) - self._remove_invalid_configs() # only need to update if supervisord is running, otherwise changes will be picked up at next start if self.__supervisord_is_running(): self.supervisorctl("update") From 667e42794f7b90e341d5a5c123e158376fef8d09 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 15:19:46 -0400 Subject: [PATCH 03/27] Default to /var/lib/gravity as the state dir if running as root --- gravity/config_manager.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 547322a..8f69a3e 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -27,6 +27,7 @@ DEFAULT_JOB_CONFIG_FILE = "config/job_conf.xml" DEFAULT_STATE_DIR = join("~", ".config", "galaxy-gravity") +DEFAULT_ROOT_STATE_DIR = "/var/lib/gravity" if "XDG_CONFIG_HOME" in os.environ: DEFAULT_STATE_DIR = join(os.environ["XDG_CONFIG_HOME"], "galaxy-gravity") @@ -42,7 +43,10 @@ class ConfigManager(object): def __init__(self, state_dir=None, python_exe=None): if state_dir is None: - state_dir = DEFAULT_STATE_DIR + if self.is_root: + state_dir = DEFAULT_ROOT_STATE_DIR + else: + state_dir = DEFAULT_STATE_DIR self.state_dir = abspath(expanduser(state_dir)) debug(f"Gravity state dir: {self.state_dir}") self.__configs = {} @@ -88,6 +92,10 @@ def __convert_config_1_0(self, state): del config[key] state.config_files[config_file] = config + @property + def is_root(self): + return os.geteuid() == 0 + def get_config(self, conf, defaults=None): if conf in self.__configs: return self.__configs[conf] From 0a807a87ad994be32cc121ae34397291f4f0e607 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 15:20:42 -0400 Subject: [PATCH 04/27] systemd PM: require virtualenv most of the time, support update --force --- gravity/process_manager/systemd.py | 46 ++++++++++++++++++------------ tests/test_config_manager.py | 3 ++ 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 3224904..68a6469 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -4,8 +4,9 @@ import os import shlex import subprocess +from glob import glob -from gravity.io import debug, info +from gravity.io import debug, error, info, warn from gravity.process_manager import BaseProcessManager from gravity.settings import ProcessManager @@ -51,7 +52,7 @@ class SystemdProcessManager(BaseProcessManager): def __init__(self, state_dir=None, start_daemon=True, foreground=False, **kwargs): super(SystemdProcessManager, self).__init__(state_dir=state_dir, **kwargs) - self.user_mode = os.geteuid() != 0 + self.user_mode = not self.config_manager.is_root @property def __systemd_unit_dir(self): @@ -75,7 +76,7 @@ def __systemctl(self, *args, **kwargs): def terminate(self): """ """ - debug("TERMINATE") + pass def __unit_name(self, instance_name, service): unit_name = f"{service['config_type']}-" @@ -96,7 +97,19 @@ def __update_service(self, config_file, config, attribs, service, instance_name) # Insert a single leading space attach_to_pool_opt = f" {_attach_to_pool_opt}" + # under supervisor we expect that gravity is installed in the galaxy venv and the venv is active when gravity + # runs, but under systemd this is not the case. we do assume $VIRTUAL_ENV is the galaxy venv if running as an + # unprivileged user, though. virtualenv_dir = attribs.get("virtualenv") + environ_virtual_env = os.environ.get("VIRTUAL_ENV") + if not virtualenv_dir and self.user_mode and environ_virtual_env: + warn(f"Assuming Galaxy virtualenv is value of $VIRTUAL_ENV: {environ_virtual_env}") + warn("Set `virtualenv` in Gravity configuration to override") + virtualenv_dir = environ_virtual_env + elif not virtualenv_dir: + error("The `virtualenv` Gravtiy config option must be set when using the systemd process manager") + sys.exit(1) + virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" gunicorn_options = attribs["gunicorn"].copy() gunicorn_options["preload"] = "--preload" if gunicorn_options["preload"] else "" @@ -217,22 +230,19 @@ def status(self, configs=None, service_names=None): def update(self, configs=None, force=False, **kwargs): """ """ + if force: + for config in configs: + service_units = glob(os.path.join(self.__systemd_unit_dir, f"{config.config_type}-*.service")) + # TODO: would need to add targets here assuming we add one + if service_units: + newline = '\n' + info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") + map(os.unlink, service_units) for config in configs: - process_manager = config["process_manager"] - if process_manager == self.name: - self._process_config(config) - else: - pass - """ - # FIXME: refactor - instance_name = config["instance_name"] - instance_conf_dir = join(self.supervisord_conf_dir, f"{instance_name}.d") - group_file = join(self.supervisord_conf_dir, f"group_{instance_name}.conf") - if os.path.exists(instance_conf_dir): - shutil.rmtree(instance_conf_dir) - if os.path.exists(group_file): - os.unlink(group_file) - """ + self._process_config(config) + # FIXME: you need an equivalent to the supervisor pm's remove invalid here + # self._remove_invalid_configs(valid_configs=configs) + # FIXME: only reload if there are changes self.__systemctl("daemon-reload") diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 08e35b1..3610444 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -114,3 +114,6 @@ def test_convert_0_x_config(state_dir, galaxy_yml, configstate_yaml_0_x): assert config.config_type == "galaxy" assert config.instance_name == "gravity-0-x" assert "attribs" not in config + + +# TODO: tests for switching process managers between supervisor and systemd From ebfeaf015de6dd2b57050863f28e5869e0c5b5a4 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 17:05:45 -0400 Subject: [PATCH 05/27] Partial handling of instance name collision (on register, but not rename) --- gravity/config_manager.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 8f69a3e..d6894c8 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -119,6 +119,9 @@ def get_config(self, conf, defaults=None): config.attribs = {} config.services = [] config.__file__ = conf + # FIXME: I don't think we use the persisted value of instance_name anymore, this comes straight from the Gravity + # config. We might not need it at all, but we also need to validate that it doesn't collide with other + # registered instances on every load, in case the admin has changed the instance name since last run. config.instance_name = gravity_config.instance_name config.config_type = server_section config.process_manager = gravity_config.process_manager @@ -336,6 +339,8 @@ def add(self, config_files, galaxy_root=None): exception(f"Cannot add {config_file}: File is unknown type") if conf["instance_name"] is None: conf["instance_name"] = conf["config_type"] + "-" + hashlib.md5(os.urandom(32)).hexdigest()[:12] + if conf["instance_name"] in self.get_registered_instance_names(): + exception(f"Cannot add {config_file}: instance_name '{conf['instance_name']}' already in use") conf_data = {} for key in ConfigFile.persist_keys: conf_data[key] = conf[key] From 16b4dca5e18a749ab07f20cccc6e6754adfd9205 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 29 Sep 2022 17:06:46 -0400 Subject: [PATCH 06/27] Require galaxy_user when running as root in systemd mode, refuse to run as root if not in systemd mode. --- gravity/config_manager.py | 2 ++ gravity/process_manager/systemd.py | 24 +++++++++++++----------- gravity/settings.py | 27 ++++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index d6894c8..5eb9cc9 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -136,6 +136,8 @@ def get_config(self, conf, defaults=None): config.attribs["tusd"] = gravity_config.tusd.dict() config.attribs["celery"] = gravity_config.celery.dict() config.attribs["handlers"] = gravity_config.handlers + config.attribs["galaxy_user"] = gravity_config.galaxy_user + config.attribs["galaxy_group"] = gravity_config.galaxy_group # Store gravity version, in case we need to convert old setting webapp_service_names = [] diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 68a6469..783b7a5 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -6,7 +6,7 @@ import subprocess from glob import glob -from gravity.io import debug, error, info, warn +from gravity.io import debug, error, exception, info, warn from gravity.process_manager import BaseProcessManager from gravity.settings import ProcessManager @@ -45,7 +45,6 @@ SYSTEMD_SERVICE_TEMPLATES["celery-beat"] = SYSTEMD_SERVICE_TEMPLATES["gunicorn"] -# FIXME: need to document and enforce that gravity_config.virtualenv is required in systemd mode class SystemdProcessManager(BaseProcessManager): name = ProcessManager.systemd @@ -64,13 +63,17 @@ def __use_instance(self): #return not self.config_manager.single_instance return False - def __systemctl(self, *args, **kwargs): + def __systemctl(self, *args, ignore_rc=None, **kwargs): args = list(args) if self.user_mode: args = ["--user"] + args try: debug("Calling systemctl with args: %s", args) - subprocess.check_call(["systemctl"] + args) + try: + subprocess.check_call(["systemctl"] + args) + except subprocess.CalledProcessError as exc: + if ignore_rc is None or exc.returncode not in ignore_rc: + raise except: raise @@ -78,6 +81,7 @@ def terminate(self): """ """ pass + # FIXME: should be __service_program_name for follow()? def __unit_name(self, instance_name, service): unit_name = f"{service['config_type']}-" if self.__use_instance: @@ -107,8 +111,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) warn("Set `virtualenv` in Gravity configuration to override") virtualenv_dir = environ_virtual_env elif not virtualenv_dir: - error("The `virtualenv` Gravtiy config option must be set when using the systemd process manager") - sys.exit(1) + exception("The `virtualenv` Gravity config option must be set when using the systemd process manager") virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" gunicorn_options = attribs["gunicorn"].copy() @@ -139,7 +142,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) format_vars["command"] = f"{virtualenv_bin}/{format_vars['command']}" if not self.user_mode: format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" - if "galaxy_group" in attribs: + if attribs["galaxy_group"] is not None: format_vars["systemd_user_group"] += f"\nGroup={attribs['galaxy_group']}" conf = os.path.join(self.__systemd_unit_dir, unit_name) @@ -155,8 +158,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) format_vars["environment"] = "\n".join("Environment={}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) contents = template.format(**format_vars) - service_name = self._service_program_name(instance_name, service) - self._update_file(conf, contents, service_name, "service") + self._update_file(conf, contents, unit_name, "service") return conf @@ -226,7 +228,7 @@ def graceful(self, configs=None, service_names=None): def status(self, configs=None, service_names=None): """ """ unit_names = self.__unit_names(configs, service_names) - self.__systemctl("status", "--lines=0", *unit_names) + self.__systemctl("status", "--lines=0", *unit_names, ignore_rc=(3,)) def update(self, configs=None, force=False, **kwargs): """ """ @@ -237,7 +239,7 @@ def update(self, configs=None, force=False, **kwargs): if service_units: newline = '\n' info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") - map(os.unlink, service_units) + list(map(os.unlink, service_units)) for config in configs: self._process_config(config) # FIXME: you need an equivalent to the supervisor pm's remove invalid here diff --git a/gravity/settings.py b/gravity/settings.py index 8661a83..041203a 100644 --- a/gravity/settings.py +++ b/gravity/settings.py @@ -1,3 +1,4 @@ +import os from enum import Enum from typing import ( Any, @@ -183,6 +184,7 @@ class Settings(BaseSettings): description=""" Process manager to use. ``supervisor`` is the default process manager. +``systemd`` is also supported. """) galaxy_root: Optional[str] = Field( @@ -190,6 +192,18 @@ class Settings(BaseSettings): description=""" Specify Galaxy's root directory. Gravity will attempt to find the root directory, but you can set the directory explicitly with this option. +""") + galaxy_user: Optional[str] = Field( + None, + description=""" +User to run Galaxy as, required when using the systemd process manager as root. +Ignored with supervisor or user-mode systemd. +""") + galaxy_group: Optional[str] = Field( + None, + description=""" +Group to run Galaxy as, optional when using the systemd process manager as root. +Ignored with supervisor or user-mode systemd. """) log_dir: Optional[str] = Field( None, @@ -199,7 +213,8 @@ class Settings(BaseSettings): """) virtualenv: Optional[str] = Field(None, description=""" Set to Galaxy's virtualenv directory. -If not specified, Gravity assumes all processes are on PATH. +If not specified, Gravity assumes all processes are on PATH. This option is required in most circumstances when using +the ``systemd`` process manager. """) app_server: AppServer = Field( AppServer.gunicorn, @@ -233,6 +248,16 @@ class Settings(BaseSettings): _normalize_celery = validator("celery", allow_reuse=True, pre=True)(none_to_default) _normalize_tusd = validator("tusd", allow_reuse=True, pre=True)(none_to_default) + # Require galaxy_user if running as root + @validator("galaxy_user") + def _user_required_if_root(cls, v, values): + if os.geteuid() == 0: + if values["process_manager"] == ProcessManager.systemd: + raise ValueError("galaxy_user is required when running as root") + else: + raise ValueError("Gravity cannot be run as root unless using the systemd process manager") + return v + class Config: env_prefix = "gravity_" env_nested_delimiter = "." From 9c0ecc424f8ab9c6badcff211530f418414b6bd4 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 7 Oct 2022 14:49:58 +0200 Subject: [PATCH 07/27] Systemd fixes --- gravity/process_manager/supervisor.py | 5 ++-- gravity/process_manager/systemd.py | 40 ++++++++++++++++++--------- gravity/state.py | 5 ++-- 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 08f377f..a4f6788 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -267,13 +267,14 @@ def __update_service(self, config_file, config, attribs, service, instance_conf_ program_name = self._service_program_name(instance_name, service) - # used by the "standalone" service type + # TODO: used by the "standalone" service type, refactor this attach_to_pool_opt = "" server_pools = service.get("server_pools") if server_pools: _attach_to_pool_opt = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) # Insert a single leading space attach_to_pool_opt = f" {_attach_to_pool_opt}" + pid_file_opt = f" --pid-file={self.supervisor_state_dir}/{program_name}.pid" virtualenv_dir = attribs.get("virtualenv") virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" @@ -286,6 +287,7 @@ def __update_service(self, config_file, config, attribs, service, instance_conf_ "config_type": service["config_type"], "server_name": service["service_name"], "attach_to_pool_opt": attach_to_pool_opt, + "pid_file_opt": pid_file_opt, "gunicorn": gunicorn_options, "celery": attribs["celery"], "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], @@ -297,7 +299,6 @@ def __update_service(self, config_file, config, attribs, service, instance_conf_ "galaxy_conf": config_file, "galaxy_root": config["galaxy_root"], "virtualenv_bin": virtualenv_bin, - "supervisor_state_dir": self.supervisor_state_dir, "state_dir": self.state_dir, } format_vars["command_arguments"] = service.get_command_arguments(attribs, format_vars) diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 783b7a5..d87a4ed 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -11,7 +11,7 @@ from gravity.settings import ProcessManager SYSTEMD_SERVICE_TEMPLATES = {} -SYSTEMD_SERVICE_TEMPLATES["gunicorn"] = """; +DEFAULT_SYSTEMD_SERVICE_TEMPLATE = """; ; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN ; @@ -41,8 +41,12 @@ WantedBy=multi-user.target """ -SYSTEMD_SERVICE_TEMPLATES["celery"] = SYSTEMD_SERVICE_TEMPLATES["gunicorn"] -SYSTEMD_SERVICE_TEMPLATES["celery-beat"] = SYSTEMD_SERVICE_TEMPLATES["gunicorn"] +SYSTEMD_SERVICE_TEMPLATES["gunicorn"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE +SYSTEMD_SERVICE_TEMPLATES["celery"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE +SYSTEMD_SERVICE_TEMPLATES["celery-beat"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE +SYSTEMD_SERVICE_TEMPLATES["standalone"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE +SYSTEMD_SERVICE_TEMPLATES["gx-it-proxy"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE +SYSTEMD_SERVICE_TEMPLATES["tusd"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE class SystemdProcessManager(BaseProcessManager): @@ -55,7 +59,9 @@ def __init__(self, state_dir=None, start_daemon=True, foreground=False, **kwargs @property def __systemd_unit_dir(self): - unit_path = "/etc/systemd/system" if not self.user_mode else os.path.expanduser("~/.config/systemd/user") + unit_path = os.environ.get("SYSTEMD_UNIT_PATH") + if not unit_path: + unit_path = "/etc/systemd/system" if not self.user_mode else os.path.expanduser("~/.config/systemd/user") return unit_path @property @@ -92,7 +98,6 @@ def __unit_name(self, instance_name, service): def __update_service(self, config_file, config, attribs, service, instance_name): unit_name = self.__unit_name(instance_name, service) - # FIXME: refactor # used by the "standalone" service type attach_to_pool_opt = "" server_pools = service.get("server_pools") @@ -125,6 +130,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) "config_type": service["config_type"], "server_name": service["service_name"], "attach_to_pool_opt": attach_to_pool_opt, + "pid_file_opt": "", "gunicorn": gunicorn_options, "celery": attribs["celery"], "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], @@ -168,7 +174,6 @@ def _process_config(self, config, **kwargs): attribs = config["attribs"] config_file = config.__file__ intended_configs = set() - present_configs = set() try: os.makedirs(self.__systemd_unit_dir) @@ -176,13 +181,25 @@ def _process_config(self, config, **kwargs): if exc.errno != errno.EEXIST: raise - # FIXME: none of this works for instances for service in config["services"]: intended_configs.add(self.__update_service(config_file, config, attribs, service, instance_name)) + return intended_configs + + def _process_configs(self, configs): + intended_configs = set() + + for config in configs: + intended_configs = intended_configs | self._process_config(config) + + # the unit dir might not exist if $SYSTEMD_UNIT_PATH is set (e.g. for tests), but this is fine if there are no + # intended configs + if not intended_configs and not os.path.exists(self.__systemd_unit_dir): + return + # FIXME: should use config_type, but that's per-service _present_configs = filter(lambda f: f.startswith("galaxy-"), os.listdir(self.__systemd_unit_dir)) - present_configs.update([os.path.join(self.__systemd_unit_dir, f) for f in _present_configs]) + present_configs = set([os.path.join(self.__systemd_unit_dir, f) for f in _present_configs]) for file in (present_configs - intended_configs): service_name = os.path.basename(os.path.splitext(file)[0]) @@ -240,11 +257,8 @@ def update(self, configs=None, force=False, **kwargs): newline = '\n' info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") list(map(os.unlink, service_units)) - for config in configs: - self._process_config(config) - # FIXME: you need an equivalent to the supervisor pm's remove invalid here - # self._remove_invalid_configs(valid_configs=configs) - # FIXME: only reload if there are changes + self._process_configs(configs) + # TODO: only reload if there are changes self.__systemctl("daemon-reload") diff --git a/gravity/state.py b/gravity/state.py index ad7557c..93e4bf8 100644 --- a/gravity/state.py +++ b/gravity/state.py @@ -178,9 +178,8 @@ class GalaxyTUSDService(Service): class GalaxyStandaloneService(Service): service_type = "standalone" service_name = "standalone" - # FIXME: supervisor-specific - command_template = "{virtualenv_bin}python ./lib/galaxy/main.py -c {galaxy_conf} --server-name={server_name}{attach_to_pool_opt}" \ - " --pid-file={supervisor_state_dir}/{program_name}.pid" + command_template = "{virtualenv_bin}python ./lib/galaxy/main.py -c {galaxy_conf} --server-name={server_name}" \ + "{attach_to_pool_opt}{pid_file_opt}" def get_environment(self): return self.get("environment") or {} From de4168e2e70394ca28a6ab01c6005a7f5bdd55d2 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 7 Oct 2022 14:50:34 +0200 Subject: [PATCH 08/27] Use pytest parameterization better, and test the systemd pm in test_process_manager --- .github/workflows/test.yaml | 1 + tests/conftest.py | 19 +++- tests/test_process_manager.py | 203 ++++++++++++++++++++-------------- 3 files changed, 137 insertions(+), 86 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 6f55499..44fda7a 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -25,5 +25,6 @@ jobs: run: tox -e test env: GRAVITY_TEST_GALAXY_BRANCH: ${{ matrix.galaxy-branch }} + XDG_RUNTIME_DIR: /run/user/$UID - name: "Upload coverage to Codecov" uses: codecov/codecov-action@v2 diff --git a/tests/conftest.py b/tests/conftest.py index 426a97e..090a868 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,6 +14,7 @@ TEST_DIR = Path(os.path.dirname(__file__)) GXIT_CONFIG = """ gravity: + process_manager: {process_manager_name} gunicorn: bind: 'localhost:{gx_port}' gx_it_proxy: @@ -122,6 +123,7 @@ def galaxy_yml(galaxy_root_dir): @pytest.fixture() def state_dir(): directory = tempfile.mkdtemp() + os.environ['SYSTEMD_UNIT_PATH'] = os.path.join(directory, 'systemd') try: yield Path(directory) finally: @@ -149,11 +151,11 @@ def configstate_yaml_0_x(galaxy_root_dir, state_dir, galaxy_yml): @pytest.fixture() def job_conf(request, galaxy_root_dir): - conf = yaml.safe_load(request.param[0]) + conf = yaml.safe_load(request.param) ext = "xml" if isinstance(conf, str) else "yml" job_conf_path = galaxy_root_dir / 'config' / f'job_conf.{ext}' with open(job_conf_path, 'w') as jcfh: - jcfh.write(request.param[0]) + jcfh.write(request.param) yield job_conf_path os.unlink(job_conf_path) @@ -187,14 +189,19 @@ def startup_config(galaxy_virtualenv, free_port): @pytest.fixture -def gxit_config(free_port, another_free_port): - config_yaml = GXIT_CONFIG.format(gxit_port=another_free_port, gx_port=free_port) +def gxit_config(free_port, another_free_port, process_manager_name): + config_yaml = GXIT_CONFIG.format( + gxit_port=another_free_port, + gx_port=free_port, + process_manager_name=process_manager_name) return yaml.safe_load(config_yaml) @pytest.fixture -def tusd_config(startup_config, free_port, another_free_port): - startup_config["gravity"] = {"tusd": {"enable": True, "port": another_free_port, "upload_dir": "/tmp"}} +def tusd_config(startup_config, free_port, another_free_port, process_manager_name): + startup_config["gravity"] = { + "process_manager": process_manager_name, + "tusd": {"enable": True, "port": another_free_port, "upload_dir": "/tmp"}} startup_config["galaxy"]["galaxy_infrastructure_url"] = f"http://localhost:{free_port}" return startup_config diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index c520b05..470a6d3 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -1,4 +1,5 @@ import json +import string from pathlib import Path import pytest @@ -12,6 +13,11 @@ + + + + + """ @@ -53,6 +59,7 @@ DYNAMIC_HANDLER_CONFIG = """ gravity: + process_manager: %(process_manager_name)s handlers: handler: processes: 2 @@ -77,18 +84,62 @@ """ -def test_update(galaxy_yml, default_config_manager): +# make pytest.params out of constants +params={} +for name in [n for n in dir() if all([(c in string.ascii_uppercase + '_') for c in n])]: + params[name] = pytest.param(globals()[name], id=name.lower()) + + +def service_conf_dir(state_dir, process_manager_name): + state_dir = Path(state_dir) + if process_manager_name == 'supervisor': + return state_dir / 'supervisor' / 'supervisord.conf.d' / '_default_.d' + elif process_manager_name == 'systemd': + return state_dir / 'systemd' + raise Exception(f"Invalid process manager name: {process_manager_name}") + + +def service_conf_file(process_manager_name, service_name, service_type=None): + service_type = service_type or service_name + if process_manager_name == 'supervisor': + return f'galaxy_{service_type}_{service_name}.conf' + elif process_manager_name == 'systemd': + return f'galaxy-{service_name}.service' + raise Exception(f"Invalid process manager name: {process_manager_name}") + + +def service_conf_path(state_dir, process_manager_name, service_name, service_type=None): + conf_dir = service_conf_dir(state_dir, process_manager_name) + conf_file = service_conf_file(process_manager_name, service_name, service_type) + return conf_dir / conf_file + + +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_update(galaxy_yml, default_config_manager, process_manager_name): default_config_manager.add([str(galaxy_yml)]) new_bind = 'localhost:8081' - galaxy_yml.write(json.dumps({'galaxy': None, 'gravity': {'gunicorn': {'bind': new_bind}}})) + galaxy_yml.write(json.dumps( + {'galaxy': None, 'gravity': {'process_manager': process_manager_name, 'gunicorn': {'bind': new_bind}}})) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() -def test_update_force(galaxy_yml, default_config_manager): - test_update(galaxy_yml, default_config_manager) - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - gunicorn_conf_path = instance_conf_dir / "galaxy_gunicorn_gunicorn.conf" +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_update_default_services(galaxy_yml, default_config_manager, process_manager_name): + test_update(galaxy_yml, default_config_manager, process_manager_name) + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + gunicorn_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'gunicorn') + assert gunicorn_conf_path.exists() + celery_conf_path = conf_dir / service_conf_file(process_manager_name, 'celery') + assert celery_conf_path.exists() + celery_beat_conf_path = conf_dir / service_conf_file(process_manager_name, 'celery-beat') + assert celery_beat_conf_path.exists() + + +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_update_force(galaxy_yml, default_config_manager, process_manager_name): + test_update(galaxy_yml, default_config_manager, process_manager_name) + gunicorn_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'gunicorn') assert gunicorn_conf_path.exists() update_time = gunicorn_conf_path.stat().st_mtime with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: @@ -99,10 +150,10 @@ def test_update_force(galaxy_yml, default_config_manager): assert gunicorn_conf_path.stat().st_mtime != update_time -def test_cleanup(galaxy_yml, default_config_manager): - test_update(galaxy_yml, default_config_manager) - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - gunicorn_conf_path = instance_conf_dir / "galaxy_gunicorn_gunicorn.conf" +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_cleanup(galaxy_yml, default_config_manager, process_manager_name): + test_update(galaxy_yml, default_config_manager, process_manager_name) + gunicorn_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'gunicorn') default_config_manager.remove([str(galaxy_yml)]) assert gunicorn_conf_path.exists() with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: @@ -110,21 +161,23 @@ def test_cleanup(galaxy_yml, default_config_manager): assert not gunicorn_conf_path.exists() -def test_disable_services(galaxy_yml, default_config_manager): +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_disable_services(galaxy_yml, default_config_manager, process_manager_name): default_config_manager.add([str(galaxy_yml)]) galaxy_yml.write(json.dumps( {'galaxy': None, 'gravity': { + 'process_manager': process_manager_name, 'gunicorn': {'enable': False}, 'celery': {'enable': False, 'enable_beat': False}}} )) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - gunicorn_conf_path = instance_conf_dir / "galaxy_gunicorn_gunicorn.conf" + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + gunicorn_conf_path = conf_dir / service_conf_file(process_manager_name, 'gunicorn') assert not gunicorn_conf_path.exists() - celery_conf_path = instance_conf_dir / "galaxy_celery_celery.conf" + celery_conf_path = conf_dir / service_conf_file(process_manager_name, 'celery') assert not celery_conf_path.exists() - celery_beat_conf_path = instance_conf_dir / "galaxy_celery-beat_celery-beat.conf" + celery_beat_conf_path = conf_dir / service_conf_file(process_manager_name, 'celery-beat') assert not celery_beat_conf_path.exists() @@ -156,123 +209,113 @@ def test_gunicorn_graceful_method_no_preload(galaxy_yml, default_config_manager) assert graceful_method == GracefulMethod.SIGHUP -@pytest.mark.parametrize('job_conf', [[JOB_CONF_XML_DYNAMIC_HANDLERS]], indirect=True) -def test_dynamic_handlers(default_config_manager, galaxy_yml, job_conf): - galaxy_yml.write(DYNAMIC_HANDLER_CONFIG) +@pytest.mark.parametrize('job_conf', [params["JOB_CONF_XML_DYNAMIC_HANDLERS"]], indirect=True) +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_dynamic_handlers(default_config_manager, galaxy_yml, job_conf, process_manager_name): + galaxy_yml.write(DYNAMIC_HANDLER_CONFIG % {"process_manager_name": process_manager_name}) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - handler_config_paths = [instance_conf_dir / f'galaxy_standalone_handler{i}.conf' for i in range(3)] + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + handler_config_paths = [conf_dir / service_conf_file( + process_manager_name, f'handler{i}', service_type='standalone') for i in range(3)] for config_path in handler_config_paths: assert config_path.exists() handler0_config = handler_config_paths[0].open().read() assert " --server-name=handler0" in handler0_config assert " --attach-to-pool=job-handlers --attach-to-pool=workflow-schedulers" in handler0_config - assert " FOO=foo" in handler0_config + assert "FOO=foo" in handler0_config handler1_config = handler_config_paths[1].open().read() assert " --server-name=handler1" in handler1_config assert " --attach-to-pool=job-handlers.special" in handler1_config - assert " BAR=bar" in handler1_config + assert "BAR=bar" in handler1_config handler2_config = handler_config_paths[2].open().read() assert " --server-name=handler2" in handler2_config assert " --attach-to-pool=job-handlers --attach-to-pool=job-handlers.special" in handler2_config -@pytest.mark.parametrize('job_conf', [[JOB_CONF_YAML_NO_HANDLERS]], indirect=True) -def test_no_static_handlers_yaml(default_config_manager, galaxy_yml, job_conf): +@pytest.mark.parametrize( + 'job_conf', [params["JOB_CONF_YAML_NO_HANDLERS"], params["JOB_CONF_XML_NO_HANDLERS"]], indirect=True) +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_no_static_handlers(default_config_manager, galaxy_yml, job_conf, process_manager_name): with open(galaxy_yml, 'w') as config_fh: - config_fh.write(json.dumps({'galaxy': {'job_config_file': str(job_conf)}})) + config_fh.write(json.dumps({ + 'gravity': {'process_manager': process_manager_name}, + 'galaxy': {'job_config_file': str(job_conf)}})) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - assert not (instance_conf_dir / 'galaxy_standalone_handler0.conf').exists() + handler_config_path = service_conf_path( + default_config_manager.state_dir, + process_manager_name, 'handler0', service_type='standalone') + assert not handler_config_path.exists() -@pytest.mark.parametrize('job_conf', [[JOB_CONF_XML_NO_HANDLERS]], indirect=True) -def test_no_static_handlers_xml(default_config_manager, galaxy_yml, job_conf): +@pytest.mark.parametrize( + 'job_conf', [params["JOB_CONF_YAML_STATIC_HANDLERS"], params["JOB_CONF_XML_STATIC_HANDLERS"]], indirect=True) +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_static_handlers(default_config_manager, galaxy_yml, job_conf, process_manager_name): with open(galaxy_yml, 'w') as config_fh: - config_fh.write(json.dumps({'galaxy': {'job_config_file': str(job_conf)}})) + config_fh.write(json.dumps({ + 'gravity': {'process_manager': process_manager_name}, + 'galaxy': {'job_config_file': str(job_conf)}})) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - assert not (instance_conf_dir / 'galaxy_standalone_handler0.conf').exists() - - -@pytest.mark.parametrize('job_conf', [[JOB_CONF_YAML_STATIC_HANDLERS]], indirect=True) -def test_static_handlers_yaml(default_config_manager, galaxy_yml, job_conf): - with open(galaxy_yml, 'w') as config_fh: - config_fh.write(json.dumps({'galaxy': {'job_config_file': str(job_conf)}})) - default_config_manager.add([str(galaxy_yml)]) - with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: - pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - handler0_config_path = instance_conf_dir / 'galaxy_standalone_handler0.conf' + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler0', service_type='standalone') assert handler0_config_path.exists() - assert '.yml --server-name=handler0 --pid-file=' in handler0_config_path.open().read() - handler1_config_path = instance_conf_dir / 'galaxy_standalone_handler1.conf' + assert '.yml --server-name=handler0' in handler0_config_path.open().read() + handler1_config_path = conf_dir / service_conf_file(process_manager_name, 'handler1', service_type='standalone') assert handler1_config_path.exists() handler1_config = handler1_config_path.open().read() - assert '.yml --server-name=handler1 --pid-file=' in handler1_config - assert 'BAZ=baz' in handler1_config - assert (instance_conf_dir / 'galaxy_standalone_sge_handler.conf').exists() - assert (instance_conf_dir / 'galaxy_standalone_special_handler0.conf').exists() - assert (instance_conf_dir / 'galaxy_standalone_special_handler1.conf').exists() + assert '.yml --server-name=handler1' in handler1_config + if job_conf.ext == '.yml': + # setting env vars on static handlers is only supported with YAML job conf + assert 'BAZ=baz' in handler1_config + for handler_name in ('sge_handler', 'special_handler0', 'special_handler1'): + assert (conf_dir / service_conf_file(process_manager_name, handler_name, service_type='standalone')).exists() -def test_static_handlers_embedded_in_galaxy_yml(default_config_manager, galaxy_yml): +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_static_handlers_embedded_in_galaxy_yml(default_config_manager, galaxy_yml, process_manager_name): with open(galaxy_yml, 'w') as config_fh: - config_fh.write(json.dumps({'galaxy': {'job_config': safe_load(JOB_CONF_YAML_STATIC_HANDLERS)}})) - default_config_manager.add([str(galaxy_yml)]) - with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: - pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - handler0_config_path = instance_conf_dir / 'galaxy_standalone_handler0.conf' - assert handler0_config_path.exists() - assert '.yml --server-name=handler0 --pid-file=' in handler0_config_path.open().read() - handler1_config_path = instance_conf_dir / 'galaxy_standalone_handler1.conf' - assert handler1_config_path.exists() - assert '.yml --server-name=handler1 --pid-file=' in handler1_config_path.open().read() - assert (instance_conf_dir / 'galaxy_standalone_sge_handler.conf').exists() - assert (instance_conf_dir / 'galaxy_standalone_special_handler0.conf').exists() - assert (instance_conf_dir / 'galaxy_standalone_special_handler1.conf').exists() - - -@pytest.mark.parametrize('job_conf', [[JOB_CONF_XML_STATIC_HANDLERS]], indirect=True) -def test_static_handlers(default_config_manager, galaxy_yml, job_conf): + config_fh.write(json.dumps({ + 'gravity': {'process_manager': process_manager_name}, + 'galaxy': {'job_config': safe_load(JOB_CONF_YAML_STATIC_HANDLERS)}})) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - handler0_config_path = instance_conf_dir / 'galaxy_standalone_handler0.conf' + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler0', service_type='standalone') assert handler0_config_path.exists() - assert f'{str(galaxy_yml)} --server-name=handler0 --pid-file=' in handler0_config_path.open().read() - handler1_config_path = instance_conf_dir / 'galaxy_standalone_handler1.conf' + assert '.yml --server-name=handler0' in handler0_config_path.open().read() + handler1_config_path = conf_dir / service_conf_file(process_manager_name, 'handler1', service_type='standalone') assert handler1_config_path.exists() - assert f'{str(galaxy_yml)} --server-name=handler1 --pid-file=' in handler1_config_path.open().read() + assert '.yml --server-name=handler1' in handler1_config_path.open().read() + for handler_name in ('sge_handler', 'special_handler0', 'special_handler1'): + assert (conf_dir / service_conf_file(process_manager_name, handler_name, service_type='standalone')).exists() -def test_gxit_handler(default_config_manager, galaxy_yml, gxit_config): +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_gxit_handler(default_config_manager, galaxy_yml, gxit_config, process_manager_name): galaxy_yml.write(json.dumps(gxit_config)) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - gxit_config_path = instance_conf_dir / 'galaxy_gx-it-proxy_gx-it-proxy.conf' + gxit_config_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'gx-it-proxy') assert gxit_config_path.exists() gxit_port = gxit_config["gravity"]["gx_it_proxy"]["port"] sessions = "database/interactivetools_map.sqlite" assert f'npx gx-it-proxy --ip localhost --port {gxit_port} --sessions {sessions}' in gxit_config_path.read_text() -def test_tusd_process(default_config_manager, galaxy_yml, tusd_config): +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_tusd_process(default_config_manager, galaxy_yml, tusd_config, process_manager_name): galaxy_yml.write(json.dumps(tusd_config)) default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() - instance_conf_dir = Path(default_config_manager.state_dir) / 'supervisor' / 'supervisord.conf.d' / '_default_.d' - tusd_config_path = instance_conf_dir / 'galaxy_tusd_tusd.conf' + tusd_config_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'tusd') assert tusd_config_path.exists() assert "tusd -host" in tusd_config_path.read_text() From ee8e099a6f9df58c907542cd5f635d1151a97d12 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 7 Oct 2022 16:05:11 +0200 Subject: [PATCH 09/27] Fix operations tests, plus run on ubuntu-22.04 where hopefully systemctl --user works --- .github/workflows/test.yaml | 5 ++--- gravity/process_manager/systemd.py | 19 +++++++++---------- tests/test_operations.py | 4 +++- tests/test_process_manager.py | 2 +- tox.ini | 3 +++ 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 44fda7a..4d369c0 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -6,14 +6,14 @@ concurrency: jobs: test: name: Test - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 strategy: fail-fast: false matrix: python-version: ['3.7', '3.10'] galaxy-branch: ['release_22.01', 'dev'] steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - uses: actions/setup-python@v2 with: python-version: ${{ matrix.python-version }} @@ -25,6 +25,5 @@ jobs: run: tox -e test env: GRAVITY_TEST_GALAXY_BRANCH: ${{ matrix.galaxy-branch }} - XDG_RUNTIME_DIR: /run/user/$UID - name: "Upload coverage to Codecov" uses: codecov/codecov-action@v2 diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index d87a4ed..1c42bfd 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -6,7 +6,7 @@ import subprocess from glob import glob -from gravity.io import debug, error, exception, info, warn +from gravity.io import debug, exception, info, warn from gravity.process_manager import BaseProcessManager from gravity.settings import ProcessManager @@ -71,17 +71,17 @@ def __use_instance(self): def __systemctl(self, *args, ignore_rc=None, **kwargs): args = list(args) + extra_args = os.environ.get("GRAVITY_SYSTEMCTL_EXTRA_ARGS") + if extra_args: + args = shlex.split(extra_args) + args if self.user_mode: args = ["--user"] + args + debug("Calling systemctl with args: %s", args) try: - debug("Calling systemctl with args: %s", args) - try: - subprocess.check_call(["systemctl"] + args) - except subprocess.CalledProcessError as exc: - if ignore_rc is None or exc.returncode not in ignore_rc: - raise - except: - raise + subprocess.check_call(["systemctl"] + args) + except subprocess.CalledProcessError as exc: + if ignore_rc is None or exc.returncode not in ignore_rc: + raise def terminate(self): """ """ @@ -261,7 +261,6 @@ def update(self, configs=None, force=False, **kwargs): # TODO: only reload if there are changes self.__systemctl("daemon-reload") - def shutdown(self): """ """ debug(f"SHUTDOWN") diff --git a/tests/test_operations.py b/tests/test_operations.py index 51c4646..01e817c 100644 --- a/tests/test_operations.py +++ b/tests/test_operations.py @@ -4,6 +4,7 @@ import time from pathlib import Path +import pytest import requests from click.testing import CliRunner from yaml import safe_load @@ -92,7 +93,8 @@ def test_cmd_start(state_dir, galaxy_yml, startup_config, free_port): assert "All processes stopped, supervisord will exit" in result.output -def test_cmd_start_with_gxit(state_dir, galaxy_yml, gxit_startup_config, free_port): +@pytest.mark.parametrize('process_manager_name', ['supervisor']) +def test_cmd_start_with_gxit(state_dir, galaxy_yml, gxit_startup_config, free_port, process_manager_name): galaxy_yml.write(json.dumps(gxit_startup_config)) runner = CliRunner() result = runner.invoke(galaxyctl, ['--state-dir', state_dir, 'register', str(galaxy_yml)]) diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index 470a6d3..7568a37 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -85,7 +85,7 @@ # make pytest.params out of constants -params={} +params = {} for name in [n for n in dir() if all([(c in string.ascii_uppercase + '_') for c in n])]: params[name] = pytest.param(globals()[name], id=name.lower()) diff --git a/tox.ini b/tox.ini index 2997981..1e88aff 100644 --- a/tox.ini +++ b/tox.ini @@ -18,3 +18,6 @@ deps = test: requests passenv = GRAVITY_TEST_GALAXY_BRANCH + GRAVITY_SYSTEMCTL_EXTRA_ARGS + DBUS_SESSION_BUS_ADDRESS + XDG_RUNTIME_DIR From 9d37836b28f0b3940251bf4f483ad14bf06a3bf8 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 7 Oct 2022 18:02:57 +0200 Subject: [PATCH 10/27] Support filtering by process manager when getting regisered configs from config manager --- gravity/config_manager.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 5eb9cc9..69a75e8 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -291,12 +291,13 @@ def single_instance(self): """Indicate if there is only one configured instance""" return self.instance_count == 1 - def get_registered_configs(self, instances=None): + def get_registered_configs(self, instances=None, process_manager=None): """Return the persisted values of all config files registered with the config manager.""" rval = [] config_files = self.state.config_files for config_file, config in list(config_files.items()): - if (instances is not None and config["instance_name"] in instances) or instances is None: + if (((instances is not None and config["instance_name"] in instances) or instances is None) and + ((process_manager is not None and config.get("process_manager", "supervisor") == process_manager) or process_manager is None)): rval.append(self.get_config(config_file)) return rval From e2fe16cb9ea81db83ab534930feadace0d4496b9 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 7 Oct 2022 18:04:08 +0200 Subject: [PATCH 11/27] More systemd fixes --- gravity/process_manager/__init__.py | 2 +- gravity/process_manager/supervisor.py | 4 +- gravity/process_manager/systemd.py | 63 +++++++++++++++++++-------- tests/conftest.py | 2 +- tests/test_process_manager.py | 9 +++- 5 files changed, 57 insertions(+), 23 deletions(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 303eec2..c6889a8 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -245,5 +245,5 @@ def terminate(self): """ """ @route - def pm(self): + def pm(self, *args): """ """ diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index a4f6788..50f6328 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -363,7 +363,7 @@ def _process_config(self, config): def _remove_invalid_configs(self, valid_configs=None): if not valid_configs: - valid_configs = [c for c in self.config_manager.get_registered_configs() if c.process_manager == self.name] + valid_configs = self.config_manager.get_registered_configs(process_manager=self.name) valid_names = [c.instance_name for c in valid_configs] valid_instance_dirs = [f"{name}.d" for name in valid_names] valid_group_confs = [] @@ -455,7 +455,7 @@ def update(self, configs=None, force=False, **kwargs): if self.__supervisord_is_running(): self.supervisorctl("update") - def supervisorctl(self, *args, **kwargs): + def supervisorctl(self, *args): if not self.__supervisord_is_running(): warn("supervisord is not running") return diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 1c42bfd..17c428c 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -59,35 +59,49 @@ def __init__(self, state_dir=None, start_daemon=True, foreground=False, **kwargs @property def __systemd_unit_dir(self): - unit_path = os.environ.get("SYSTEMD_UNIT_PATH") + unit_path = os.environ.get("GRAVITY_SYSTEMD_UNIT_PATH") if not unit_path: unit_path = "/etc/systemd/system" if not self.user_mode else os.path.expanduser("~/.config/systemd/user") return unit_path @property def __use_instance(self): - #return not self.config_manager.single_instance - return False + return not self.config_manager.single_instance - def __systemctl(self, *args, ignore_rc=None, **kwargs): + def __systemctl(self, *args, ignore_rc=None, capture=False, **kwargs): args = list(args) + call = subprocess.check_call extra_args = os.environ.get("GRAVITY_SYSTEMCTL_EXTRA_ARGS") if extra_args: args = shlex.split(extra_args) + args if self.user_mode: args = ["--user"] + args debug("Calling systemctl with args: %s", args) + if capture: + call = subprocess.check_output try: - subprocess.check_call(["systemctl"] + args) + return call(["systemctl"] + args, text=True) except subprocess.CalledProcessError as exc: if ignore_rc is None or exc.returncode not in ignore_rc: raise + def __journalctl(self, *args, **kwargs): + args = list(args) + if self.user_mode: + args = ["--user"] + args + debug("Calling journalctl with args: %s", args) + subprocess.check_call(["journalctl"] + args) + + def __systemd_env_path(self): + environ = self.__systemctl("show-environment", capture=True) + for line in environ.splitlines(): + if line.startswith("PATH="): + return line.split("=", 1)[1] + def terminate(self): - """ """ + # this is used to stop a foreground supervisord in the supervisor PM, so it is a no-op here pass - # FIXME: should be __service_program_name for follow()? def __unit_name(self, instance_name, service): unit_name = f"{service['config_type']}-" if self.__use_instance: @@ -158,8 +172,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) environment = self._service_environment(service, attribs) if virtualenv_bin and service.add_virtualenv_to_path: - # FIXME: what should we use for a default here? - path = environment.get("PATH", "%(ENV_PATH)s") + path = self.__systemd_env_path() environment["PATH"] = ":".join([virtualenv_bin, path]) format_vars["environment"] = "\n".join("Environment={}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) @@ -168,6 +181,12 @@ def __update_service(self, config_file, config, attribs, service, instance_name) return conf + def follow(self, configs=None, service_names=None, quiet=False): + """ """ + unit_names = self.__unit_names(configs, service_names) + u_args = [i for l in list(zip(["-u"] * len(unit_names), unit_names)) for i in l] + self.__journalctl("-f", *u_args) + def _process_config(self, config, **kwargs): """ """ instance_name = config["instance_name"] @@ -192,19 +211,21 @@ def _process_configs(self, configs): for config in configs: intended_configs = intended_configs | self._process_config(config) - # the unit dir might not exist if $SYSTEMD_UNIT_PATH is set (e.g. for tests), but this is fine if there are no - # intended configs + # the unit dir might not exist if $GRAVITY_SYSTEMD_UNIT_PATH is set (e.g. for tests), but this is fine if there + # are no intended configs if not intended_configs and not os.path.exists(self.__systemd_unit_dir): return # FIXME: should use config_type, but that's per-service - _present_configs = filter(lambda f: f.startswith("galaxy-"), os.listdir(self.__systemd_unit_dir)) + _present_configs = filter( + lambda f: f.startswith("galaxy-") and f.endswith(".service"), + os.listdir(self.__systemd_unit_dir)) present_configs = set([os.path.join(self.__systemd_unit_dir, f) for f in _present_configs]) for file in (present_configs - intended_configs): - service_name = os.path.basename(os.path.splitext(file)[0]) - info(f"Ensuring service is stopped: {service_name}") - self.__systemctl("stop", service_name) + unit_name = os.path.basename(file) + service_name = os.path.splitext(unit_name)[0] + self.__systemctl("disable", "--now", unit_name) info("Removing service config %s", file) os.unlink(file) @@ -220,7 +241,7 @@ def __unit_names(self, configs, service_names): def start(self, configs=None, service_names=None): """ """ unit_names = self.__unit_names(configs, service_names) - self.__systemctl("start", *unit_names) + self.__systemctl("enable", "--now", *unit_names) def stop(self, configs=None, service_names=None): """ """ @@ -263,7 +284,13 @@ def update(self, configs=None, force=False, **kwargs): def shutdown(self): """ """ - debug(f"SHUTDOWN") + configs = self.config_manager.get_registered_configs(process_manager=self.name) + if self.__use_instance: + instance_name = config["instance_name"] + self.__systemctl("stop", f"galaxy-{instance_name}-*.service") + else: + self.__systemctl("stop", f"galaxy-*.service") - def pm(self): + def pm(self, *args): """ """ + self.__systemctl(*args) diff --git a/tests/conftest.py b/tests/conftest.py index 090a868..14bddac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -123,7 +123,7 @@ def galaxy_yml(galaxy_root_dir): @pytest.fixture() def state_dir(): directory = tempfile.mkdtemp() - os.environ['SYSTEMD_UNIT_PATH'] = os.path.join(directory, 'systemd') + os.environ['GRAVITY_SYSTEMD_UNIT_PATH'] = f"/run/user/{os.getuid()}/systemd/user" try: yield Path(directory) finally: diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index 7568a37..3e2f432 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -1,4 +1,5 @@ import json +import os import string from pathlib import Path @@ -95,7 +96,7 @@ def service_conf_dir(state_dir, process_manager_name): if process_manager_name == 'supervisor': return state_dir / 'supervisor' / 'supervisord.conf.d' / '_default_.d' elif process_manager_name == 'systemd': - return state_dir / 'systemd' + return Path(os.environ.get('GRAVITY_SYSTEMD_UNIT_PATH')) raise Exception(f"Invalid process manager name: {process_manager_name}") @@ -154,11 +155,17 @@ def test_update_force(galaxy_yml, default_config_manager, process_manager_name): def test_cleanup(galaxy_yml, default_config_manager, process_manager_name): test_update(galaxy_yml, default_config_manager, process_manager_name) gunicorn_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'gunicorn') + celery_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'celery') + celery_beat_conf_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'celery-beat') default_config_manager.remove([str(galaxy_yml)]) assert gunicorn_conf_path.exists() + assert celery_conf_path.exists() + assert celery_beat_conf_path.exists() with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() assert not gunicorn_conf_path.exists() + assert not celery_conf_path.exists() + assert not celery_beat_conf_path.exists() @pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) From 3b8607c61037190abcd13b1dc3c86bc6850f5a75 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Mon, 10 Oct 2022 15:15:09 +0200 Subject: [PATCH 12/27] Incomplete lint --- gravity/config_manager.py | 6 ++++-- gravity/process_manager/systemd.py | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 69a75e8..1bbcf40 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -296,8 +296,10 @@ def get_registered_configs(self, instances=None, process_manager=None): rval = [] config_files = self.state.config_files for config_file, config in list(config_files.items()): - if (((instances is not None and config["instance_name"] in instances) or instances is None) and - ((process_manager is not None and config.get("process_manager", "supervisor") == process_manager) or process_manager is None)): + config_pm = config.get("process_manager", "supervisor") + if ((instances is not None and config["instance_name"] in instances) or instances is None) and ( + (process_manager is not None and config_pm == process_manager) or process_manager is None + ): rval.append(self.get_config(config_file)) return rval diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 17c428c..67d36ea 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -224,7 +224,6 @@ def _process_configs(self, configs): for file in (present_configs - intended_configs): unit_name = os.path.basename(file) - service_name = os.path.splitext(unit_name)[0] self.__systemctl("disable", "--now", unit_name) info("Removing service config %s", file) os.unlink(file) @@ -285,11 +284,12 @@ def update(self, configs=None, force=False, **kwargs): def shutdown(self): """ """ configs = self.config_manager.get_registered_configs(process_manager=self.name) + # FIXME: what config below? if self.__use_instance: instance_name = config["instance_name"] self.__systemctl("stop", f"galaxy-{instance_name}-*.service") else: - self.__systemctl("stop", f"galaxy-*.service") + self.__systemctl("stop", "galaxy-*.service") def pm(self, *args): """ """ From 4c8163756fac39ca95af75a2369eacd065823861 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 11 Oct 2022 12:38:32 +0200 Subject: [PATCH 13/27] Lint and fix filtering get_registered_configs by PM --- gravity/config_manager.py | 14 +++++++++----- gravity/process_manager/systemd.py | 13 ++++++------- tests/test_process_manager.py | 3 +++ 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 1bbcf40..8b1b56f 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -296,11 +296,15 @@ def get_registered_configs(self, instances=None, process_manager=None): rval = [] config_files = self.state.config_files for config_file, config in list(config_files.items()): - config_pm = config.get("process_manager", "supervisor") - if ((instances is not None and config["instance_name"] in instances) or instances is None) and ( - (process_manager is not None and config_pm == process_manager) or process_manager is None - ): - rval.append(self.get_config(config_file)) + # if ((instances is not None and config["instance_name"] in instances) or instances is None) and ( + # (process_manager is not None and config_pm == process_manager) or process_manager is None + # ): + # TODO: if we add process_manager to the state, then we can filter for it as above instead of after + # get_config as below + if (instances is not None and config["instance_name"] in instances) or instances is None: + config = self.get_config(config_file) + if (process_manager is not None and config["process_manager"] == process_manager) or process_manager is None: + rval.append(config) return rval def get_registered_config(self, config_file): diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 67d36ea..319a0b1 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -112,6 +112,8 @@ def __unit_name(self, instance_name, service): def __update_service(self, config_file, config, attribs, service, instance_name): unit_name = self.__unit_name(instance_name, service) + # TODO before 1.0.0, most of this should be refactored + # used by the "standalone" service type attach_to_pool_opt = "" server_pools = service.get("server_pools") @@ -137,8 +139,6 @@ def __update_service(self, config_file, config, attribs, service, instance_name) gunicorn_options["preload"] = "--preload" if gunicorn_options["preload"] else "" format_vars = { - #"log_dir": attribs["log_dir"], - #"log_file": self._service_log_file(attribs["log_dir"], program_name), "program_name": service["service_name"], "systemd_user_group": "", "config_type": service["config_type"], @@ -184,7 +184,7 @@ def __update_service(self, config_file, config, attribs, service, instance_name) def follow(self, configs=None, service_names=None, quiet=False): """ """ unit_names = self.__unit_names(configs, service_names) - u_args = [i for l in list(zip(["-u"] * len(unit_names), unit_names)) for i in l] + u_args = [i for sl in list(zip(["-u"] * len(unit_names), unit_names)) for i in sl] self.__journalctl("-f", *u_args) def _process_config(self, config, **kwargs): @@ -283,11 +283,10 @@ def update(self, configs=None, force=False, **kwargs): def shutdown(self): """ """ - configs = self.config_manager.get_registered_configs(process_manager=self.name) - # FIXME: what config below? if self.__use_instance: - instance_name = config["instance_name"] - self.__systemctl("stop", f"galaxy-{instance_name}-*.service") + # we could use galaxy-*.service but this only shuts down the instances managed by *this* gravity + configs = self.config_manager.get_registered_configs(process_manager=self.name) + self.__systemctl("stop", *[f"galaxy-{c.instance_name}-*.service" for c in configs]) else: self.__systemctl("stop", "galaxy-*.service") diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index 3e2f432..a6e8604 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -326,3 +326,6 @@ def test_tusd_process(default_config_manager, galaxy_yml, tusd_config, process_m tusd_config_path = service_conf_path(default_config_manager.state_dir, process_manager_name, 'tusd') assert tusd_config_path.exists() assert "tusd -host" in tusd_config_path.read_text() + + +# TODO: test switching PMs in between invocations, test multiple instances From 4f58259dd8483fa8fbc3afa83b41f7be409adad8 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 11 Oct 2022 18:17:27 +0200 Subject: [PATCH 14/27] Refactoring in process managers, and begin work on passthru exec mode --- gravity/commands/cmd_exec.py | 31 ++++ gravity/config_manager.py | 14 +- gravity/options.py | 4 + gravity/process_manager/__init__.py | 49 +++++++ gravity/process_manager/supervisor.py | 195 ++++---------------------- gravity/process_manager/systemd.py | 86 ++++-------- gravity/settings.py | 8 ++ gravity/state.py | 82 +++++++---- 8 files changed, 209 insertions(+), 260 deletions(-) create mode 100644 gravity/commands/cmd_exec.py diff --git a/gravity/commands/cmd_exec.py b/gravity/commands/cmd_exec.py new file mode 100644 index 0000000..d54f0cf --- /dev/null +++ b/gravity/commands/cmd_exec.py @@ -0,0 +1,31 @@ +import click + +from gravity import options +from gravity import process_manager +from gravity.io import exception + + +@click.command("exec") +@options.instances_services_arg() +@click.pass_context +def cli(ctx, instances_services): + """Run a single Galaxy service, with logging output to stdout/stderr. + + Zero or one instance names can be provided in INSTANCES, it is required if more than one Galaxy instance is + configured in Gravity. + + Exactly one service name is required in SERVICES. + """ + with process_manager.process_manager(state_dir=ctx.parent.state_dir) as pm: + """ + instance_names, service_names = pm._instance_service_names(instances_services) + if len(instance_names) == 0 and pm.config_manager.single_instance: + instance_name = None + elif len(instance_names) != 1: + exception("Only zero or one instance name can be provided") + else: + instance_name = instance_names[0] + if len(service_names) != 1: + exception("Exactly one service_name must be provided") + """ + pm.exec(instance_names=instances_services) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 8b1b56f..f4f8807 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -181,7 +181,9 @@ def get_config(self, conf, defaults=None): config.services.append(service_for_service_type("standalone")( config_type=config.config_type, service_name=handler_settings["service_name"], - environment=handler_settings.get("environment") + environment=handler_settings.get("environment"), + start_timeout=handler_settings.get("start_timeout"), + stop_timeout=handler_settings.get("stop_timeout") )) # FIXME: This should imply explicit configuration of the handler assignment method. If not explicitly set, the @@ -194,16 +196,24 @@ def get_config(self, conf, defaults=None): return config def create_handler_services(self, gravity_config: Settings, config): + # we pull push environment from settings to services but the rest of the services pull their env options from + # settings directly. this can be a bit confusing but is probably ok since there are 3 ways to configure + # handlers, and gravity is only 1 of them. expanded_handlers = self.expand_handlers(gravity_config, config) for service_name, handler_settings in expanded_handlers.items(): pools = handler_settings.get('pools') environment = handler_settings.get("environment") + # TODO: add these to Galaxy docs + start_timeout = handler_settings.get("start_timeout") + stop_timeout = handler_settings.get("stop_timeout") config.services.append( service_for_service_type("standalone")( config_type=config.config_type, service_name=service_name, server_pools=pools, - environment=environment + environment=environment, + start_timeout=start_timeout, + stop_timeout=stop_timeout )) def create_gxit_services(self, gravity_config: Settings, app_config, config): diff --git a/gravity/options.py b/gravity/options.py index 0a4f14c..78908b7 100644 --- a/gravity/options.py +++ b/gravity/options.py @@ -35,3 +35,7 @@ def required_config_arg(name="config", exists=False, nargs=None): def required_instance_arg(): return click.argument("instance", nargs=-1) + + +def instances_services_arg(): + return click.argument("instances_services", metavar="[INSTANCES] [SERVICES]", nargs=-1) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index c6889a8..0dc8ea9 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -70,6 +70,13 @@ def __init__(self, state_dir=None, config_manager=None, start_daemon=True, foreg def _service_log_file(self, log_dir, program_name): return os.path.join(log_dir, program_name + ".log") + def _service_default_path(self): + return os.environ["PATH"] + + @abstractmethod + def _service_environment_formatter(self, environment, format_vars): + raise NotImplementedError() + def _service_program_name(self, instance_name, service): return f"{instance_name}_{service['config_type']}_{service['service_type']}_{service['service_name']}" @@ -81,6 +88,41 @@ def _service_environment(self, service, attribs): environment.update(attribs.get(environment_from, {}).get("environment", {})) return environment + def _service_format_vars(self, config, service, program_name, pm_format_vars): + attribs = config.attribs + virtualenv_dir = attribs.get("virtualenv") + virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" + + format_vars = { + "config_type": service["config_type"], + "server_name": service["service_name"], + "program_name": program_name, + "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], + "galaxy_umask": service.get("umask", "022"), + "galaxy_conf": config.__file__, + "galaxy_root": config["galaxy_root"], + "virtualenv_bin": virtualenv_bin, + "state_dir": self.state_dir, + } + format_vars["settings"] = service.get_settings(attribs, format_vars) + + # update here from PM overrides + format_vars.update(pm_format_vars) + + # template the command template + format_vars["command_arguments"] = service.get_command_arguments(attribs, format_vars) + format_vars["command"] = service.command_template.format(**format_vars) + + # template env vars + environment = self._service_environment(service, attribs) + virtualenv_bin = format_vars["virtualenv_bin"] # could have been changed by pm_format_vars + if virtualenv_bin and service.add_virtualenv_to_path: + path = environment.get("PATH", self._service_default_path()) + environment["PATH"] = ":".join([virtualenv_bin, path]) + format_vars["environment"] = self._service_environment_formatter(environment, format_vars) + + return format_vars + def _file_needs_update(self, path, contents): """Update if contents differ""" if os.path.exists(path): @@ -101,6 +143,9 @@ def _update_file(self, path, contents, name, file_type): else: debug("No changes to existing config for %s %s at %s", file_type, name, path) + def exec(self, configs=None, service_names=None): + pass + def follow(self, configs=None, service_names=None, quiet=False): # supervisor has a built-in tail command but it only works on a single log file. `galaxyctl supervisorctl tail # ...` can be used if desired, though @@ -204,6 +249,10 @@ def _instance_service_names(self, names): exception("No provided names are known instance or service names") return (instance_names, service_names) + @route + def exec(self, instance_names=None): + """ """ + @route def follow(self, instance_names=None, quiet=None): """ """ diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 50f6328..7205375 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -41,48 +41,8 @@ files = supervisord.conf.d/*.d/*.conf supervisord.conf.d/*.conf """ -# TODO: with more templating you only need one of these -SUPERVISORD_SERVICE_TEMPLATES = {} -SUPERVISORD_SERVICE_TEMPLATES["unicornherder"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; - -[program:{program_name}] -command = {command} -directory = {galaxy_root} -umask = {galaxy_umask} -autostart = true -autorestart = true -startsecs = 15 -stopwaitsecs = 65 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" - -SUPERVISORD_SERVICE_TEMPLATES["gunicorn"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; - -[program:{program_name}] -command = {command} -directory = {galaxy_root} -umask = {galaxy_umask} -autostart = true -autorestart = true -startsecs = 15 -stopwaitsecs = 65 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" - -SUPERVISORD_SERVICE_TEMPLATES["celery"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN +SUPERVISORD_SERVICE_TEMPLATE = """; +; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN ; [program:{program_name}] @@ -91,8 +51,8 @@ umask = {galaxy_umask} autostart = true autorestart = true -startsecs = 10 -stopwaitsecs = 10 +startsecs = {settings[start_timeout]} +stopwaitsecs = {settings[stop_timeout]} environment = {environment} numprocs = 1 stdout_logfile = {log_file} @@ -100,79 +60,6 @@ {process_name_opt} """ -SUPERVISORD_SERVICE_TEMPLATES["celery-beat"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; - -[program:{program_name}] -command = {command} -directory = {galaxy_root} -umask = {galaxy_umask} -autostart = true -autorestart = true -startsecs = 10 -stopwaitsecs = 10 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" - -SUPERVISORD_SERVICE_TEMPLATES["tusd"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; -[program:{program_name}] -command = {command} -directory = {galaxy_root} -umask = {galaxy_umask} -autostart = true -autorestart = true -startsecs = 10 -stopwaitsecs = 10 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" - -SUPERVISORD_SERVICE_TEMPLATES["gx-it-proxy"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; - -[program:{program_name}] -command = {command} -directory = {galaxy_root} -umask = {galaxy_umask} -autostart = true -autorestart = true -startsecs = 10 -stopwaitsecs = 10 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" - -SUPERVISORD_SERVICE_TEMPLATES["standalone"] = """; -; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN -; - -[program:{program_name}] -command = {command} -directory = {galaxy_root} -autostart = true -autorestart = true -startsecs = 20 -stopwaitsecs = 65 -environment = {environment} -numprocs = 1 -stdout_logfile = {log_file} -redirect_stderr = true -{process_name_opt} -""" SUPERVISORD_GROUP_TEMPLATE = """; ; This file is maintained by Galaxy - CHANGES WILL BE OVERWRITTEN @@ -248,6 +135,12 @@ def __get_supervisor(self): options.realize(args=["-c", self.supervisord_conf_path]) return supervisorctl.Controller(options).get_supervisor() + def _service_default_path(self): + return "%(ENV_PATH)s" + + def _service_environment_formatter(self, environment, format_vars): + return ",".join("{}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) + def terminate(self): if self.foreground: # if running in foreground, if terminate is called, then supervisord should've already received a SIGINT @@ -259,66 +152,24 @@ def _service_program_name(self, instance_name, service): else: return service["service_name"] - def __update_service(self, config_file, config, attribs, service, instance_conf_dir, instance_name): - if self.use_group: - process_name_opt = f"process_name = {service['service_name']}" - else: - process_name_opt = "" - + def __update_service(self, config, service, instance_conf_dir, instance_name): + attribs = config.attribs program_name = self._service_program_name(instance_name, service) - # TODO: used by the "standalone" service type, refactor this - attach_to_pool_opt = "" - server_pools = service.get("server_pools") - if server_pools: - _attach_to_pool_opt = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) - # Insert a single leading space - attach_to_pool_opt = f" {_attach_to_pool_opt}" - pid_file_opt = f" --pid-file={self.supervisor_state_dir}/{program_name}.pid" - - virtualenv_dir = attribs.get("virtualenv") - virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" - gunicorn_options = attribs["gunicorn"].copy() - gunicorn_options["preload"] = "--preload" if gunicorn_options["preload"] else "" - - format_vars = { + # supervisor-specific format vars + supervisor_format_vars = { "log_dir": attribs["log_dir"], "log_file": self._service_log_file(attribs["log_dir"], program_name), - "config_type": service["config_type"], - "server_name": service["service_name"], - "attach_to_pool_opt": attach_to_pool_opt, - "pid_file_opt": pid_file_opt, - "gunicorn": gunicorn_options, - "celery": attribs["celery"], - "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], - "tusd": attribs["tusd"], - "gx_it_proxy": attribs["gx-it-proxy"], - "galaxy_umask": service.get("umask", "022"), "program_name": program_name, - "process_name_opt": process_name_opt, - "galaxy_conf": config_file, - "galaxy_root": config["galaxy_root"], - "virtualenv_bin": virtualenv_bin, - "state_dir": self.state_dir, + "process_name_opt": f"process_name = {service['service_name']}" if self.use_group else "", } - format_vars["command_arguments"] = service.get_command_arguments(attribs, format_vars) - format_vars["command"] = service.command_template.format(**format_vars) - conf = join(instance_conf_dir, f"{service['config_type']}_{service['service_type']}_{service['service_name']}.conf") - # FIXME: this should be done once, not on every service - if not exists(attribs["log_dir"]): - os.makedirs(attribs["log_dir"]) + format_vars = self._service_format_vars(config, service, program_name, supervisor_format_vars) - template = SUPERVISORD_SERVICE_TEMPLATES.get(service["service_type"]) - if not template: - raise Exception(f"Unknown service type: {service['service_type']}") - - environment = self._service_environment(service, attribs) - if virtualenv_bin and service.add_virtualenv_to_path: - path = environment.get("PATH", "%(ENV_PATH)s") - environment["PATH"] = ":".join([virtualenv_bin, path]) - format_vars["environment"] = ",".join("{}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) + conf = join(instance_conf_dir, f"{service['config_type']}_{service['service_type']}_{service['service_name']}.conf") + # FIXME: dedup below + template = SUPERVISORD_SERVICE_TEMPLATE contents = template.format(**format_vars) service_name = self._service_program_name(instance_name, service) self._update_file(conf, contents, service_name, "service") @@ -331,8 +182,6 @@ def _process_config(self, config): Does not call ``supervisorctl update``. """ instance_name = config["instance_name"] - attribs = config["attribs"] - config_file = config.__file__ instance_conf_dir = join(self.supervisord_conf_dir, f"{instance_name}.d") intended_configs = set() try: @@ -343,7 +192,7 @@ def _process_config(self, config): programs = [] for service in config["services"]: - intended_configs.add(self.__update_service(config_file, config, attribs, service, instance_conf_dir, instance_name)) + intended_configs.add(self.__update_service(config, service, instance_conf_dir, instance_name)) programs.append(f"{instance_name}_{service['config_type']}_{service['service_type']}_{service['service_name']}") # TODO: test group mode @@ -361,6 +210,10 @@ def _process_config(self, config): info("Removing service config %s", file) os.unlink(file) + # ensure log dir exists only if configs exist + if intended_configs and not exists(config.attribs["log_dir"]): + os.makedirs(config.attribs["log_dir"]) + def _remove_invalid_configs(self, valid_configs=None): if not valid_configs: valid_configs = self.config_manager.get_registered_configs(process_manager=self.name) diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 319a0b1..3c23c09 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -11,7 +11,7 @@ from gravity.settings import ProcessManager SYSTEMD_SERVICE_TEMPLATES = {} -DEFAULT_SYSTEMD_SERVICE_TEMPLATE = """; +SYSTEMD_SERVICE_TEMPLATE = """; ; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN ; @@ -25,7 +25,8 @@ Type=simple {systemd_user_group} WorkingDirectory={galaxy_root} -TimeoutStartSec=15 +TimeoutStartSec={settings[start_timeout]} +TimeoutStopSec={settings[stop_timeout]} ExecStart={command} #ExecReload= #ExecStop= @@ -41,13 +42,6 @@ WantedBy=multi-user.target """ -SYSTEMD_SERVICE_TEMPLATES["gunicorn"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE -SYSTEMD_SERVICE_TEMPLATES["celery"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE -SYSTEMD_SERVICE_TEMPLATES["celery-beat"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE -SYSTEMD_SERVICE_TEMPLATES["standalone"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE -SYSTEMD_SERVICE_TEMPLATES["gx-it-proxy"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE -SYSTEMD_SERVICE_TEMPLATES["tusd"] = DEFAULT_SYSTEMD_SERVICE_TEMPLATE - class SystemdProcessManager(BaseProcessManager): @@ -92,12 +86,15 @@ def __journalctl(self, *args, **kwargs): debug("Calling journalctl with args: %s", args) subprocess.check_call(["journalctl"] + args) - def __systemd_env_path(self): + def _service_default_path(self): environ = self.__systemctl("show-environment", capture=True) for line in environ.splitlines(): if line.startswith("PATH="): return line.split("=", 1)[1] + def _service_environment_formatter(self, environment, format_vars): + return "\n".join("Environment={}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) + def terminate(self): # this is used to stop a foreground supervisord in the supervisor PM, so it is a no-op here pass @@ -109,19 +106,11 @@ def __unit_name(self, instance_name, service): unit_name += f"{service['service_name']}.service" return unit_name - def __update_service(self, config_file, config, attribs, service, instance_name): + def __update_service(self, config, service, instance_name): + attribs = config.attribs + program_name = service["service_name"] unit_name = self.__unit_name(instance_name, service) - # TODO before 1.0.0, most of this should be refactored - - # used by the "standalone" service type - attach_to_pool_opt = "" - server_pools = service.get("server_pools") - if server_pools: - _attach_to_pool_opt = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) - # Insert a single leading space - attach_to_pool_opt = f" {_attach_to_pool_opt}" - # under supervisor we expect that gravity is installed in the galaxy venv and the venv is active when gravity # runs, but under systemd this is not the case. we do assume $VIRTUAL_ENV is the galaxy venv if running as an # unprivileged user, though. @@ -134,48 +123,25 @@ def __update_service(self, config_file, config, attribs, service, instance_name) elif not virtualenv_dir: exception("The `virtualenv` Gravity config option must be set when using the systemd process manager") - virtualenv_bin = f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "" - gunicorn_options = attribs["gunicorn"].copy() - gunicorn_options["preload"] = "--preload" if gunicorn_options["preload"] else "" - - format_vars = { - "program_name": service["service_name"], - "systemd_user_group": "", - "config_type": service["config_type"], - "server_name": service["service_name"], - "attach_to_pool_opt": attach_to_pool_opt, - "pid_file_opt": "", - "gunicorn": gunicorn_options, - "celery": attribs["celery"], - "galaxy_infrastructure_url": attribs["galaxy_infrastructure_url"], - "tusd": attribs["tusd"], - "gx_it_proxy": attribs["gx_it_proxy"], - "galaxy_umask": service.get("umask", "022"), - "galaxy_conf": config_file, - "galaxy_root": config["galaxy_root"], - "virtualenv_bin": virtualenv_bin, - "state_dir": self.state_dir, + # systemd-specific format vars + systemd_format_vars = { + "virtualenv_bin": f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "", } - format_vars["command"] = service.command_template.format(**format_vars) - if not format_vars["command"].startswith("/"): - # FIXME: bit of a hack - format_vars["command"] = f"{virtualenv_bin}/{format_vars['command']}" if not self.user_mode: - format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" + systemd_format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" if attribs["galaxy_group"] is not None: - format_vars["systemd_user_group"] += f"\nGroup={attribs['galaxy_group']}" - conf = os.path.join(self.__systemd_unit_dir, unit_name) + systemd_format_vars["systemd_user_group"] += f"\nGroup={attribs['galaxy_group']}" - template = SYSTEMD_SERVICE_TEMPLATES.get(service["service_type"]) - if not template: - raise Exception(f"Unknown service type: {service['service_type']}") + format_vars = self._service_format_vars(config, service, program_name, systemd_format_vars) - environment = self._service_environment(service, attribs) - if virtualenv_bin and service.add_virtualenv_to_path: - path = self.__systemd_env_path() - environment["PATH"] = ":".join([virtualenv_bin, path]) - format_vars["environment"] = "\n".join("Environment={}={}".format(k, shlex.quote(v.format(**format_vars))) for k, v in environment.items()) + # FIXME: bit of a hack + if not format_vars["command"].startswith("/"): + format_vars["command"] = f"{virtualenv_bin}/{format_vars['command']}" + + conf = os.path.join(self.__systemd_unit_dir, unit_name) + # FIXME: dedup below + template = SYSTEMD_SERVICE_TEMPLATE contents = template.format(**format_vars) self._update_file(conf, contents, unit_name, "service") @@ -190,8 +156,6 @@ def follow(self, configs=None, service_names=None, quiet=False): def _process_config(self, config, **kwargs): """ """ instance_name = config["instance_name"] - attribs = config["attribs"] - config_file = config.__file__ intended_configs = set() try: @@ -201,7 +165,7 @@ def _process_config(self, config, **kwargs): raise for service in config["services"]: - intended_configs.add(self.__update_service(config_file, config, attribs, service, instance_name)) + intended_configs.add(self.__update_service(config, service, instance_name)) return intended_configs @@ -278,7 +242,7 @@ def update(self, configs=None, force=False, **kwargs): info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") list(map(os.unlink, service_units)) self._process_configs(configs) - # TODO: only reload if there are changes + # FIXME BEFORE RELEASE: only reload if there are changes self.__systemctl("daemon-reload") def shutdown(self): diff --git a/gravity/settings.py b/gravity/settings.py index 041203a..c98b0a4 100644 --- a/gravity/settings.py +++ b/gravity/settings.py @@ -72,6 +72,8 @@ class TusdSettings(BaseModel): You can find a list of available hooks at https://github.com/tus/tusd/blob/master/docs/hooks.md#list-of-available-hooks. """) extra_args: str = Field(default="", description="Extra arguments to pass to tusd command line.") + start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") + stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") environment: Dict[str, str] = Field( default={}, description=""" @@ -88,6 +90,8 @@ class CelerySettings(BaseModel): queues: str = Field("celery,galaxy.internal,galaxy.external", description="Queues to join") pool: Pool = Field(Pool.threads, description="Pool implementation") extra_args: str = Field(default="", description="Extra arguments to pass to Celery command line.") + start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") + stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") environment: Dict[str, str] = Field( default={}, description=""" @@ -129,6 +133,8 @@ class GunicornSettings(BaseModel): Use Gunicorn's --preload option to fork workers after loading the Galaxy Application. Consumes less memory when multiple processes are configured. Default is ``false`` if using unicornherder, else ``true``. """) + start_timeout: int = Field(15, description="Value of supervisor startsecs, systemd TimeoutStartSec") + stop_timeout: int = Field(65, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") environment: Dict[str, str] = Field( default={}, description=""" @@ -165,6 +171,8 @@ class GxItProxySettings(BaseModel): Rewrite location blocks with proxy port. This is an advanced option that is only needed when proxying to remote interactive tool container that cannot be reached through the local network. """) + start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") + stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") environment: Dict[str, str] = Field( default={}, description=""" diff --git a/gravity/state.py b/gravity/state.py index 93e4bf8..af183b5 100644 --- a/gravity/state.py +++ b/gravity/state.py @@ -31,6 +31,7 @@ class Service(AttributeDict): service_type = "service" service_name = "_default_" environment_from = None + settings_from = None default_environment = {} add_virtualenv_to_path = False graceful_method = GracefulMethod.DEFAULT @@ -57,7 +58,7 @@ def get_environment(self): def get_command_arguments(self, attribs, format_vars): rval = {} - for setting, value in attribs.get(self.service_type, {}).items(): + for setting, value in attribs.get(self.settings_from or self.service_type, {}).items(): if setting in self.command_arguments: # FIXME: this truthiness testing of value is probably not the best if value: @@ -68,20 +69,26 @@ def get_command_arguments(self, attribs, format_vars): rval[setting] = value return rval + def get_settings(self, attribs, format_vars): + return attribs[self.settings_from or self.service_type].copy() + class GalaxyGunicornService(Service): service_type = "gunicorn" service_name = "gunicorn" default_environment = DEFAULT_GALAXY_ENVIRONMENT + command_arguments = { + "preload": "--preload" + } command_template = "{virtualenv_bin}gunicorn 'galaxy.webapps.galaxy.fast_factory:factory()'" \ - " --timeout {gunicorn[timeout]}" \ + " --timeout {settings[timeout]}" \ " --pythonpath lib" \ " -k galaxy.webapps.galaxy.workers.Worker" \ - " -b {gunicorn[bind]}" \ - " --workers={gunicorn[workers]}" \ + " -b {settings[bind]}" \ + " --workers={settings[workers]}" \ " --config python:galaxy.web_stack.gunicorn_config" \ - " {gunicorn[preload]}" \ - " {gunicorn[extra_args]}" + " {command_arguments[preload]}" \ + " {settings[extra_args]}" # TODO: services should maybe have access to settings or attribs, and should maybe template their own command lines def get_graceful_method(self, attribs): @@ -102,18 +109,20 @@ class GalaxyUnicornHerderService(Service): service_type = "unicornherder" service_name = "unicornherder" environment_from = "gunicorn" + settings_from = "gunicorn" graceful_method = GracefulMethod.SIGHUP default_environment = DEFAULT_GALAXY_ENVIRONMENT - command_template = "{virtualenv_bin}unicornherder --pidfile {supervisor_state_dir}/{program_name}.pid --" \ + command_arguments = GalaxyGunicornService.command_arguments + command_template = "{virtualenv_bin}unicornherder --pidfile {state_dir}/{program_name}.pid --" \ " 'galaxy.webapps.galaxy.fast_factory:factory()'" \ - " --timeout {gunicorn[timeout]}" \ + " --timeout {settings[timeout]}" \ " --pythonpath lib" \ " -k galaxy.webapps.galaxy.workers.Worker" \ - " -b {gunicorn[bind]}" \ - " --workers={gunicorn[workers]}" \ + " -b {settings[bind]}" \ + " --workers={settings[workers]}" \ " --config python:galaxy.web_stack.gunicorn_config" \ - " {gunicorn[preload]}" \ - " {gunicorn[extra_args]}" + " {command_arguments[preload]}" \ + " {settings[extra_args]}" def get_environment(self): environment = self.default_environment.copy() @@ -129,21 +138,22 @@ class GalaxyCeleryService(Service): default_environment = DEFAULT_GALAXY_ENVIRONMENT command_template = "{virtualenv_bin}celery" \ " --app galaxy.celery worker" \ - " --concurrency {celery[concurrency]}" \ - " --loglevel {celery[loglevel]}" \ - " --pool {celery[pool]}" \ - " --queues {celery[queues]}" \ - " {celery[extra_args]}" + " --concurrency {settings[concurrency]}" \ + " --loglevel {settings[loglevel]}" \ + " --pool {settings[pool]}" \ + " --queues {settings[queues]}" \ + " {settings[extra_args]}" class GalaxyCeleryBeatService(Service): service_type = "celery-beat" service_name = "celery-beat" + settings_from = "celery" default_environment = DEFAULT_GALAXY_ENVIRONMENT command_template = "{virtualenv_bin}celery" \ " --app galaxy.celery" \ " beat" \ - " --loglevel {celery[loglevel]}" \ + " --loglevel {settings[loglevel]}" \ " --schedule {state_dir}/" + CELERY_BEAT_DB_FILENAME @@ -156,12 +166,12 @@ class GalaxyGxItProxyService(Service): # the npx shebang is $!/usr/bin/env node, so $PATH has to be correct add_virtualenv_to_path = True command_arguments = { - "forward_ip": "--forwardIP {gx_it_proxy[forward_ip]}", - "forward_port": "--forwardPort {gx_it_proxy[forward_port]}", + "forward_ip": "--forwardIP {settings[forward_ip]}", + "forward_port": "--forwardPort {settings[forward_port]}", "reverse_proxy": "--reverseProxy", } - command_template = "{virtualenv_bin}npx gx-it-proxy --ip {gx_it_proxy[ip]} --port {gx_it_proxy[port]}" \ - " --sessions {gx_it_proxy[sessions]} {gx_it_proxy[verbose]}" \ + command_template = "{virtualenv_bin}npx gx-it-proxy --ip {settings[ip]} --port {settings[port]}" \ + " --sessions {settings[sessions]} {settings[verbose]}" \ " {command_arguments[forward_ip]} {command_arguments[forward_port]}" \ " {command_arguments[reverse_proxy]}" @@ -169,21 +179,41 @@ class GalaxyGxItProxyService(Service): class GalaxyTUSDService(Service): service_type = "tusd" service_name = "tusd" - command_template = "{tusd[tusd_path]} -host={tusd[host]} -port={tusd[port]} -upload-dir={tusd[upload_dir]}" \ + command_template = "{settings[tusd_path]} -host={settings[host]} -port={settings[port]}" \ + " -upload-dir={settings[upload_dir]}" \ " -hooks-http={galaxy_infrastructure_url}/api/upload/hooks" \ - " -hooks-http-forward-headers=X-Api-Key,Cookie {tusd[extra_args]}" \ - " -hooks-enabled-events {tusd[hooks_enabled_events]}" + " -hooks-http-forward-headers=X-Api-Key,Cookie {settings[extra_args]}" \ + " -hooks-enabled-events {settings[hooks_enabled_events]}" class GalaxyStandaloneService(Service): service_type = "standalone" service_name = "standalone" + default_start_timeout = 20 + default_stop_timeout = 65 command_template = "{virtualenv_bin}python ./lib/galaxy/main.py -c {galaxy_conf} --server-name={server_name}" \ - "{attach_to_pool_opt}{pid_file_opt}" + " {command_arguments[attach_to_pool]} {command_arguments[pid_file]}" def get_environment(self): return self.get("environment") or {} + def get_command_arguments(self, attribs, format_vars): + # full override because standalone doesn't have settings + command_arguments = {} + server_pools = self.get("server_pools") + if server_pools: + _attach_to_pool = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) + # Insert a single leading space + command_arguments["attach_to_pool"] = f" {_attach_to_pool}" + command_arguments["pid_file"] = " --pid-file={state_dir}/{program_name}.pid".format(**format_vars) + return command_arguments + + def get_settings(self, attribs, format_vars): + return { + "start_timeout": self.start_timeout or self.default_start_timeout, + "stop_timeout": self.stop_timeout or self.default_stop_timeout, + } + class ConfigFile(AttributeDict): persist_keys = ( From a8ac7614e3a771f18a86ed09ad73e340faeebcdb Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 11 Oct 2022 23:26:54 +0200 Subject: [PATCH 15/27] Pass-through execution mode with `galaxyctl exec ` --- gravity/commands/cmd_exec.py | 16 +------ gravity/config_manager.py | 7 +++ gravity/process_manager/__init__.py | 65 ++++++++++++++++++++++----- gravity/process_manager/supervisor.py | 3 +- gravity/process_manager/systemd.py | 6 +-- gravity/state.py | 6 ++- 6 files changed, 69 insertions(+), 34 deletions(-) diff --git a/gravity/commands/cmd_exec.py b/gravity/commands/cmd_exec.py index d54f0cf..54c9c27 100644 --- a/gravity/commands/cmd_exec.py +++ b/gravity/commands/cmd_exec.py @@ -2,30 +2,18 @@ from gravity import options from gravity import process_manager -from gravity.io import exception @click.command("exec") @options.instances_services_arg() @click.pass_context def cli(ctx, instances_services): - """Run a single Galaxy service, with logging output to stdout/stderr. + """Run a single Galaxy service in the foreground, with logging output to stdout/stderr. Zero or one instance names can be provided in INSTANCES, it is required if more than one Galaxy instance is configured in Gravity. Exactly one service name is required in SERVICES. """ - with process_manager.process_manager(state_dir=ctx.parent.state_dir) as pm: - """ - instance_names, service_names = pm._instance_service_names(instances_services) - if len(instance_names) == 0 and pm.config_manager.single_instance: - instance_name = None - elif len(instance_names) != 1: - exception("Only zero or one instance name can be provided") - else: - instance_name = instance_names[0] - if len(service_names) != 1: - exception("Exactly one service_name must be provided") - """ + with process_manager.process_manager(state_dir=ctx.parent.state_dir, start_daemon=False) as pm: pm.exec(instance_names=instances_services) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index f4f8807..993d307 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -323,6 +323,13 @@ def get_registered_config(self, config_file): return self.get_config(config_file) return None + def get_configured_service_names(self): + rval = set() + for config in self.get_registered_configs(): + for service in config["services"]: + rval.add(service["service_name"]) + return rval + def get_registered_instance_names(self): return [c["instance_name"] for c in self.state.config_files.values()] diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 0dc8ea9..a21fa08 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -5,6 +5,7 @@ import importlib import inspect import os +import shlex import subprocess from abc import ABCMeta, abstractmethod from functools import partial, wraps @@ -60,22 +61,21 @@ def decorator(self, *args, instance_names=None, **kwargs): route_to_all = partial(_route, all_process_managers=True) -class BaseProcessManager(metaclass=ABCMeta): - +class BaseProcessExecutionEnvironment(metaclass=ABCMeta): def __init__(self, state_dir=None, config_manager=None, start_daemon=True, foreground=False): self.config_manager = config_manager or ConfigManager(state_dir=state_dir) self.state_dir = self.config_manager.state_dir self.tail = which("tail") - def _service_log_file(self, log_dir, program_name): - return os.path.join(log_dir, program_name + ".log") + @abstractmethod + def _service_environment_formatter(self, environment, format_vars): + raise NotImplementedError() def _service_default_path(self): return os.environ["PATH"] - @abstractmethod - def _service_environment_formatter(self, environment, format_vars): - raise NotImplementedError() + def _service_log_file(self, log_dir, program_name): + return os.path.join(log_dir, program_name + ".log") def _service_program_name(self, instance_name, service): return f"{instance_name}_{service['config_type']}_{service['service_type']}_{service['service_name']}" @@ -123,6 +123,8 @@ def _service_format_vars(self, config, service, program_name, pm_format_vars): return format_vars + +class BaseProcessManager(BaseProcessExecutionEnvironment, metaclass=ABCMeta): def _file_needs_update(self, path, contents): """Update if contents differ""" if os.path.exists(path): @@ -143,9 +145,6 @@ def _update_file(self, path, contents, name, file_type): else: debug("No changes to existing config for %s %s at %s", file_type, name, path) - def exec(self, configs=None, service_names=None): - pass - def follow(self, configs=None, service_names=None, quiet=False): # supervisor has a built-in tail command but it only works on a single log file. `galaxyctl supervisorctl tail # ...` can be used if desired, though @@ -216,11 +215,33 @@ def pm(self, *args, **kwargs): """Direct pass-thru to process manager.""" +class ProcessExecutor(BaseProcessExecutionEnvironment): + def _service_environment_formatter(self, environment, format_vars): + return {k: v.format(**format_vars) for k, v in environment.items()} + + def exec(self, config, service): + service_name = service["service_name"] + + format_vars = self._service_format_vars(config, service, service_name, {}) + print_env = ' '.join('{}={}'.format(k, shlex.quote(v)) for k, v in format_vars["environment"].items()) + + cmd = shlex.split(format_vars["command"]) + env = format_vars["environment"] | os.environ + cwd = format_vars["galaxy_root"] + + info(f"Working directory: {cwd}") + info(f"Executing: {print_env} {format_vars['command']}") + + os.chdir(cwd) + os.execvpe(cmd[0], cmd, env) + + class ProcessManagerRouter: def __init__(self, state_dir=None, **kwargs): self.config_manager = ConfigManager(state_dir=state_dir) self.state_dir = self.config_manager.state_dir self._load_pm_modules(state_dir=state_dir, **kwargs) + self._process_executor = ProcessExecutor(config_manager=self.config_manager) def _load_pm_modules(self, *args, **kwargs): self.process_managers = {} @@ -237,11 +258,12 @@ def _instance_service_names(self, names): instance_names = [] service_names = [] registered_instance_names = self.config_manager.get_registered_instance_names() + configured_service_names = self.config_manager.get_configured_service_names() if names: for name in names: if name in registered_instance_names: instance_names.append(name) - elif name in VALID_SERVICE_NAMES: + elif name in configured_service_names | VALID_SERVICE_NAMES: service_names.append(name) else: warn(f"Warning: Not a known instance or service name: {name}") @@ -249,9 +271,28 @@ def _instance_service_names(self, names): exception("No provided names are known instance or service names") return (instance_names, service_names) - @route def exec(self, instance_names=None): """ """ + instance_names, service_names = self._instance_service_names(instance_names) + + if len(instance_names) == 0 and self.config_manager.single_instance: + instance_names = None + elif len(instance_names) != 1: + exception("Only zero or one instance name can be provided") + + config = self.config_manager.get_registered_configs(instances=instance_names)[0] + service_list = ", ".join(s["service_name"] for s in config["services"]) + + if len(service_names) != 1: + exception(f"Exactly one service name must be provided. Configured service(s): {service_list}") + + service_name = service_names[0] + services = [s for s in config["services"] if s["service_name"] == service_name] + if not services: + exception(f"Service '{service_name}' is not configured. Configured service(s): {service_list}") + + service = services[0] + return self._process_executor.exec(config, service) @route def follow(self, instance_names=None, quiet=None): diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 7205375..12c4e66 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -171,8 +171,7 @@ def __update_service(self, config, service, instance_conf_dir, instance_name): # FIXME: dedup below template = SUPERVISORD_SERVICE_TEMPLATE contents = template.format(**format_vars) - service_name = self._service_program_name(instance_name, service) - self._update_file(conf, contents, service_name, "service") + self._update_file(conf, contents, program_name, "service") return conf diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 3c23c09..e9afcfe 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -126,6 +126,7 @@ def __update_service(self, config, service, instance_name): # systemd-specific format vars systemd_format_vars = { "virtualenv_bin": f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "", + "systemd_user_group": "", } if not self.user_mode: systemd_format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" @@ -134,13 +135,10 @@ def __update_service(self, config, service, instance_name): format_vars = self._service_format_vars(config, service, program_name, systemd_format_vars) - # FIXME: bit of a hack if not format_vars["command"].startswith("/"): - format_vars["command"] = f"{virtualenv_bin}/{format_vars['command']}" + format_vars["command"] = f"{format_vars['virtualenv_bin']}/{format_vars['command']}" conf = os.path.join(self.__systemd_unit_dir, unit_name) - - # FIXME: dedup below template = SYSTEMD_SERVICE_TEMPLATE contents = template.format(**format_vars) self._update_file(conf, contents, unit_name, "service") diff --git a/gravity/state.py b/gravity/state.py index af183b5..e12cfd9 100644 --- a/gravity/state.py +++ b/gravity/state.py @@ -199,13 +199,15 @@ def get_environment(self): def get_command_arguments(self, attribs, format_vars): # full override because standalone doesn't have settings - command_arguments = {} + command_arguments = { + "attach_to_pool": "", + "pid_file": " --pid-file={state_dir}/{program_name}.pid".format(**format_vars), + } server_pools = self.get("server_pools") if server_pools: _attach_to_pool = " ".join(f"--attach-to-pool={server_pool}" for server_pool in server_pools) # Insert a single leading space command_arguments["attach_to_pool"] = f" {_attach_to_pool}" - command_arguments["pid_file"] = " --pid-file={state_dir}/{program_name}.pid".format(**format_vars) return command_arguments def get_settings(self, attribs, format_vars): From 60ed175cb5b6625968d4cf0008f2692760e6fadc Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 11 Oct 2022 23:42:17 +0200 Subject: [PATCH 16/27] Move preload graceful method tests to a more proper location in config manager tests, and fix. --- tests/test_config_manager.py | 21 +++++++++++++++++++++ tests/test_process_manager.py | 29 ----------------------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index 3610444..4669826 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -3,6 +3,7 @@ from gravity import __version__, config_manager from gravity.settings import Settings +from gravity.state import GracefulMethod def test_register_defaults(galaxy_yml, galaxy_root_dir, state_dir, default_config_manager): @@ -116,4 +117,24 @@ def test_convert_0_x_config(state_dir, galaxy_yml, configstate_yaml_0_x): assert "attribs" not in config +def test_gunicorn_graceful_method_preload(galaxy_yml, default_config_manager): + default_config_manager.add([str(galaxy_yml)]) + config = default_config_manager.get_registered_config(str(galaxy_yml)) + gunicorn_service = [s for s in config["services"] if s["service_name"] == "gunicorn"][0] + graceful_method = gunicorn_service.get_graceful_method(config["attribs"]) + assert graceful_method == GracefulMethod.DEFAULT + + +def test_gunicorn_graceful_method_no_preload(galaxy_yml, default_config_manager): + galaxy_yml.write(json.dumps( + {'galaxy': None, 'gravity': { + 'gunicorn': {'preload': False}}} + )) + default_config_manager.add([str(galaxy_yml)]) + config = default_config_manager.get_registered_config(str(galaxy_yml)) + gunicorn_service = [s for s in config["services"] if s["service_name"] == "gunicorn"][0] + graceful_method = gunicorn_service.get_graceful_method(config["attribs"]) + assert graceful_method == GracefulMethod.SIGHUP + + # TODO: tests for switching process managers between supervisor and systemd diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index a6e8604..48d68d6 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -5,7 +5,6 @@ import pytest from gravity import process_manager -from gravity.state import GracefulMethod from yaml import safe_load @@ -188,34 +187,6 @@ def test_disable_services(galaxy_yml, default_config_manager, process_manager_na assert not celery_beat_conf_path.exists() -def test_gunicorn_graceful_method_preload(galaxy_yml, default_config_manager): - instance_name = '_default_' - default_config_manager.add([str(galaxy_yml)]) - with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: - pm.update() - config = default_config_manager.get_instance_config(instance_name) - services = default_config_manager.get_instance_services(instance_name) - gunicorn_service = [s for s in services if s["service_name"] == "gunicorn"][0] - graceful_method = gunicorn_service.get_graceful_method(config["attribs"]) - assert graceful_method == GracefulMethod.DEFAULT - - -def test_gunicorn_graceful_method_no_preload(galaxy_yml, default_config_manager): - instance_name = '_default_' - default_config_manager.add([str(galaxy_yml)]) - galaxy_yml.write(json.dumps( - {'galaxy': None, 'gravity': { - 'gunicorn': {'preload': False}}} - )) - with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: - pm.update() - config = default_config_manager.get_instance_config(instance_name) - services = default_config_manager.get_instance_services(instance_name) - gunicorn_service = [s for s in services if s["service_name"] == "gunicorn"][0] - graceful_method = gunicorn_service.get_graceful_method(config["attribs"]) - assert graceful_method == GracefulMethod.SIGHUP - - @pytest.mark.parametrize('job_conf', [params["JOB_CONF_XML_DYNAMIC_HANDLERS"]], indirect=True) @pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) def test_dynamic_handlers(default_config_manager, galaxy_yml, job_conf, process_manager_name): From 48f982b46ca827e7123d3200c995918d6c0816ed Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 12 Oct 2022 00:27:01 +0200 Subject: [PATCH 17/27] Support for `galaxyctl exec` in systemd and supervisor configs, make it the default, tests. --- gravity/commands/cmd_start.py | 6 ++++-- gravity/config_manager.py | 1 + gravity/process_manager/__init__.py | 25 ++++++++++++++--------- gravity/process_manager/systemd.py | 2 +- gravity/settings.py | 13 ++++++++++++ tests/conftest.py | 2 ++ tests/test_process_manager.py | 31 ++++++++++++++++++++++++----- 7 files changed, 63 insertions(+), 17 deletions(-) diff --git a/gravity/commands/cmd_start.py b/gravity/commands/cmd_start.py index ba127b5..1059d3e 100644 --- a/gravity/commands/cmd_start.py +++ b/gravity/commands/cmd_start.py @@ -31,7 +31,9 @@ def cli(ctx, foreground, instance, quiet=False): pm.follow(instance_names=instance, quiet=quiet) elif pm.config_manager.single_instance: config = list(pm.config_manager.get_registered_configs())[0] - info(f"Log files are in {config.attribs['log_dir']}") + if config.process_manager != "systemd": + info(f"Log files are in {config.attribs['log_dir']}") else: for config in pm.config_manager.get_registered_configs(instances=instance or None): - info(f"Log files for {config.instance_name} are in {config.attribs['log_dir']}") + if config.process_manager != "systemd": + info(f"Log files for {config.instance_name} are in {config.attribs['log_dir']}") diff --git a/gravity/config_manager.py b/gravity/config_manager.py index 993d307..f676e44 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -125,6 +125,7 @@ def get_config(self, conf, defaults=None): config.instance_name = gravity_config.instance_name config.config_type = server_section config.process_manager = gravity_config.process_manager + config.service_command_style = gravity_config.service_command_style # FIXME: should this be attribs? config.attribs["galaxy_infrastructure_url"] = app_config.get("galaxy_infrastructure_url", "").rstrip("/") if gravity_config.tusd.enable and not config.attribs["galaxy_infrastructure_url"]: diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index a21fa08..28cf694 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -12,6 +12,7 @@ from gravity.config_manager import ConfigManager from gravity.io import debug, exception, info, warn +from gravity.settings import ServiceCommandStyle from gravity.state import VALID_SERVICE_NAMES from gravity.util import which @@ -110,15 +111,19 @@ def _service_format_vars(self, config, service, program_name, pm_format_vars): format_vars.update(pm_format_vars) # template the command template - format_vars["command_arguments"] = service.get_command_arguments(attribs, format_vars) - format_vars["command"] = service.command_template.format(**format_vars) - - # template env vars - environment = self._service_environment(service, attribs) - virtualenv_bin = format_vars["virtualenv_bin"] # could have been changed by pm_format_vars - if virtualenv_bin and service.add_virtualenv_to_path: - path = environment.get("PATH", self._service_default_path()) - environment["PATH"] = ":".join([virtualenv_bin, path]) + if config.service_command_style == ServiceCommandStyle.direct: + format_vars["command_arguments"] = service.get_command_arguments(attribs, format_vars) + format_vars["command"] = service.command_template.format(**format_vars) + + # template env vars + environment = self._service_environment(service, attribs) + virtualenv_bin = format_vars["virtualenv_bin"] # could have been changed by pm_format_vars + if virtualenv_bin and service.add_virtualenv_to_path: + path = environment.get("PATH", self._service_default_path()) + environment["PATH"] = ":".join([virtualenv_bin, path]) + else: + format_vars["command"] = f"galaxyctl exec {config.instance_name} {program_name}" + environment = {"GRAVITY_STATE_DIR": "{state_dir}"} format_vars["environment"] = self._service_environment_formatter(environment, format_vars) return format_vars @@ -222,6 +227,8 @@ def _service_environment_formatter(self, environment, format_vars): def exec(self, config, service): service_name = service["service_name"] + # force generation of real commands + config.service_command_style = ServiceCommandStyle.direct format_vars = self._service_format_vars(config, service, service_name, {}) print_env = ' '.join('{}={}'.format(k, shlex.quote(v)) for k, v in format_vars["environment"].items()) diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index e9afcfe..9f88958 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -136,7 +136,7 @@ def __update_service(self, config, service, instance_name): format_vars = self._service_format_vars(config, service, program_name, systemd_format_vars) if not format_vars["command"].startswith("/"): - format_vars["command"] = f"{format_vars['virtualenv_bin']}/{format_vars['command']}" + format_vars["command"] = f"{format_vars['virtualenv_bin']}{format_vars['command']}" conf = os.path.join(self.__systemd_unit_dir, unit_name) template = SYSTEMD_SERVICE_TEMPLATE diff --git a/gravity/settings.py b/gravity/settings.py index c98b0a4..097e2e0 100644 --- a/gravity/settings.py +++ b/gravity/settings.py @@ -33,6 +33,11 @@ class ProcessManager(str, Enum): systemd = "systemd" +class ServiceCommandStyle(str, Enum): + gravity = "gravity" + direct = "direct" + + class AppServer(str, Enum): gunicorn = "gunicorn" unicornherder = "unicornherder" @@ -193,6 +198,14 @@ class Settings(BaseSettings): Process manager to use. ``supervisor`` is the default process manager. ``systemd`` is also supported. +""") + + service_command_style: ServiceCommandStyle = Field( + ServiceCommandStyle.gravity, + description=""" +What command to write to the process manager configs +`gravity` (`galaxyctl exec `) is the default +`direct` (each service's actual command) is also supported. """) galaxy_root: Optional[str] = Field( diff --git a/tests/conftest.py b/tests/conftest.py index 14bddac..3aef666 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,7 @@ GXIT_CONFIG = """ gravity: process_manager: {process_manager_name} + service_command_style: direct gunicorn: bind: 'localhost:{gx_port}' gx_it_proxy: @@ -201,6 +202,7 @@ def gxit_config(free_port, another_free_port, process_manager_name): def tusd_config(startup_config, free_port, another_free_port, process_manager_name): startup_config["gravity"] = { "process_manager": process_manager_name, + "service_command_style": "direct", "tusd": {"enable": True, "port": another_free_port, "upload_dir": "/tmp"}} startup_config["galaxy"]["galaxy_infrastructure_url"] = f"http://localhost:{free_port}" return startup_config diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index 48d68d6..cfa98b1 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -60,6 +60,7 @@ DYNAMIC_HANDLER_CONFIG = """ gravity: process_manager: %(process_manager_name)s + service_command_style: direct handlers: handler: processes: 2 @@ -238,6 +239,28 @@ def test_static_handlers(default_config_manager, galaxy_yml, job_conf, process_m 'gravity': {'process_manager': process_manager_name}, 'galaxy': {'job_config_file': str(job_conf)}})) default_config_manager.add([str(galaxy_yml)]) + with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: + pm.update() + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler0', service_type='standalone') + assert handler0_config_path.exists() + assert 'exec _default_ handler0' in handler0_config_path.open().read() + handler1_config_path = conf_dir / service_conf_file(process_manager_name, 'handler1', service_type='standalone') + assert handler1_config_path.exists() + handler1_config = handler1_config_path.open().read() + assert 'exec _default_ handler1' in handler1_config + for handler_name in ('sge_handler', 'special_handler0', 'special_handler1'): + assert (conf_dir / service_conf_file(process_manager_name, handler_name, service_type='standalone')).exists() + + +@pytest.mark.parametrize('job_conf', [params["JOB_CONF_YAML_STATIC_HANDLERS"]], indirect=True) +@pytest.mark.parametrize('process_manager_name', ['supervisor', 'systemd']) +def test_static_handlers_direct(default_config_manager, galaxy_yml, job_conf, process_manager_name): + with open(galaxy_yml, 'w') as config_fh: + config_fh.write(json.dumps({ + 'gravity': {'process_manager': process_manager_name, 'service_command_style': 'direct'}, + 'galaxy': {'job_config_file': str(job_conf)}})) + default_config_manager.add([str(galaxy_yml)]) with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: pm.update() conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) @@ -248,9 +271,7 @@ def test_static_handlers(default_config_manager, galaxy_yml, job_conf, process_m assert handler1_config_path.exists() handler1_config = handler1_config_path.open().read() assert '.yml --server-name=handler1' in handler1_config - if job_conf.ext == '.yml': - # setting env vars on static handlers is only supported with YAML job conf - assert 'BAZ=baz' in handler1_config + assert 'BAZ=baz' in handler1_config for handler_name in ('sge_handler', 'special_handler0', 'special_handler1'): assert (conf_dir / service_conf_file(process_manager_name, handler_name, service_type='standalone')).exists() @@ -267,10 +288,10 @@ def test_static_handlers_embedded_in_galaxy_yml(default_config_manager, galaxy_y conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler0', service_type='standalone') assert handler0_config_path.exists() - assert '.yml --server-name=handler0' in handler0_config_path.open().read() + assert 'exec _default_ handler0' in handler0_config_path.open().read() handler1_config_path = conf_dir / service_conf_file(process_manager_name, 'handler1', service_type='standalone') assert handler1_config_path.exists() - assert '.yml --server-name=handler1' in handler1_config_path.open().read() + assert 'exec _default_ handler1' in handler1_config_path.open().read() for handler_name in ('sge_handler', 'special_handler0', 'special_handler1'): assert (conf_dir / service_conf_file(process_manager_name, handler_name, service_type='standalone')).exists() From a7e0ac39bbcc2131d0619f2cd0c55a347319ba2d Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 12 Oct 2022 13:08:47 +0200 Subject: [PATCH 18/27] Convert os.environ to a dict for union --- gravity/process_manager/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 28cf694..3b8c93d 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -233,7 +233,7 @@ def exec(self, config, service): print_env = ' '.join('{}={}'.format(k, shlex.quote(v)) for k, v in format_vars["environment"].items()) cmd = shlex.split(format_vars["command"]) - env = format_vars["environment"] | os.environ + env = format_vars["environment"] | dict(os.environ) cwd = format_vars["galaxy_root"] info(f"Working directory: {cwd}") From fe73afd32d3c07e9bee6b21d2a7d02d293aa601a Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 12 Oct 2022 14:54:45 +0200 Subject: [PATCH 19/27] Fix syntax for 3.7 --- gravity/process_manager/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 3b8c93d..33bc567 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -233,7 +233,7 @@ def exec(self, config, service): print_env = ' '.join('{}={}'.format(k, shlex.quote(v)) for k, v in format_vars["environment"].items()) cmd = shlex.split(format_vars["command"]) - env = format_vars["environment"] | dict(os.environ) + env = {**format_vars["environment"], **dict(os.environ)} cwd = format_vars["galaxy_root"] info(f"Working directory: {cwd}") From 68a12468c264f9d634c5ef97ee6e81fca3e96df6 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 18 Oct 2022 11:49:56 -0400 Subject: [PATCH 20/27] Update usage of inspect to preferred (non-deprecated) Python 3 method --- gravity/process_manager/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 33bc567..27d1f9d 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -44,7 +44,7 @@ def decorator(self, *args, instance_names=None, **kwargs): pm_names = configs_by_pm.keys() for pm_name in pm_names: routed_func = getattr(self.process_managers[pm_name], func.__name__) - routed_func_params = inspect.getargspec(routed_func).args + routed_func_params = list(inspect.signature(routed_func).parameters) if "configs" in routed_func_params: pm_configs = configs_by_pm.get(pm_name, []) kwargs["configs"] = pm_configs From 83daa807f02680155b61c49712b780ad2c8bafc1 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 18 Oct 2022 11:51:08 -0400 Subject: [PATCH 21/27] Set systemd ExecReload to HUP when it should be used, enable setting memory limits globally or on individual services. --- gravity/config_manager.py | 6 +++- gravity/process_manager/systemd.py | 16 +++++++-- gravity/settings.py | 40 ++++++++++++++++++++-- tests/test_process_manager.py | 55 ++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 6 deletions(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index f676e44..dd5b2d4 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -139,6 +139,7 @@ def get_config(self, conf, defaults=None): config.attribs["handlers"] = gravity_config.handlers config.attribs["galaxy_user"] = gravity_config.galaxy_user config.attribs["galaxy_group"] = gravity_config.galaxy_group + config.attribs["memory_limit"] = gravity_config.memory_limit # Store gravity version, in case we need to convert old setting webapp_service_names = [] @@ -183,6 +184,7 @@ def get_config(self, conf, defaults=None): config_type=config.config_type, service_name=handler_settings["service_name"], environment=handler_settings.get("environment"), + memory_limit=handler_settings.get("memory_limit"), start_timeout=handler_settings.get("start_timeout"), stop_timeout=handler_settings.get("stop_timeout") )) @@ -207,6 +209,7 @@ def create_handler_services(self, gravity_config: Settings, config): # TODO: add these to Galaxy docs start_timeout = handler_settings.get("start_timeout") stop_timeout = handler_settings.get("stop_timeout") + memory_limit = handler_settings.get("memory_limit") config.services.append( service_for_service_type("standalone")( config_type=config.config_type, @@ -214,7 +217,8 @@ def create_handler_services(self, gravity_config: Settings, config): server_pools=pools, environment=environment, start_timeout=start_timeout, - stop_timeout=stop_timeout + stop_timeout=stop_timeout, + memory_limit=memory_limit )) def create_gxit_services(self, gravity_config: Settings, app_config, config): diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 9f88958..54946e4 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -9,6 +9,7 @@ from gravity.io import debug, exception, info, warn from gravity.process_manager import BaseProcessManager from gravity.settings import ProcessManager +from gravity.state import GracefulMethod SYSTEMD_SERVICE_TEMPLATES = {} SYSTEMD_SERVICE_TEMPLATE = """; @@ -28,10 +29,9 @@ TimeoutStartSec={settings[start_timeout]} TimeoutStopSec={settings[stop_timeout]} ExecStart={command} -#ExecReload= -#ExecStop= +{systemd_exec_reload} {environment} -#MemoryLimit= +{systemd_memory_limit} Restart=always MemoryAccounting=yes @@ -127,14 +127,24 @@ def __update_service(self, config, service, instance_name): systemd_format_vars = { "virtualenv_bin": f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "", "systemd_user_group": "", + "systemd_exec_reload": "", + "systemd_memory_limit": "", } if not self.user_mode: systemd_format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" if attribs["galaxy_group"] is not None: systemd_format_vars["systemd_user_group"] += f"\nGroup={attribs['galaxy_group']}" + if service.get_graceful_method(attribs) == GracefulMethod.SIGHUP: + systemd_format_vars["systemd_exec_reload"] = "ExecReload=/bin/kill -HUP $MAINPID" + format_vars = self._service_format_vars(config, service, program_name, systemd_format_vars) + # this is done after the superclass formatting since it accesses service settings + memory_limit = format_vars["settings"].get("memory_limit") or attribs["memory_limit"] + if memory_limit: + format_vars["systemd_memory_limit"] = f"MemoryLimit={memory_limit}G" + if not format_vars["command"].startswith("/"): format_vars["command"] = f"{format_vars['virtualenv_bin']}{format_vars['command']}" diff --git a/gravity/settings.py b/gravity/settings.py index 097e2e0..591588f 100644 --- a/gravity/settings.py +++ b/gravity/settings.py @@ -79,6 +79,13 @@ class TusdSettings(BaseModel): extra_args: str = Field(default="", description="Extra arguments to pass to tusd command line.") start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") + memory_limit: Optional[int] = Field( + None, + description=""" +Memory limit (in GB). If the service exceeds the limit, it will be killed. Default is no limit or the value of the +``memory_limit`` setting at the top level of the Gravity configuration, if set. Ignored if ``process_manager`` is +``supervisor``. +""") environment: Dict[str, str] = Field( default={}, description=""" @@ -97,6 +104,13 @@ class CelerySettings(BaseModel): extra_args: str = Field(default="", description="Extra arguments to pass to Celery command line.") start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") + memory_limit: Optional[int] = Field( + None, + description=""" +Memory limit (in GB). If the service exceeds the limit, it will be killed. Default is no limit or the value of the +``memory_limit`` setting at the top level of the Gravity configuration, if set. Ignored if ``process_manager`` is +``supervisor``. +""") environment: Dict[str, str] = Field( default={}, description=""" @@ -140,6 +154,13 @@ class GunicornSettings(BaseModel): """) start_timeout: int = Field(15, description="Value of supervisor startsecs, systemd TimeoutStartSec") stop_timeout: int = Field(65, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") + memory_limit: Optional[int] = Field( + None, + description=""" +Memory limit (in GB). If the service exceeds the limit, it will be killed. Default is no limit or the value of the +``memory_limit`` setting at the top level of the Gravity configuration, if set. Ignored if ``process_manager`` is +``supervisor``. +""") environment: Dict[str, str] = Field( default={}, description=""" @@ -178,6 +199,13 @@ class GxItProxySettings(BaseModel): """) start_timeout: int = Field(10, description="Value of supervisor startsecs, systemd TimeoutStartSec") stop_timeout: int = Field(10, description="Value of supervisor stopwaitsecs, systemd TimeoutStopSec") + memory_limit: Optional[int] = Field( + None, + description=""" +Memory limit (in GB). If the service exceeds the limit, it will be killed. Default is no limit or the value of the +``memory_limit`` setting at the top level of the Gravity configuration, if set. Ignored if ``process_manager`` is +``supervisor``. +""") environment: Dict[str, str] = Field( default={}, description=""" @@ -206,6 +234,14 @@ class Settings(BaseSettings): What command to write to the process manager configs `gravity` (`galaxyctl exec `) is the default `direct` (each service's actual command) is also supported. +""") + + memory_limit: Optional[int] = Field( + None, + description=""" +Memory limit (in GB), processes exceeding the limit will be killed. Default is no limit. If set, this is default value +for all services. Setting ``memory_limit`` on an individual service overrides this value. Ignored if ``process_manager`` +is ``supervisor``. """) galaxy_root: Optional[str] = Field( @@ -218,13 +254,13 @@ class Settings(BaseSettings): None, description=""" User to run Galaxy as, required when using the systemd process manager as root. -Ignored with supervisor or user-mode systemd. +Ignored if ``process_manager`` is ``supervisor`` or user-mode (non-root) ``systemd``. """) galaxy_group: Optional[str] = Field( None, description=""" Group to run Galaxy as, optional when using the systemd process manager as root. -Ignored with supervisor or user-mode systemd. +Ignored if ``process_manager`` is ``supervisor`` or user-mode (non-root) ``systemd``. """) log_dir: Optional[str] = Field( None, diff --git a/tests/test_process_manager.py b/tests/test_process_manager.py index cfa98b1..5442207 100644 --- a/tests/test_process_manager.py +++ b/tests/test_process_manager.py @@ -320,4 +320,59 @@ def test_tusd_process(default_config_manager, galaxy_yml, tusd_config, process_m assert "tusd -host" in tusd_config_path.read_text() +def test_default_memory_limit(galaxy_yml, default_config_manager): + process_manager_name = 'systemd' + galaxy_yml.write(json.dumps( + {'galaxy': None, 'gravity': { + 'process_manager': process_manager_name, + 'memory_limit': 2, + 'handlers': {'handler': {}}}})) + default_config_manager.add([str(galaxy_yml)]) + with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: + pm.update() + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + gunicorn_conf_path = conf_dir / service_conf_file(process_manager_name, 'gunicorn') + assert 'MemoryLimit=2G' in gunicorn_conf_path.open().read() + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler_0', service_type='standalone') + assert handler0_config_path.exists(), os.listdir(conf_dir) + assert 'MemoryLimit=2G' in handler0_config_path.open().read() + + +def test_service_memory_limit(galaxy_yml, default_config_manager): + process_manager_name = 'systemd' + galaxy_yml.write(json.dumps( + {'galaxy': None, 'gravity': { + 'process_manager': process_manager_name, + 'gunicorn': {'memory_limit': 4}, + 'handlers': {'handler': {}}}})) + default_config_manager.add([str(galaxy_yml)]) + with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: + pm.update() + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + gunicorn_conf_path = conf_dir / service_conf_file(process_manager_name, 'gunicorn') + assert 'MemoryLimit=4G' in gunicorn_conf_path.open().read() + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler_0', service_type='standalone') + assert handler0_config_path.exists(), os.listdir(conf_dir) + assert 'MemoryLimit' not in handler0_config_path.open().read() + + +def test_override_memory_limit(galaxy_yml, default_config_manager): + process_manager_name = 'systemd' + galaxy_yml.write(json.dumps( + {'galaxy': None, 'gravity': { + 'process_manager': process_manager_name, + 'memory_limit': 2, + 'gunicorn': {'memory_limit': 4}, + 'handlers': {'handler': {}}}})) + default_config_manager.add([str(galaxy_yml)]) + with process_manager.process_manager(state_dir=default_config_manager.state_dir) as pm: + pm.update() + conf_dir = service_conf_dir(default_config_manager.state_dir, process_manager_name) + gunicorn_conf_path = conf_dir / service_conf_file(process_manager_name, 'gunicorn') + assert 'MemoryLimit=4G' in gunicorn_conf_path.open().read() + handler0_config_path = conf_dir / service_conf_file(process_manager_name, 'handler_0', service_type='standalone') + assert handler0_config_path.exists(), os.listdir(conf_dir) + assert 'MemoryLimit=2G' in handler0_config_path.open().read() + + # TODO: test switching PMs in between invocations, test multiple instances From 2676da5b12b482698b74e2bcf1d833caa4d7c8f2 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 18 Oct 2022 12:49:40 -0400 Subject: [PATCH 22/27] Only run systemctl daemon-reload if there are changes to service units --- gravity/process_manager/__init__.py | 5 +++++ gravity/process_manager/supervisor.py | 1 - gravity/process_manager/systemd.py | 8 ++++++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 27d1f9d..94a2644 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -130,6 +130,10 @@ def _service_format_vars(self, config, service, program_name, pm_format_vars): class BaseProcessManager(BaseProcessExecutionEnvironment, metaclass=ABCMeta): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._service_changes = None + def _file_needs_update(self, path, contents): """Update if contents differ""" if os.path.exists(path): @@ -147,6 +151,7 @@ def _update_file(self, path, contents, name, file_type): info("%s %s %s", verb, file_type, name) with open(path, "w") as out: out.write(contents) + self._service_changes = True else: debug("No changes to existing config for %s %s at %s", file_type, name, path) diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 12c4e66..2c5acbb 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -168,7 +168,6 @@ def __update_service(self, config, service, instance_conf_dir, instance_name): conf = join(instance_conf_dir, f"{service['config_type']}_{service['service_type']}_{service['service_name']}.conf") - # FIXME: dedup below template = SUPERVISORD_SERVICE_TEMPLATE contents = template.format(**format_vars) self._update_file(conf, contents, program_name, "service") diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index 54946e4..a0f2d27 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -199,6 +199,7 @@ def _process_configs(self, configs): self.__systemctl("disable", "--now", unit_name) info("Removing service config %s", file) os.unlink(file) + self._service_changes = True def __unit_names(self, configs, service_names): unit_names = [] @@ -249,9 +250,12 @@ def update(self, configs=None, force=False, **kwargs): newline = '\n' info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") list(map(os.unlink, service_units)) + self._service_changes = True self._process_configs(configs) - # FIXME BEFORE RELEASE: only reload if there are changes - self.__systemctl("daemon-reload") + if self._service_changes: + self.__systemctl("daemon-reload") + else: + debug("No service changes, daemon-reload not performed") def shutdown(self): """ """ From 2f9379474b3b6f6660aae0f28691d5b61f0252e3 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 19 Oct 2022 14:59:28 -0400 Subject: [PATCH 23/27] Actually check the value of galaxy_user when validating --- gravity/settings.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gravity/settings.py b/gravity/settings.py index 591588f..f671f1e 100644 --- a/gravity/settings.py +++ b/gravity/settings.py @@ -309,9 +309,10 @@ class Settings(BaseSettings): @validator("galaxy_user") def _user_required_if_root(cls, v, values): if os.geteuid() == 0: - if values["process_manager"] == ProcessManager.systemd: + is_systemd = values["process_manager"] == ProcessManager.systemd + if is_systemd and not v: raise ValueError("galaxy_user is required when running as root") - else: + elif not is_systemd: raise ValueError("Gravity cannot be run as root unless using the systemd process manager") return v From 3a7d35537a5e4c1d6bf90fb89162735b5f3741a7 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 19 Oct 2022 15:00:19 -0400 Subject: [PATCH 24/27] Ignore test venv --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index cf17867..ad99a0c 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ coverage.xml /*.egg-info/ *.egg tests/galaxy.git +tests/galaxy_venv docs/_build # dev scripts From 7fd65789b4e8a97b8d88cf15e0bac8160f3d0204 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 19 Oct 2022 17:05:20 -0400 Subject: [PATCH 25/27] systemd target support --- gravity/process_manager/__init__.py | 2 + gravity/process_manager/supervisor.py | 1 - gravity/process_manager/systemd.py | 73 ++++++++++++++++++++++----- 3 files changed, 61 insertions(+), 15 deletions(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index 94a2644..b7c9729 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -152,8 +152,10 @@ def _update_file(self, path, contents, name, file_type): with open(path, "w") as out: out.write(contents) self._service_changes = True + return True else: debug("No changes to existing config for %s %s at %s", file_type, name, path) + return False def follow(self, configs=None, service_names=None, quiet=False): # supervisor has a built-in tail command but it only works on a single log file. `galaxyctl supervisorctl tail diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 2c5acbb..104c87e 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -160,7 +160,6 @@ def __update_service(self, config, service, instance_conf_dir, instance_name): supervisor_format_vars = { "log_dir": attribs["log_dir"], "log_file": self._service_log_file(attribs["log_dir"], program_name), - "program_name": program_name, "process_name_opt": f"process_name = {service['service_name']}" if self.use_group else "", } diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index a0f2d27..bc13e7c 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -11,15 +11,15 @@ from gravity.settings import ProcessManager from gravity.state import GracefulMethod -SYSTEMD_SERVICE_TEMPLATES = {} SYSTEMD_SERVICE_TEMPLATE = """; ; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN ; [Unit] -Description=Galaxy {program_name} +Description={systemd_description} After=network.target After=time-sync.target +PartOf={systemd_target} [Service] UMask={galaxy_umask} @@ -42,6 +42,20 @@ WantedBy=multi-user.target """ +SYSTEMD_TARGET_TEMPLATE = """; +; This file is maintained by Gravity - CHANGES WILL BE OVERWRITTEN +; + +[Unit] +Description={systemd_description} +After=network.target +After=time-sync.target +Wants={systemd_target_wants} + +[Install] +WantedBy=multi-user.target +""" + class SystemdProcessManager(BaseProcessManager): @@ -99,11 +113,13 @@ def terminate(self): # this is used to stop a foreground supervisord in the supervisor PM, so it is a no-op here pass - def __unit_name(self, instance_name, service): - unit_name = f"{service['config_type']}-" + def __unit_name(self, instance_name, service, unit_type="service"): + unit_name = f"{service['config_type']}" if self.__use_instance: - unit_name += f"{instance_name}-" - unit_name += f"{service['service_name']}.service" + unit_name += f"-{instance_name}" + if unit_type == "service": + unit_name += f"-{service['service_name']}" + unit_name += f".{unit_type}" return unit_name def __update_service(self, config, service, instance_name): @@ -123,12 +139,19 @@ def __update_service(self, config, service, instance_name): elif not virtualenv_dir: exception("The `virtualenv` Gravity config option must be set when using the systemd process manager") + if self.__use_instance: + description = f"{config['config_type'].capitalize()} {instance_name} {program_name}" + else: + description = f"{config['config_type'].capitalize()} {program_name}" + # systemd-specific format vars systemd_format_vars = { "virtualenv_bin": f'{os.path.join(virtualenv_dir, "bin")}{os.path.sep}' if virtualenv_dir else "", "systemd_user_group": "", "systemd_exec_reload": "", "systemd_memory_limit": "", + "systemd_description": description, + "systemd_target": self.__unit_name(instance_name, config, unit_type="target"), } if not self.user_mode: systemd_format_vars["systemd_user_group"] = f"User={attribs['galaxy_user']}" @@ -157,7 +180,7 @@ def __update_service(self, config, service, instance_name): def follow(self, configs=None, service_names=None, quiet=False): """ """ - unit_names = self.__unit_names(configs, service_names) + unit_names = self.__unit_names(configs, service_names, use_target=False) u_args = [i for sl in list(zip(["-u"] * len(unit_names), unit_names)) for i in sl] self.__journalctl("-f", *u_args) @@ -172,8 +195,26 @@ def _process_config(self, config, **kwargs): if exc.errno != errno.EEXIST: raise + service_units = [] for service in config["services"]: - intended_configs.add(self.__update_service(config, service, instance_name)) + conf = self.__update_service(config, service, instance_name) + intended_configs.add(conf) + service_units.append(os.path.basename(conf)) + + # create systemd target + target_unit_name = self.__unit_name(instance_name, config, unit_type="target") + target_conf = os.path.join(self.__systemd_unit_dir, target_unit_name) + format_vars = { + "systemd_description": config["config_type"].capitalize(), + "systemd_target_wants": " ".join(service_units), + } + if self.__use_instance: + format_vars["systemd_description"] += f" {instance_name}" + contents = SYSTEMD_TARGET_TEMPLATE.format(**format_vars) + updated = self._update_file(target_conf, contents, instance_name, "systemd target unit") + intended_configs.add(target_conf) + if updated: + self.__systemctl("enable", target_unit_conf) return intended_configs @@ -190,22 +231,26 @@ def _process_configs(self, configs): # FIXME: should use config_type, but that's per-service _present_configs = filter( - lambda f: f.startswith("galaxy-") and f.endswith(".service"), + lambda f: f.startswith("galaxy-") and (f.endswith(".service") or f.endswith(".target")), os.listdir(self.__systemd_unit_dir)) present_configs = set([os.path.join(self.__systemd_unit_dir, f) for f in _present_configs]) for file in (present_configs - intended_configs): unit_name = os.path.basename(file) self.__systemctl("disable", "--now", unit_name) - info("Removing service config %s", file) + info("Removing systemd config %s", file) os.unlink(file) self._service_changes = True - def __unit_names(self, configs, service_names): + def __unit_names(self, configs, service_names, use_target=True, include_services=False): unit_names = [] for config in configs: services = config.services - if service_names: + if not service_names and use_target: + unit_names.append(self.__unit_name(config.instance_name, config, unit_type="target")) + if not include_services: + services = [] + elif service_names: services = [s for s in config.services if s["service_name"] in service_names] unit_names.extend([self.__unit_name(config.instance_name, s) for s in services]) return unit_names @@ -213,7 +258,7 @@ def __unit_names(self, configs, service_names): def start(self, configs=None, service_names=None): """ """ unit_names = self.__unit_names(configs, service_names) - self.__systemctl("enable", "--now", *unit_names) + self.__systemctl("start", *unit_names) def stop(self, configs=None, service_names=None): """ """ @@ -237,7 +282,7 @@ def graceful(self, configs=None, service_names=None): def status(self, configs=None, service_names=None): """ """ - unit_names = self.__unit_names(configs, service_names) + unit_names = self.__unit_names(configs, service_names, include_services=True) self.__systemctl("status", "--lines=0", *unit_names, ignore_rc=(3,)) def update(self, configs=None, force=False, **kwargs): From 55123252d5d003d17fe250a7ee2648df715064ca Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 19 Oct 2022 18:21:07 -0400 Subject: [PATCH 26/27] Force interactivetools_enable to be set if gx_it_proxy.enable is set. --- gravity/config_manager.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gravity/config_manager.py b/gravity/config_manager.py index dd5b2d4..b628e27 100644 --- a/gravity/config_manager.py +++ b/gravity/config_manager.py @@ -222,7 +222,10 @@ def create_handler_services(self, gravity_config: Settings, config): )) def create_gxit_services(self, gravity_config: Settings, app_config, config): - if app_config.get("interactivetools_enable") and gravity_config.gx_it_proxy.enable: + interactivetools_enable = app_config.get("interactivetools_enable") + if gravity_config.gx_it_proxy.enable and not interactivetools_enable: + exception("To run the gx-it-proxy server you need to set interactivetools_enable in the galaxy section of galaxy.yml") + if gravity_config.gx_it_proxy.enable: # TODO: resolve against data_dir, or bring in galaxy-config ? # CWD in supervisor template is galaxy_root, so this should work for simple cases as is gxit_config = gravity_config.gx_it_proxy From 8825c8aaabf822b1beb9983d15798758e5497e97 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 19 Oct 2022 18:21:36 -0400 Subject: [PATCH 27/27] systemd target and graceful fixes, drop the reload() method on PMs which odesn't actually exist, it's graceful() --- gravity/process_manager/__init__.py | 8 ----- gravity/process_manager/supervisor.py | 7 ++--- gravity/process_manager/systemd.py | 43 ++++++++++++++------------- 3 files changed, 25 insertions(+), 33 deletions(-) diff --git a/gravity/process_manager/__init__.py b/gravity/process_manager/__init__.py index b7c9729..2dada37 100644 --- a/gravity/process_manager/__init__.py +++ b/gravity/process_manager/__init__.py @@ -198,10 +198,6 @@ def stop(self, configs=None, service_names=None): def restart(self, configs=None, service_names=None): """ """ - @abstractmethod - def reload(self, configs=None, service_names=None): - """ """ - @abstractmethod def graceful(self, configs=None, service_names=None): """ """ @@ -324,10 +320,6 @@ def stop(self, instance_names=None): def restart(self, instance_names=None): """ """ - @route - def reload(self, instance_names=None): - """ """ - @route def graceful(self, instance_names=None): """ """ diff --git a/gravity/process_manager/supervisor.py b/gravity/process_manager/supervisor.py index 104c87e..b4a6338 100644 --- a/gravity/process_manager/supervisor.py +++ b/gravity/process_manager/supervisor.py @@ -240,7 +240,7 @@ def __start_stop(self, op, configs, service_names): target = f"{config.instance_name}:*" if self.use_group else "all" self.supervisorctl(op, target) - def __reload_graceful(self, op, configs, service_names): + def __reload_graceful(self, configs, service_names): self.update(configs=configs) for config in configs: if service_names: @@ -272,11 +272,8 @@ def stop(self, configs=None, service_names=None): def restart(self, configs=None, service_names=None): self.__start_stop("restart", configs, service_names) - def reload(self, configs=None, service_names=None): - self.__reload_graceful("reload", configs, service_names) - def graceful(self, configs=None, service_names=None): - self.__reload_graceful("graceful", configs, service_names) + self.__reload_graceful(configs, service_names) def status(self, configs=None, service_names=None): # TODO: create our own formatted output diff --git a/gravity/process_manager/systemd.py b/gravity/process_manager/systemd.py index bc13e7c..ce01674 100644 --- a/gravity/process_manager/systemd.py +++ b/gravity/process_manager/systemd.py @@ -6,7 +6,7 @@ import subprocess from glob import glob -from gravity.io import debug, exception, info, warn +from gravity.io import debug, error, exception, info, warn from gravity.process_manager import BaseProcessManager from gravity.settings import ProcessManager from gravity.state import GracefulMethod @@ -174,7 +174,7 @@ def __update_service(self, config, service, instance_name): conf = os.path.join(self.__systemd_unit_dir, unit_name) template = SYSTEMD_SERVICE_TEMPLATE contents = template.format(**format_vars) - self._update_file(conf, contents, unit_name, "service") + self._update_file(conf, contents, unit_name, "systemd unit") return conf @@ -211,10 +211,10 @@ def _process_config(self, config, **kwargs): if self.__use_instance: format_vars["systemd_description"] += f" {instance_name}" contents = SYSTEMD_TARGET_TEMPLATE.format(**format_vars) - updated = self._update_file(target_conf, contents, instance_name, "systemd target unit") + updated = self._update_file(target_conf, contents, target_unit_name, "systemd unit") intended_configs.add(target_conf) if updated: - self.__systemctl("enable", target_unit_conf) + self.__systemctl("enable", target_conf) return intended_configs @@ -270,31 +270,35 @@ def restart(self, configs=None, service_names=None): unit_names = self.__unit_names(configs, service_names) self.__systemctl("restart", *unit_names) - def reload(self, configs=None, service_names=None): - """ """ - unit_names = self.__unit_names(configs, service_names) - self.__systemctl("reload", *unit_names) - def graceful(self, configs=None, service_names=None): """ """ - unit_names = self.__unit_names(configs, service_names) - self.__systemctl("reload", *unit_names) + # reload-or-restart on a target does a restart on its services, so we use the services directly + unit_names = self.__unit_names(configs, service_names, use_target=False) + self.__systemctl("reload-or-restart", *unit_names) def status(self, configs=None, service_names=None): """ """ unit_names = self.__unit_names(configs, service_names, include_services=True) - self.__systemctl("status", "--lines=0", *unit_names, ignore_rc=(3,)) + try: + self.__systemctl("status", "--lines=0", *unit_names, ignore_rc=(3,)) + except subprocess.CalledProcessError as exc: + if exc.returncode == 4: + error("Some expected systemd units were not found, did you forget to run `galaxyctl update`?") + else: + raise def update(self, configs=None, force=False, **kwargs): """ """ if force: for config in configs: - service_units = glob(os.path.join(self.__systemd_unit_dir, f"{config.config_type}-*.service")) - # TODO: would need to add targets here assuming we add one - if service_units: + units = (glob(os.path.join(self.__systemd_unit_dir, f"{config.config_type}-*.service")) + + glob(os.path.join(self.__systemd_unit_dir, f"{config.config_type}-*.target")) + + glob(os.path.join(self.__systemd_unit_dir, f"{config.config_type}.target"))) + if units: newline = '\n' - info(f"Removing systemd units due to --force option:{newline}{newline.join(service_units)}") - list(map(os.unlink, service_units)) + info(f"Removing systemd units due to --force option:{newline}{newline.join(units)}") + [self.__systemctl("disable", os.path.basename(u)) for u in units] + list(map(os.unlink, units)) self._service_changes = True self._process_configs(configs) if self._service_changes: @@ -305,11 +309,10 @@ def update(self, configs=None, force=False, **kwargs): def shutdown(self): """ """ if self.__use_instance: - # we could use galaxy-*.service but this only shuts down the instances managed by *this* gravity configs = self.config_manager.get_registered_configs(process_manager=self.name) - self.__systemctl("stop", *[f"galaxy-{c.instance_name}-*.service" for c in configs]) + self.__systemctl("stop", *[f"galaxy-{c.instance_name}.target" for c in configs]) else: - self.__systemctl("stop", "galaxy-*.service") + self.__systemctl("stop", "galaxy.target") def pm(self, *args): """ """