From c8791a98340acd28566a818244bba4c886fdd68e Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 4 Sep 2021 20:21:30 +0200 Subject: [PATCH] Add config check during service_reload_or_restart --- src/yunohost/app.py | 14 ++++---------- src/yunohost/service.py | 25 ++++++++++++++++++++++++- src/yunohost/tests/test_service.py | 22 ++++++++++++++++++++++ src/yunohost/utils/config.py | 14 +++----------- 4 files changed, 53 insertions(+), 22 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 83ff27cdf..23f3b011b 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -52,7 +52,6 @@ from moulinette.utils.filesystem import ( mkdir, ) -from yunohost.service import service_status, _run_service_command from yunohost.utils import packages from yunohost.utils.config import ( ConfigPanel, @@ -424,6 +423,7 @@ def app_change_url(operation_logger, app, domain, path): """ from yunohost.hook import hook_exec, hook_callback + from yunohost.service import service_reload_or_restart installed = _is_installed(app) if not installed: @@ -492,15 +492,7 @@ def app_change_url(operation_logger, app, domain, path): app_ssowatconf() - # avoid common mistakes - if _run_service_command("reload", "nginx") is False: - # grab nginx errors - # the "exit 0" is here to avoid check_output to fail because 'nginx -t' - # will return != 0 since we are in a failed state - nginx_errors = check_output("nginx -t; exit 0") - raise YunohostError( - "app_change_url_failed_nginx_reload", nginx_errors=nginx_errors - ) + service_reload_or_restart("nginx") logger.success(m18n.n("app_change_url_success", app=app, domain=domain, path=path)) @@ -2883,6 +2875,8 @@ def unstable_apps(): def _assert_system_is_sane_for_app(manifest, when): + from yunohost.service import service_status + logger.debug("Checking that required services are up and running...") services = manifest.get("services", []) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index fb12e9053..da3bded3c 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -256,7 +256,7 @@ def service_restart(names): ) -def service_reload_or_restart(names): +def service_reload_or_restart(names, test_conf=True): """ Reload one or more services if they support it. If not, restart them instead. If the services are not running yet, they will be started. @@ -266,7 +266,30 @@ def service_reload_or_restart(names): """ if isinstance(names, str): names = [names] + + services = _get_services() + for name in names: + + logger.debug(f"Reloading service {name}") + + test_conf_cmd = services.get(service, {}).get("test_conf") + if test_conf and test_conf_cmd: + + p = subprocess.Popen( + test_conf_cmd, + shell=True, + executable="/bin/bash", + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + ) + + out, _ = p.communicate() + if p.returncode != 0: + errors = out.strip().split("\n") + logger.error(m18n.("service_not_reloading_because_conf_broken", errors=errors)) + continue + if _run_service_command("reload-or-restart", name): logger.success(m18n.n("service_reloaded_or_restarted", service=name)) else: diff --git a/src/yunohost/tests/test_service.py b/src/yunohost/tests/test_service.py index 1f82dc8fd..1007419a1 100644 --- a/src/yunohost/tests/test_service.py +++ b/src/yunohost/tests/test_service.py @@ -9,6 +9,7 @@ from yunohost.service import ( service_add, service_remove, service_log, + service_reload_or_restart, ) @@ -38,6 +39,10 @@ def clean(): _save_services(services) + if os.path.exists("/etc/nginx/conf.d/broken.conf"): + os.remove("/etc/nginx/conf.d/broken.conf") + os.system("systemctl reload-or-restart nginx") + def test_service_status_all(): @@ -118,3 +123,20 @@ def test_service_update_to_remove_properties(): assert _get_services()["dummyservice"].get("test_status") == "false" service_add("dummyservice", description="dummy", test_status="") assert not _get_services()["dummyservice"].get("test_status") + + +def test_service_conf_broken(): + + os.system("echo pwet > /etc/nginx/conf.d/broken.conf") + + status = service_status("nginx") + assert status["status"] == "running" + assert status["configuration"] == "broken" + assert "broken.conf" in status["configuration-details"] + + # Service reload-or-restart should check that the conf ain't valid + # before reload-or-restart, hence the service should still be running + service_reload_or_restart("nginx") + assert status["status"] == "running" + + os.remove("/etc/nginx/conf.d/broken.conf") diff --git a/src/yunohost/utils/config.py b/src/yunohost/utils/config.py index 8fcf493ed..9cdb30119 100644 --- a/src/yunohost/utils/config.py +++ b/src/yunohost/utils/config.py @@ -289,7 +289,7 @@ class ConfigPanel: def _reload_services(self): - from yunohost.service import _run_service_command, _get_services + from yunohost.service import service_reload_or_restart logger.info("Reloading services...") services_to_reload = set() @@ -299,16 +299,8 @@ class ConfigPanel: services_to_reload = list(services_to_reload) services_to_reload.sort(key="nginx".__eq__) for service in services_to_reload: - if "__APP__": - service = service.replace("__APP__", self.app) - logger.debug(f"Reloading {service}") - if not _run_service_command("reload-or-restart", service): - services = _get_services() - test_conf = services[service].get("test_conf", "true") - errors = check_output(f"{test_conf}; exit 0") if test_conf else "" - raise YunohostError( - "config_failed_service_reload", service=service, errors=errors - ) + service = service.replace("__APP__", self.app) + service_reload_or_restart(service) def _iterate(self, trigger=["option"]): for panel in self.config.get("panels", []):