From: Mike Yuan <me@yhndnzj.com>
Date: Mon, 14 Oct 2024 17:59:47 +0000 (+0200)
Subject: core/service: don't propagate stop jobs if RestartMode=direct
X-Git-Tag: v257-rc1~53^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7a13937007ccc7246b8351c7d31d17252b9940a0;p=thirdparty%2Fsystemd.git

core/service: don't propagate stop jobs if RestartMode=direct

The goal of RestartMode=direct is to make restarts invisible
to dependents, so auto restart jobs shouldn't bring them down
at all. So far we only skipped going through failed/dead states
in service_enter_dead(), i.e. the unit would never be considered
dead. But when constructing restart transaction, the stop job
would be propagated to dependents. Consider the following 2 units:

dependent.target:
[Unit]
BindsTo=a.service
After=a.service

a.service:
[Service]
ExecStart=bash -c 'sleep 100 && exit 1'
Restart=on-failure
RestartMode=direct

Before this commit, even though BindsTo= isn't triggered since
a.service never failed, when a.service auto-restarts, dependent.target
is also restarted. Let's suppress it by using JOB_REPLACE instead of
JOB_RESTART_DEPENDENCIES in service_enter_restart().

Fixes #34758

The example above is subtly different from the original report,
to illustrate that the new behavior makes sense for less exotic
use cases too.
---

diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index 3066c919557..a5c0dc84e12 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -942,7 +942,7 @@
               <listitem>
                 <para>If set to <option>direct</option>, the service transitions to the activating
                 state directly during auto-restart, skipping failed/inactive state.
-                <varname>ExecStopPost=</varname> is invoked.
+                <varname>ExecStopPost=</varname> is still invoked.
                 <varname>OnSuccess=</varname> and <varname>OnFailure=</varname> are skipped.</para>
 
                 <para>This option is useful in cases where a dependency can fail temporarily but we don't
diff --git a/src/core/service.c b/src/core/service.c
index 1185e2fbcff..8cea17f853f 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -2646,16 +2646,17 @@ static void service_enter_restart(Service *s, bool shortcut) {
                 return;
         }
 
-        /* Any units that are bound to this service must also be restarted. We use JOB_START for ourselves
-         * but then set JOB_RESTART_DEPENDENCIES which will enqueue JOB_RESTART for those dependency jobs.
+        /* Any units that are bound to this service must also be restarted, unless RestartMode=direct.
+         * We use JOB_START for ourselves but then set JOB_RESTART_DEPENDENCIES which will enqueue JOB_RESTART
+         * for those dependency jobs in the former case, plain JOB_REPLACE when RestartMode=direct.
          *
-         * When RestartMode=direct is used, the service being restarted don't enter the inactive/failed state,
+         * Also, when RestartMode=direct is used, the service being restarted don't enter the inactive/failed state,
          * i.e. unit_process_job -> job_finish_and_invalidate is never called, and the previous job might still
          * be running (especially for Type=oneshot services).
          * We need to refuse late merge and re-enqueue the anchor job. */
         r = manager_add_job_full(UNIT(s)->manager,
                                  JOB_START, UNIT(s),
-                                 JOB_RESTART_DEPENDENCIES,
+                                 s->restart_mode == SERVICE_RESTART_MODE_DIRECT ? JOB_REPLACE : JOB_RESTART_DEPENDENCIES,
                                  TRANSACTION_REENQUEUE_ANCHOR,
                                  /* affected_jobs = */ NULL,
                                  &error, /* ret = */ NULL);