diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 7ca926c4dac..3df096799bd 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -10,6 +10,10 @@ on: - README* - '**.md' - changes-entries/* + - 'modules/arch/unix/**' + - 'os/unix/**' + - server/mpm_unix.c + - 'test/modules/arch/linux/**' tags: - 2.* pull_request: @@ -21,6 +25,10 @@ on: - README* - '**.md' - changes-entries/* + - 'modules/arch/unix/**' + - 'os/unix/**' + - server/mpm_unix.c + - 'test/modules/arch/linux/**' permissions: contents: read diff --git a/docs/manual/mod/mod_systemd.xml b/docs/manual/mod/mod_systemd.xml index 3db140d584f..ee5a7857fa6 100644 --- a/docs/manual/mod/mod_systemd.xml +++ b/docs/manual/mod/mod_systemd.xml @@ -32,7 +32,7 @@

This module provides support for systemd integration. It allows httpd to be used in a service with the systemd - Type=notify (see Type=notify or Type=notify-reload (see systemd.service(5) for more information). The module is activated if loaded.

@@ -69,6 +69,53 @@ WantedBy=multi-user.target href="https://www.freedesktop.org/software/systemd/man/systemd.kill.html">systemd.kill(5) for more information.

+

A service manager from systemd 253 onwards offers + Type=notify-reload, which is worth using in preference. + Under Type=notify a systemctl reload + returns as soon as the ExecReload command has sent its + signal, which is before the new configuration has been read, and it + reports success whatever becomes of the restart afterwards. + Type=notify-reload instead holds the reload open until + the server reports it finished, so the command waits for the new + configuration to be in use and fails if it never is. mod_systemd + sends the RELOADING=1 notification the protocol expects + while the configuration is being read, stamped with the + MONOTONIC_USEC the service manager requires, and + READY=1 once it has been loaded.

+ + + Example of a service unit which reloads synchronously + 
+[Service]
+Type=notify-reload
+ReloadSignal=SIGCONT
+ExecStart=/usr/local/apache2/bin/httpd -D FOREGROUND -k start
+ExecReload=/usr/local/apache2/bin/httpd -k graceful
+KillMode=mixed
+
+ + +

The service manager runs ExecReload first, and sends + the signal named by ReloadSignal only once that command + has exited successfully. Keeping ExecReload is what + makes the reload safe: httpd -k graceful parses the new + configuration in a process of its own and exits without signalling + anything if it does not parse, so the reload fails and the running + server carries on with the configuration it already has. Leaving + ExecReload out, and letting the service manager signal + the server directly, gives up that check: the running parent reads + the new configuration itself, and a configuration which does not + parse makes it exit, taking the server down.

+ +

The signal sent after ExecReload has run is then + redundant, so ReloadSignal should name one httpd does + not act on, such as SIGCONT. It matters that it is set: + the default is SIGHUP, which httpd takes as an + ungraceful restart, dropping the connections a reload is + meant to preserve. A unit which does leave out + ExecReload must set ReloadSignal=SIGUSR1, + the signal httpd restarts gracefully on.

+

Systemd socket activation is supported if httpd was built with it. Each Listen port must then be one passed in by systemd; a port which was not is a @@ -99,7 +146,7 @@ WantedBy=multi-user.target WatchdogSec= of at least 20 seconds.

- Adding watchdog supervision to the unit above + Adding watchdog supervision to either unit above 
 [Service]
 WatchdogSec=30
diff --git a/test/modules/arch/linux/README b/test/modules/arch/linux/README
index f8037431e91..4bdd7af91d1 100644
--- a/test/modules/arch/linux/README
+++ b/test/modules/arch/linux/README
@@ -27,8 +27,8 @@ None of that is observable over HTTP, so the tests observe it directly.
 How the tests run without systemd, and without privileges
 ---------------------------------------------------------
 There is no need for a service manager to exercise the protocol.  Only
-test_005, and the last test of test_006, involve systemd at all; the rest
-run anywhere, and need nothing from the systemd package beyond the
+test_005, the last test of test_006, and test_007 involve systemd at all;
+the rest run anywhere, and need nothing from the systemd package beyond the
 libsystemd httpd itself is linked against.
 
   test_001_notify.py   $NOTIFY_SOCKET is an ordinary AF_UNIX datagram
@@ -78,13 +78,23 @@ libsystemd httpd itself is linked against.
                        reported STATUS= as the unit's status text.
                        Skipped when the user has no systemd manager.
 
+  test_007_notify_reload.py
+                       The reload protocol, as a transient
+                       Type=notify-reload unit: that "systemctl reload"
+                       waits for the new configuration to be in use, that a
+                       configuration which does not parse fails the reload
+                       without disturbing the running server, and that a
+                       reload restarts it gracefully and only once.  Also
+                       skipped where systemd is older than 253, which is
+                       where Type=notify-reload arrived.
+
 The whole package is skipped unless mod_systemd was built, which needs
 configure --enable-systemd.  A static module is enough for everything
 except the test which has to leave mod_systemd out of the configuration;
 that one needs --enable-systemd=shared.
 
-Running the last suite where there is no user session
------------------------------------------------------
+Running the systemd suites where there is no user session
+---------------------------------------------------------
 "systemctl --user" needs a per-user manager, which a login session has but
 a CI container does not; enabling lingering needs privileges.  Where that
 is a problem, run the tests inside a container with systemd as pid 1,
diff --git a/test/modules/arch/linux/env.py b/test/modules/arch/linux/env.py
index 7caf74fe996..ce1c2464f27 100644
--- a/test/modules/arch/linux/env.py
+++ b/test/modules/arch/linux/env.py
@@ -286,6 +286,25 @@ def http_responds(port: int, timeout: float = 2.0) -> bool:
         return False
 
 
+def error_log_size(env: SystemdTestEnv) -> int:
+    """Where the shared error log ends now, so that a later read can take
+    only what one operation wrote."""
+    try:
+        return os.path.getsize(env.httpd_error_log.path)
+    except OSError:
+        return 0
+
+
+def error_log_since(env: SystemdTestEnv, pos: int) -> str:
+    """The error log written since error_log_size() returned pos."""
+    try:
+        with open(env.httpd_error_log.path, errors='replace') as fd:
+            fd.seek(pos)
+            return fd.read()
+    except OSError:
+        return ''
+
+
 class ActivatedServer:
     """An httpd handed a listening socket the way a service manager does.
 
@@ -445,6 +464,24 @@ def __exit__(self, *args):
         self.stop()
 
 
+# Type=notify-reload and ReloadSignal= both arrived in systemd 253.  An
+# unrecognised Type= does not degrade to anything, it stops the unit loading
+# at all, so there is nothing to fall back to and the tests are skipped.
+NOTIFY_RELOAD_VERSION = 253
+
+
+def systemd_version() -> int:
+    """The version of the systemd on this host, or 0 if it cannot be asked.
+    "systemctl --version" opens with "systemd 259 (259.8-1.fc44)"."""
+    try:
+        p = subprocess.run(['systemctl', '--version'], capture_output=True,
+                           text=True, timeout=15)
+    except (OSError, subprocess.TimeoutExpired):
+        return 0
+    m = re.match(r'systemd (\d+)', p.stdout)
+    return int(m.group(1)) if m else 0
+
+
 class TransientService:
     """httpd run as a real transient systemd unit, with systemd-run.
 
@@ -454,17 +491,24 @@ class TransientService:
     tracks MAINPID, and shows the reported STATUS= as the unit's status
     text.  It needs a per-user service manager, which a login session has
     but a bare CI container does not.
+
+    service_type selects what is exercised: "notify" for startup and
+    shutdown, "notify-reload" for the reload protocol on top of them.
     """
 
     def __init__(self, env: SystemdTestEnv, port: int,
                  name: str = None, extra: str = '',
+                 service_type: str = 'notify', exec_reload: bool = True,
                  properties: List[str] = None):
         self.env = env
         self.port = port
         self.unit = name or f'httpd-test-{os.getpid()}'
+        self.service_type = service_type
+        self.exec_reload = exec_reload
         self.conf_file = write_server_conf(env, 'transient', port, extra=extra)
         self.pid_file = os.path.join(env.server_logs_dir, 'transient.pid')
-        # Extra --property arguments for the unit, such as WatchdogSec=.
+        # Extra --property arguments for the unit, such as WatchdogSec= or
+        # ReloadSignal=.
         self.properties = list(properties or [])
 
     def read_pid(self) -> Optional[int]:
@@ -490,9 +534,10 @@ def is_available() -> bool:
         except (OSError, subprocess.TimeoutExpired):
             return False
 
-    def systemctl(self, *args) -> subprocess.CompletedProcess:
+    def systemctl(self, *args,
+                  timeout: float = 60.0) -> subprocess.CompletedProcess:
         return subprocess.run(['systemctl', '--user', *args],
-                              capture_output=True, text=True)
+                              capture_output=True, text=True, timeout=timeout)
 
     def show(self, prop: str) -> str:
         r = self.systemctl('show', '-p', prop, '--value', f'{self.unit}.service')
@@ -500,19 +545,40 @@ def show(self, prop: str) -> str:
 
     def start(self, timeout: float = 20.0) -> subprocess.CompletedProcess:
         httpd = self.env.httpd_bin
+        props = [
+            f'--service-type={self.service_type}',
+            '--property=KillMode=mixed',
+            # A reload which is never reported finished holds the job open
+            # until this elapses, so keep it to the same bound as the start.
+            f'--property=TimeoutStartSec={int(timeout)}',
+        ]
+        if self.exec_reload:
+            props.append(f'--property=ExecReload={httpd} '
+                         f'-d {self.env.server_dir} '
+                         f'-f {self.conf_file} -k graceful')
+        props += [f'--property={p}' for p in self.properties]
         r = subprocess.run([
             'systemd-run', '--user', '--collect', '--quiet',
-            '--unit', self.unit,
-            '--service-type=notify',
-            '--property=KillMode=mixed',
-            f'--property=ExecReload={httpd} -d {self.env.server_dir} '
-            f'-f {self.conf_file} -k graceful',
-            *[f'--property={p}' for p in self.properties],
+            '--unit', self.unit, *props,
             httpd, '-DFOREGROUND',
             '-d', self.env.server_dir, '-f', self.conf_file,
         ], capture_output=True, text=True, timeout=timeout)
         return r
 
+    def reload(self, timeout: float = 60.0) -> subprocess.CompletedProcess:
+        """Ask the manager to reload the unit.  Under Type=notify-reload
+        this returns once the server has reported the reload finished;
+        under Type=notify, as soon as ExecReload= has exited."""
+        return self.systemctl('reload', f'{self.unit}.service',
+                              timeout=timeout)
+
+    def rewrite_conf(self, extra: str = ''):
+        """Replace the configuration the unit reads on its next reload.
+        The path does not change, so ExecStart= and ExecReload= still name
+        it."""
+        self.conf_file = write_server_conf(self.env, 'transient', self.port,
+                                           extra=extra)
+
     def wait_active(self, timeout: float = 20.0) -> bool:
         end = time.time() + timeout
         while time.time() < end:
diff --git a/test/modules/arch/linux/test_007_notify_reload.py b/test/modules/arch/linux/test_007_notify_reload.py
new file mode 100644
index 00000000000..93833299c63
--- /dev/null
+++ b/test/modules/arch/linux/test_007_notify_reload.py
@@ -0,0 +1,139 @@
+import os
+import time
+
+import pytest
+
+from .env import (NOTIFY_RELOAD_VERSION, TransientService, error_log_since,
+                  error_log_size, http_responds, systemd_version)
+
+pytestmark = [
+    pytest.mark.skipif(
+        not TransientService.is_available(),
+        reason="no per-user systemd manager to run a transient service under"),
+    pytest.mark.skipif(
+        systemd_version() < NOTIFY_RELOAD_VERSION,
+        reason=f"Type=notify-reload needs systemd {NOTIFY_RELOAD_VERSION}"),
+]
+
+
+class TestNotifyReload:
+    """httpd as a Type=notify-reload unit.
+
+    Under Type=notify a reload is only whatever ExecReload= does, and
+    systemctl returns as soon as that command exits.  For "httpd -k
+    graceful" that is as soon as the signal has been sent, long before the
+    new configuration is in use, and it reports success however the restart
+    turns out.  Type=notify-reload holds the reload job open until the
+    service sends RELOADING=1 and then READY=1, which mod_systemd sends
+    from pre_config and post_config, so the result is reported once it is
+    known.
+
+    The unit here keeps ExecReload= and sets ReloadSignal=SIGCONT, which
+    httpd ignores.  The manager runs ExecReload= first and sends
+    ReloadSignal= only once it has exited successfully, so:
+
+      - "httpd -k graceful" stays the thing which restarts the server, and
+        because it parses the new configuration in its own process before
+        signalling, a configuration which does not parse fails the reload
+        without the running server being signalled at all.  A bare
+        ReloadSignal= has the running parent read the new configuration and
+        exit if it does not parse.
+
+      - the signal the manager then sends is redundant, so it is pointed at
+        one httpd does not act on.  The default is SIGHUP, which httpd
+        takes as an *ungraceful* restart.
+    """
+
+    @pytest.fixture(autouse=True, scope='class')
+    def _class_scope(self, env):
+        assert env.apache_stop() == 0
+        yield
+        assert env.apache_stop() == 0
+
+    @pytest.fixture
+    def service(self, env) -> TransientService:
+        svc = TransientService(env, port=env.http_port2,
+                               name=f'httpd-reload-{os.getpid()}',
+                               service_type='notify-reload',
+                               properties=['ReloadSignal=SIGCONT'])
+        yield svc
+        svc.stop()
+
+    def test_systemd_007_01_reload_waits_for_the_new_config(self, env, service):
+        """systemctl reload returns only once the reloaded server is
+        serving: the port added to the configuration answers without the
+        test waiting for it, because READY=1 is sent from post_config, by
+        which point the listeners are open."""
+        assert service.start().returncode == 0
+        assert service.wait_active()
+        assert http_responds(env.http_port2)
+
+        service.rewrite_conf(extra=f'Listen {env.proxy_port}')
+        r = service.reload()
+        assert r.returncode == 0, f"reload failed: {r.stderr}"
+        assert http_responds(env.proxy_port, timeout=10.0), \
+            "reload returned before the newly configured port was served"
+        assert http_responds(env.http_port2)
+
+    def test_systemd_007_02_broken_config_fails_the_reload(self, env, service):
+        """A configuration which does not parse fails the reload and leaves
+        the running server alone.  ExecReload= reads it in a process of its
+        own and exits without signalling, and the manager sends no reload
+        signal of its own once that has failed, so the parent never reads
+        it."""
+        assert service.start().returncode == 0
+        assert service.wait_active()
+        pid = int(service.show('MainPID'))
+
+        service.rewrite_conf(extra='ThisDirectiveDoesNotExist on')
+        r = service.reload()
+        assert r.returncode != 0, \
+            "reload of an unparseable configuration reported success"
+        assert service.show('ActiveState') == 'active', \
+            "a failed reload took the unit out of active"
+        # The parent exits when a restart re-reads a configuration which
+        # does not parse, and its children go on serving for a while
+        # afterwards, so answering a request is not on its own proof that
+        # the server survived: check the process the manager tracks.
+        assert int(service.show('MainPID')) == pid, \
+            "the parent was restarted despite the configuration not parsing"
+        assert http_responds(env.http_port2)
+
+    def test_systemd_007_03_reload_restarts_gracefully_once(self, env, service):
+        """One reload is one graceful restart, and no ungraceful one.
+        Leaving ReloadSignal= at its SIGHUP default is what this catches:
+        httpd restarts ungracefully on SIGHUP, dropping the connections a
+        reload is supposed to keep."""
+        if env.mpm_module not in ('mpm_event', 'mpm_worker'):
+            pytest.skip(f"{env.mpm_module} does not log the graceful restart")
+        assert service.start().returncode == 0
+        assert service.wait_active()
+
+        pos = error_log_size(env)
+        assert service.reload().returncode == 0
+        # The restart is logged before the configuration is re-read, so it
+        # is written by the time READY=1 ends the reload; give the
+        # redundant signal which follows a moment to land as well.
+        time.sleep(2)
+        log = error_log_since(env, pos)
+        assert log.count("Attempting to restart") == 0, \
+            f"the reload restarted the server ungracefully:\n{log}"
+        assert log.count("Doing graceful restart") == 1, \
+            f"expected one graceful restart, log said:\n{log}"
+
+    def test_systemd_007_04_reload_repeats(self, env, service):
+        """Reloading twice works.  The manager ignores a RELOADING=1 which
+        is not stamped later than the reload it asked for, so a server
+        which got MONOTONIC_USEC wrong could report the first reload and
+        then leave the second to time out."""
+        assert service.start().returncode == 0
+        assert service.wait_active()
+        pid = int(service.show('MainPID'))
+
+        for i in range(2):
+            r = service.reload()
+            assert r.returncode == 0, f"reload {i + 1} failed: {r.stderr}"
+            assert service.show('ActiveState') == 'active'
+            assert int(service.show('MainPID')) == pid, \
+                "a graceful restart replaced the parent process"
+        assert http_responds(env.http_port2)