mod_systemd: Document Type=notify-reload, and test it

.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

docs/manual/mod/mod_systemd.xml

@@ -32,7 +32,7 @@
 <summary>
     <p>This module provides support for systemd integration. It allows
     httpd to be used in a service with the systemd
-    <code>Type=notify</code> (see <a
+    <code>Type=notify</code> or <code>Type=notify-reload</code> (see <a
     href="https://www.freedesktop.org/software/systemd/man/systemd.service.html">systemd.service(5)</a>
     for more information).  The module is activated if loaded.</p>
 
@@ -69,6 +69,53 @@ WantedBy=multi-user.target
    href="https://www.freedesktop.org/software/systemd/man/systemd.kill.html">systemd.kill(5)</a>
    for more information.</p>
 
+   <p>A service manager from systemd 253 onwards offers
+   <code>Type=notify-reload</code>, which is worth using in preference.
+   Under <code>Type=notify</code> a <code>systemctl reload</code>
+   returns as soon as the <code>ExecReload</code> command has sent its
+   signal, which is before the new configuration has been read, and it
+   reports success whatever becomes of the restart afterwards.
+   <code>Type=notify-reload</code> 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 <code>RELOADING=1</code> notification the protocol expects
+   while the configuration is being read, stamped with the
+   <code>MONOTONIC_USEC</code> the service manager requires, and
+   <code>READY=1</code> once it has been loaded.</p>
+
+    <example>
+    <title>Example of a service unit which reloads synchronously</title>
+    <pre>
+[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
+    </pre>
+    </example>
+
+   <p>The service manager runs <code>ExecReload</code> first, and sends
+   the signal named by <code>ReloadSignal</code> only once that command
+   has exited successfully.  Keeping <code>ExecReload</code> is what
+   makes the reload safe: <code>httpd -k graceful</code> 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
+   <code>ExecReload</code> 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.</p>
+
+   <p>The signal sent after <code>ExecReload</code> has run is then
+   redundant, so <code>ReloadSignal</code> should name one httpd does
+   not act on, such as <code>SIGCONT</code>.  It matters that it is set:
+   the default is <code>SIGHUP</code>, which httpd takes as an
+   <em>ungraceful</em> restart, dropping the connections a reload is
+   meant to preserve.  A unit which does leave out
+   <code>ExecReload</code> must set <code>ReloadSignal=SIGUSR1</code>,
+   the signal httpd restarts gracefully on.</p>
+
    <p>Systemd socket activation is supported if httpd was built with
    it.  Each <directive module="mpm_common">Listen</directive> port
    must then be one passed in by systemd; a port which was not is a
@@ -99,7 +146,7 @@ WantedBy=multi-user.target
     <code>WatchdogSec=</code> of at least 20 seconds.</p>
 
     <example>
-    <title>Adding watchdog supervision to the unit above</title>
+    <title>Adding watchdog supervision to either unit above</title>
     <pre>
 [Service]
 WatchdogSec=30

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,

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,29 +534,51 @@ 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')
         return r.stdout.strip()
 
     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: