mod_systemd: Implement watchdog support

changes-entries/systemd-watchdog.txt

@@ -0,0 +1,4 @@
+  *) mod_systemd: Support the systemd watchdog, sending the keep-alive
+     notification if the service unit sets WatchdogSec=.  The notification
+     is sent every ten seconds, so a shorter WatchdogSec cannot be met and
+     is warned about at startup.  [Joe Orton]

docs/manual/mod/mod_systemd.xml

@@ -81,6 +81,31 @@ WantedBy=multi-user.target
     module="core">ExtendedStatus</directive> is not disabled in
     the configuration, run-time load and request statistics are made
     available in the <code>systemctl status</code> output.</p>
+
+    <p>The systemd watchdog is supported.  If the service unit sets
+    <code>WatchdogSec=</code>, the parent process sends the keep-alive
+    notification which tells systemd the server is still alive; a server
+    which stops sending it is terminated and, with a suitable
+    <code>Restart=</code> setting, restarted.  The notification is sent
+    while the configuration is being read and again once it is loaded, so
+    that a reload is covered, and periodically from the parent process
+    while the server runs.</p>
+
+    <p>That periodic notification is sent about every ten seconds, which
+    is how often the parent process runs the hook it is sent from.  A
+    <code>WatchdogSec=</code> of less than twice that cannot be met, and
+    would have systemd terminating a server which is working normally;
+    such a setting is reported as a warning at startup.  Use a
+    <code>WatchdogSec=</code> of at least 20 seconds.</p>
+
+    <example>
+    <title>Adding watchdog supervision to the unit above</title>
+    <pre>
+[Service]
+WatchdogSec=30
+Restart=on-failure
+    </pre>
+    </example>
 </summary>
 
 </modulesynopsis>

modules/arch/unix/mod_systemd.c

@@ -52,23 +52,52 @@ static apr_uint64_t monotonic_usec(void)
     return (apr_uint64_t)ts.tv_sec * APR_USEC_PER_SEC + ts.tv_nsec / 1000;
 }
 
+/* ap_run_monitor() is called once every INTERVAL_OF_WRITABLE_PROBES turns
+ * of the parent's one second loop in ap_wait_or_timeout(), so that is how
+ * often a keep-alive notification can be sent, and the shortest watchdog
+ * timeout which can be met is twice that: sd_watchdog_enabled(3) asks for
+ * a notification every half of the configured timeout. */
+#define WATCHDOG_INTERVAL_SEC (10)
+
+/* The WatchdogSec= of the service in microseconds, or zero if the service
+ * manager is not watching.  Set in pre_config, before the first
+ * notification which could carry a keep-alive. */
+static apr_uint64_t watchdog_usec;
+
+/* A keep-alive assignment to paste into a notification, or nothing while
+ * the service manager is not asking for one.  Sending WATCHDOG=1 when it
+ * is not expected is harmless, but saying so only when asked keeps what
+ * httpd reports the same as what the service was configured for. */
+static const char *watchdog_ping(void)
+{
+    return watchdog_usec ? "WATCHDOG=1\n" : "";
+}
+
 static int systemd_pre_config(apr_pool_t *pconf, apr_pool_t *plog,
                               apr_pool_t *ptemp)
 {
-    apr_uint64_t usec = monotonic_usec();
+    apr_uint64_t usec = monotonic_usec(), wd_usec;
+
+    /* Read afresh on each configuration load, since a restart unloads and
+     * loads the module again, and without unsetting it as server/listen.c
+     * does for $LISTEN_FDS, which would stop the keep-alive at the first
+     * reload. */
+    watchdog_usec = sd_watchdog_enabled(0, &wd_usec) > 0 ? wd_usec : 0;
 
     /* A Type=notify-reload service ignores a reload notification which
      * does not say when it was sent. */
     if (usec) {
         sd_notifyf(0,
                    "RELOADING=1\n"
                    "MONOTONIC_USEC=%" APR_UINT64_T_FMT "\n"
-                   "STATUS=Reading configuration...\n", usec);
+                   "%s"
+                   "STATUS=Reading configuration...\n", usec, watchdog_ping());
     }
     else {
-        sd_notify(0,
-                  "RELOADING=1\n"
-                  "STATUS=Reading configuration...\n");
+        sd_notifyf(0,
+                   "RELOADING=1\n"
+                   "%s"
+                   "STATUS=Reading configuration...\n", watchdog_ping());
     }
     ap_extended_status = 1;
     return OK;
@@ -121,8 +150,28 @@ static int systemd_post_config(apr_pool_t *pconf, apr_pool_t *plog,
     apr_pool_cleanup_register(pconf, NULL, systemd_stopping,
                               apr_pool_cleanup_null);
 
-    sd_notify(0, "READY=1\n"
-              "STATUS=Configuration loaded.\n");
+    /* A timeout the parent cannot meet would have the service manager
+     * killing a healthy server every WatchdogSec, so say so rather than
+     * leaving nothing in the log to explain it. */
+    if (watchdog_usec
+        && watchdog_usec / 2 < (apr_uint64_t)WATCHDOG_INTERVAL_SEC
+                               * APR_USEC_PER_SEC) {
+        ap_log_error(APLOG_MARK, APLOG_WARNING, 0, main_server, APLOGNO(10621)
+                     "WatchdogSec is %" APR_UINT64_T_FMT "us, but keep-alive "
+                     "notifications are sent from the parent process only "
+                     "every %ds; configure a WatchdogSec of at least %ds or "
+                     "the service will be killed while it is healthy",
+                     watchdog_usec, WATCHDOG_INTERVAL_SEC,
+                     2 * WATCHDOG_INTERVAL_SEC);
+    }
+
+    /* The keep-alive rides along with the notification which ends a
+     * reload: the configuration is read outside the parent's monitor loop,
+     * so nothing reports while it is being parsed, and the service manager
+     * keeps the timeout armed throughout. */
+    sd_notifyf(0, "READY=1\n"
+               "%s"
+               "STATUS=Configuration loaded.\n", watchdog_ping());
     return OK;
 }
 
@@ -141,6 +190,12 @@ static int systemd_monitor(apr_pool_t *p, server_rec *s)
     apr_interval_time_t up_time;
     char bps[5];
 
