core: add FileDescriptorStorePreserve=on-success option (#42160)

Currently with FileDescriptorStorePreserve=yes the FD store is kept
around
regardless of what happens to a unit, which is useful in many cases. But
in
some cases, for example when complex services crash horribly, it's hard
to
reason about what was in the intermediate state, and it's better to
start
fresh.

Add a new 'on-success' option for the FileDescriptorStorePreserve=
setting
that keeps it around only for as long as the unit doesn't go to a
persistently
failed state.

This is especially useful in combination with LUO, where we don't want
to
keep around LUO sessions created by units that then proceeded to crash
and
burn, and might be in a bad state afterwards.

src/core/execute.c

@@ -3182,9 +3182,10 @@ static const char* const exec_utmp_mode_table[_EXEC_UTMP_MODE_MAX] = {
 DEFINE_STRING_TABLE_LOOKUP(exec_utmp_mode, ExecUtmpMode);
 
 static const char* const exec_preserve_mode_table[_EXEC_PRESERVE_MODE_MAX] = {
-        [EXEC_PRESERVE_NO]      = "no",
-        [EXEC_PRESERVE_YES]     = "yes",
-        [EXEC_PRESERVE_RESTART] = "restart",
+        [EXEC_PRESERVE_NO]         = "no",
+        [EXEC_PRESERVE_YES]        = "yes",
+        [EXEC_PRESERVE_RESTART]    = "restart",
+        [EXEC_PRESERVE_ON_SUCCESS] = "on-success",
 };
 
 DEFINE_STRING_TABLE_LOOKUP_WITH_BOOLEAN(exec_preserve_mode, ExecPreserveMode, EXEC_PRESERVE_YES);

src/core/execute.h

@@ -61,6 +61,7 @@ typedef enum ExecPreserveMode {
         EXEC_PRESERVE_NO,
         EXEC_PRESERVE_YES,
         EXEC_PRESERVE_RESTART,
+        EXEC_PRESERVE_ON_SUCCESS,
         _EXEC_PRESERVE_MODE_MAX,
         _EXEC_PRESERVE_MODE_INVALID = -EINVAL,
 } ExecPreserveMode;

src/core/luo.c

@@ -204,7 +204,7 @@ int manager_luo_serialize_fd_stores(Manager *m, FILE **ret_f, FDSet **ret_fds) {
 
                 s = SERVICE(u);
 
-                if (s->fd_store_preserve_mode != EXEC_PRESERVE_YES)
+                if (!IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_YES, EXEC_PRESERVE_ON_SUCCESS))
                         continue;
 
                 if (!s->fd_store)

src/core/service.c

@@ -699,7 +699,7 @@ int service_add_fd_store(Service *s, int fd_in, const char *name, bool do_poll,
          * unit and the original fdname. This way fdstore persistence chains all the way up to whichever
          * entity is ultimately responsible for surviving across kexec/restart, regardless of fdname
          * length or charset constraints. */
-        if (propagate_upstream && s->fd_store_preserve_mode == EXEC_PRESERVE_YES) {
+        if (propagate_upstream && IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_YES, EXEC_PRESERVE_ON_SUCCESS)) {
                 Manager *m = ASSERT_PTR(UNIT(s)->manager);
                 char idx_str[STRLEN(SERVICE_FDSTORE_SUB_FDNAME_PREFIX) + DECIMAL_STR_MAX(uint64_t)];
 
@@ -721,7 +721,7 @@ int service_add_fd_store(Service *s, int fd_in, const char *name, bool do_poll,
         LIST_PREPEND(fd_store, s->fd_store, TAKE_PTR(fs));
         s->n_fd_store++;
 
-        if (propagate_upstream && s->fd_store_preserve_mode == EXEC_PRESERVE_YES)
+        if (propagate_upstream && IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_YES, EXEC_PRESERVE_ON_SUCCESS))
                 /* Refresh the JSON mapping memfd so the supervisor can resolve the new index. Do this
                  * after LIST_PREPEND so the new entry is visible to the helper. */
                 (void) service_propagate_fd_store_mapping_upstream(UNIT(s)->manager);
@@ -876,7 +876,7 @@ static int service_attach_external_fd_to_fdstore(Unit *u, int fd, const char *fd
         if (r > 0 &&
             s->state == SERVICE_DEAD &&
             s->deserialized_state == SERVICE_DEAD &&
-            s->fd_store_preserve_mode == EXEC_PRESERVE_YES) {
+            IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_YES, EXEC_PRESERVE_ON_SUCCESS)) {
                 service_set_state(s, SERVICE_DEAD_RESOURCES_PINNED);
                 s->deserialized_state = SERVICE_DEAD_RESOURCES_PINNED;
         }
@@ -2350,7 +2350,7 @@ static bool service_will_restart(Unit *u) {
 static ServiceState service_determine_dead_state(Service *s) {
         assert(s);
 
-        return SERVICE_FD_STORE_POPULATED(s) && s->fd_store_preserve_mode == EXEC_PRESERVE_YES ? SERVICE_DEAD_RESOURCES_PINNED : SERVICE_DEAD;
+        return SERVICE_FD_STORE_POPULATED(s) && IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_YES, EXEC_PRESERVE_ON_SUCCESS) ? SERVICE_DEAD_RESOURCES_PINNED : SERVICE_DEAD;
 }
 
 static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) {
@@ -2454,7 +2454,8 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
         unit_destroy_runtime_data(UNIT(s), &s->exec_context, /* destroy_runtime_dir= */ true);
 
         /* Also get rid of the fd store, if that's configured. */
-        if (s->fd_store_preserve_mode == EXEC_PRESERVE_NO)
+        if (s->fd_store_preserve_mode == EXEC_PRESERVE_NO ||
+            (s->fd_store_preserve_mode == EXEC_PRESERVE_ON_SUCCESS && s->state == SERVICE_FAILED))
                 service_release_fd_store(s);
 
         /* Get rid of the IPC bits of the user */
@@ -6119,7 +6120,8 @@ static void service_release_resources(Unit *u) {
         service_release_extra_fds(s);
         s->root_directory_fd = asynchronous_close(s->root_directory_fd);
 
-        if (s->fd_store_preserve_mode != EXEC_PRESERVE_YES)
+        if (IN_SET(s->fd_store_preserve_mode, EXEC_PRESERVE_NO, EXEC_PRESERVE_RESTART) ||
+            (s->fd_store_preserve_mode == EXEC_PRESERVE_ON_SUCCESS && s->state == SERVICE_FAILED))
                 service_release_fd_store(s);
 
         if (s->state == SERVICE_DEAD_RESOURCES_PINNED && !SERVICE_FD_STORE_POPULATED(s))

src/shared/varlink-io.systemd.Unit.c

@@ -25,7 +25,8 @@ SD_VARLINK_DEFINE_ENUM_TYPE(
                 ExecPreserveMode,
                 SD_VARLINK_DEFINE_ENUM_VALUE(no),
                 SD_VARLINK_DEFINE_ENUM_VALUE(yes),
-                SD_VARLINK_DEFINE_ENUM_VALUE(restart));
+                SD_VARLINK_DEFINE_ENUM_VALUE(restart),
+                SD_VARLINK_DEFINE_ENUM_VALUE(on_success));
 
 SD_VARLINK_DEFINE_ENUM_TYPE(
                 ExecKeyringMode,

src/test/test-fdstore.c

@@ -6,6 +6,7 @@
  *
  * This binary is intentionally linked against libsystemd only so that it can go in the minimal image. */
 
+#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -28,6 +29,10 @@ static void closep(int *fd) {
         *fd = -EBADF;
 }
 
+static _Noreturn void exit_handler(int sig) {
+        _exit(EXIT_SUCCESS);
+}
+
 static int push_one(const char *fdname, const char *content) {
         _cleanup_(closep) int fd = -EBADF;
         int r;
@@ -159,8 +164,16 @@ int main(int argc, char *argv[]) {
         if (r != EXIT_SUCCESS)
                 return r;
 
-        /* On success, become sleep so if we are a container payload it can stay alive. */
-        execlp("sleep", "sleep", "infinity", (char *) NULL);
-        fprintf(stderr, "execlp(sleep) failed: %m\n");
-        return EXIT_FAILURE;
+        /* On success, stay alive so if we are a container payload we keep it running. Install handlers
+         * for the signals an outer supervisor may use to terminate us, so we exit cleanly (with status 0)
+         * and the container service ends up in 'inactive' rather than 'failed'. */
+        struct sigaction sa = { .sa_handler = exit_handler };
+        if (sigaction(SIGTERM, &sa, /* __oact= */ NULL) < 0 ||
+            sigaction(SIGINT, &sa, /* __oact= */ NULL) < 0) {
+                fprintf(stderr, "Failed to install signal handlers: %m\n");
+                return EXIT_FAILURE;
+        }
+
+        for (;;)
+                pause();
 }