Add config check during service_reload_or_restart

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", [])

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:

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

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", []):