+    /* Before anything which might decline: reporting the server is alive
+     * does not depend on there being a status line to report with it. */
+    if (watchdog_usec) {
+        sd_notify(0, "WATCHDOG=1\n");
+    }
+
     if (!ap_extended_status) {
         /* Nothing useful to report with ExtendedStatus disabled. */
         return DECLINED;

test/modules/arch/linux/README

@@ -17,15 +17,19 @@ to the service manager rather than to a client:
     activation and not loading it is what disables it.
   - ap_extended_status forced on in pre_config, so that the monitor hook
     has request counts to report.
+  - the watchdog keep-alive, WATCHDOG=1, sent while the service manager
+    asks for one.  Which it does through the environment as well:
+    WatchdogSec= in the unit becomes $WATCHDOG_USEC, and $WATCHDOG_PID
+    names the process expected to report.
 
 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 involves systemd at all; the rest run anywhere, and need nothing
-from the systemd package beyond the libsystemd httpd itself is linked
-against.
+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
+libsystemd httpd itself is linked against.
 
   test_001_notify.py   $NOTIFY_SOCKET is an ordinary AF_UNIX datagram
                        socket the test binds itself (env.NotifyListener).
@@ -51,6 +55,22 @@ against.
                        distributions, and doing it directly needs no
                        systemd tooling at all.
 
+  test_006_watchdog.py The keep-alive.  Mostly the same stand-in socket,
+                       with $WATCHDOG_USEC set in the environment httpd is
+                       started with; env.ForegroundServer runs httpd in the
+                       foreground where $WATCHDOG_PID can name it, which
+                       apachectl cannot do for a parent it has not forked
+                       yet.  The last test uses a real unit with
+                       WatchdogSec=, where systemd's own
+                       WatchdogTimestampMonotonic is the evidence that the
+                       pings arrived.
+
+                       The keep-alive is sent from the monitor hook, so it
+                       is only as frequent as that hook is: about every ten
+                       seconds.  A WatchdogSec shorter than twice that
+                       cannot be met, and mod_systemd says so (AH10621)
+                       rather than letting a healthy server be killed.
+
   test_005_service.py  The real thing: a transient Type=notify unit run
                        with "systemd-run --user".  This is what checks that
                        systemd holds the unit in "activating" until READY=1

test/modules/arch/linux/env.py

@@ -211,6 +211,28 @@ def statuses(self) -> List[str]:
 NO_MONITOR_TIMEOUT = 15.0
 
 
+# The keep-alive notification is sent from the same monitor hook, so waiting
+# for one costs the same as waiting for a status report.
+WATCHDOG_TIMEOUT = MONITOR_TIMEOUT
+
+# The shortest WatchdogSec mod_systemd will accept without complaining, which
+# is twice the monitor interval: the recommended keep-alive period is half the
+# watchdog timeout, and half of anything shorter than this is out of reach of a
+# hook which runs every ten seconds.  Keep in step with mod_systemd.c.
+SYSTEMD_MONITOR_INTERVAL = 10
+MIN_WATCHDOG_SEC = 2 * SYSTEMD_MONITOR_INTERVAL
+
+
+def is_watchdog_ping(msg: Dict[str, str]) -> bool:
+    """Whether a notification carries the keep-alive ping.
+
+    mod_systemd is free to send WATCHDOG=1 in a datagram of its own or
+    alongside whatever else it is reporting, so match on the assignment
+    rather than on the message being only that.
+    """
+    return msg.get('WATCHDOG') == '1'
+
+
 # A configuration for a server run directly rather than through apachectl,
 # sharing the server root, module list and error log with the rest of the
 # suite but with its own pid file and port.
@@ -435,12 +457,15 @@ class TransientService:
     """
 
     def __init__(self, env: SystemdTestEnv, port: int,
-                 name: str = None, extra: str = ''):
+                 name: str = None, extra: str = '',
+                 properties: List[str] = None):
         self.env = env
         self.port = port
         self.unit = name or f'httpd-test-{os.getpid()}'
         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=.
+        self.properties = list(properties or [])
 
     def read_pid(self) -> Optional[int]:
         try:
@@ -482,6 +507,7 @@ def start(self, timeout: float = 20.0) -> subprocess.CompletedProcess:
             '--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],
             httpd, '-DFOREGROUND',
             '-d', self.env.server_dir, '-f', self.conf_file,
         ], capture_output=True, text=True, timeout=timeout)
@@ -504,3 +530,43 @@ def __enter__(self) -> 'TransientService':
 
     def __exit__(self, *args):
         self.stop()
+
+
+class ForegroundServer(ActivatedServer):
+    """httpd run directly in the foreground, with extra environment.
+
+    The watchdog protocol is keyed to a process id: sd_watchdog_enabled(3)
+    ignores $WATCHDOG_USEC unless $WATCHDOG_PID is unset or names the
+    process reading it.  apachectl cannot be used to set that, since the
+    variable has to name the parent httpd and the pid is not known until it
+    exists, so the value is assigned in a shell which then exec's httpd in
+    its own place -- the same trick ActivatedServer uses for $LISTEN_PID.
+
+    Assignments are shell words, so "$$" in a value is the pid httpd will
+    have.
+    """
+
+    def __init__(self, env: SystemdTestEnv, port: int,
+                 name: str = 'foreground', extra: str = '',
+                 setenv: Dict[str, str] = None,
+                 modules_conf: str = 'modules.conf'):
+        super().__init__(env, port, name=name, extra=extra,
+                         modules_conf=modules_conf)
+        self.setenv = dict(setenv or {})
+
+    def args(self, fd: int = None) -> List[str]:
+        assigns = ' '.join(f'{k}={v}' for k, v in self.setenv.items())
+        return [
+            'bash', '-c',
+            f'export {assigns}; exec "$0" "$@"' if assigns else 'exec "$0" "$@"',
+            self.env.httpd_bin, '-DFOREGROUND',
+            '-d', self.env.server_dir, '-f', self.conf_file,
+        ]
+
+    def start(self) -> 'ForegroundServer':
+        # A new session, so the whole group can be signalled on the way out
+        # even when a failed restart leaves children behind.
+        self.proc = subprocess.Popen(
+            self.args(), env=self.env.server_env(), start_new_session=True,
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        return self

test/modules/arch/linux/test_001_notify.py

@@ -127,16 +127,26 @@ def test_systemd_001_08_reloading_monotonic(self, env):
         assert 0 < int(msg['MONOTONIC_USEC']) <= time.clock_gettime_ns(
             time.CLOCK_MONOTONIC) // 1000
 
-    @pytest.mark.xfail(reason="mod_systemd implements no watchdog keepalive, "
-                              "so a unit using WatchdogSec= would be killed")
     def test_systemd_001_09_watchdog(self, env):
+        """A server started with $WATCHDOG_USEC reports that it is alive.
+
+        $WATCHDOG_PID is left unset, which sd_watchdog_enabled(3) takes to
+        mean any process may report: it has to be, since apachectl starts a
+        parent whose pid nothing knew in advance.  The keep-alive arrives
+        with the startup notifications rather than only from the monitor
+        hook ten seconds later, which is what makes this cheap to check.
+        test_006_watchdog.py covers the rest of the protocol.
+        """
         assert env.apache_stop() == 0
-        env.set_httpd_env('WATCHDOG_USEC', '2000000')  # ping every 1s
+        # Long enough that mod_systemd does not object to it (AH10622).
+        env.set_httpd_env('WATCHDOG_USEC', str(60 * 1000000))
         try:
             env.notify.clear()
             assert env.apache_restart() == 0
-            assert env.notify.wait_for_key('WATCHDOG', timeout=4), \
-                "no watchdog keepalive was sent"
+            msg = env.notify.wait_for_key('WATCHDOG', timeout=4)
+            assert msg, f"no watchdog keepalive was sent, " \
+                        f"got {env.notify.messages}"
+            assert msg['WATCHDOG'] == '1'
         finally:
             env.set_httpd_env('WATCHDOG_USEC', None)
             assert env.apache_restart() == 0