Merge pull request #5840 from garlick/issue#5778

mergify[bot] · web-flow · commit 69711aa893d1 · 2024-03-28T23:57:53.000Z
make KVS garbage collection automatic
diff --git a/doc/man1/flux-shutdown.rst b/doc/man1/flux-shutdown.rst
@@ -82,6 +82,13 @@ OPTIONS
    the dump, and the link is removed.  :linux:man8:`systemd-tmpfiles`
    automatically cleans up dump files in ``/var/lib/flux/dump`` after 30 days.
 
+.. option:: --skip-gc
+
+   When garbage collection has been enabled automatically, as indicated
+   by the ``content.dump`` broker attribute, this option disables it
+   during shutdown.  Otherwise it is a preemptive "no" answer to the garbage
+   collection prompt.
+
 .. option:: -y, --yes
 
    Answer yes to any yes/no questions.
diff --git a/etc/flux.service.in b/etc/flux.service.in
@@ -4,7 +4,7 @@ Wants=munge.service
 
 [Service]
 Type=notify
-NotifyAccess=main
+NotifyAccess=all
 TimeoutStopSec=90
 KillMode=mixed
 ExecStart=/bin/bash -c '\
@@ -23,6 +23,7 @@ ExecStart=/bin/bash -c '\
   -Sbroker.quorum-timeout=none \
   -Sbroker.exit-norestart=42 \
   -Sbroker.sd-notify=1 \
+  -Scontent.dump=auto \
   -Scontent.restore=auto \
 '
 SyslogIdentifier=flux
diff --git a/src/cmd/Makefile.am b/src/cmd/Makefile.am
@@ -14,6 +14,7 @@ AM_CPPFLAGS = \
 	$(FLUX_SECURITY_CFLAGS) \
 	$(HWLOC_CFLAGS) \
 	$(JANSSON_CFLAGS) \
+	$(LIBSYSTEMD_CFLAGS) \
 	$(LIBARCHIVE_CFLAGS)
 
 
@@ -87,6 +88,7 @@ flux_LDADD = \
 	$(top_builddir)/src/common/libpmi/libpmi_common.la \
 	$(top_builddir)/src/common/libfilemap/libfilemap.la \
 	$(LIBARCHIVE_LIBS) \
+	$(LIBSYSTEMD_LIBS) \
 	$(fluxcmd_ldadd)
 
 #
diff --git a/src/cmd/builtin/dump.c b/src/cmd/builtin/dump.c
@@ -11,6 +11,9 @@
 #if HAVE_CONFIG_H
 # include <config.h>
 #endif
+#if HAVE_LIBSYSTEMD
+#include <systemd/sd-daemon.h>
+#endif
 #include <unistd.h>
 #include <stdarg.h>
 #include <jansson.h>
@@ -33,6 +36,7 @@ static void dump_treeobj (struct archive *ar,
                           const char *path,
                           json_t *treeobj);
 
+static bool sd_notify_flag;
 static bool verbose;
 static bool quiet;
 static int content_flags;
@@ -49,12 +53,24 @@ static void progress (int delta_keys)
         && !quiet
         && (keycount % 100 == 0 || keycount < 10))
         fprintf (stderr, "\rflux-dump: archived %d keys", keycount);
+#if HAVE_LIBSYSTEMD
+    if (sd_notify_flag
+        && (keycount % 100 == 0 || keycount < 10)) {
+        sd_notifyf (0, "EXTEND_TIMEOUT_USEC=%d", 10000000); // 10s
+        sd_notifyf (0, "STATUS=flux-dump(1) has archived %d keys", keycount);
+    }
+#endif
 }
 
 static void progress_end (void)
 {
     if (!quiet && !verbose)
         fprintf (stderr, "\rflux-dump: archived %d keys\n", keycount);
+#if HAVE_LIBSYSTEMD
+    if (sd_notify_flag) {
+        sd_notifyf (0, "STATUS=flux-dump(1) has archived %d keys", keycount);
+    }
+#endif
 }
 
 static struct archive *dump_create (const char *outfile)
@@ -369,6 +385,15 @@ static int cmd_dump (optparse_t *p, int ac, char *av[])
     dump_gid = getgid ();
 
     h = builtin_get_flux_handle (p);
+
+    /* If the broker is using sd_notify(3) to talk to systemd during
+     * start/stop, we can use it to ensure systemd doesn't kill us
+     * while dumping during shutdown.  See flux-framework/flux-core#5778.
+     */
+    const char *s;
+    if ((s = flux_attr_get (h, "broker.sd-notify")) && !streq (s, "0"))
+        sd_notify_flag = true;
+
     ar = dump_create (outfile);
     if (optparse_hasopt (p, "checkpoint")) {
         flux_future_t *f;
diff --git a/src/cmd/builtin/restore.c b/src/cmd/builtin/restore.c
@@ -11,6 +11,9 @@
 #if HAVE_CONFIG_H
 # include <config.h>
 #endif
+#if HAVE_LIBSYSTEMD
+#include <systemd/sd-daemon.h>
+#endif
 #include <unistd.h>
 #include <stdarg.h>
 #include <jansson.h>
@@ -35,6 +38,7 @@
 
 #define BLOCKSIZE 10240 // taken from libarchive example
 
+static bool sd_notify_flag;
 static bool verbose;
 static bool quiet;
 static int content_flags;
@@ -56,6 +60,13 @@ static void progress (int delta_blob, int delta_keys)
                  keycount,
                  blobcount);
     }
+#if HAVE_LIBSYSTEMD
+    if (sd_notify_flag
+        && (keycount % 100 == 0 || keycount < 10)) {
+        sd_notifyf (0, "EXTEND_TIMEOUT_USEC=%d", 10000000); // 10s
+        sd_notifyf (0, "STATUS=flux-restore(1) has restored %d keys", keycount);
+    }
+#endif
 }
 static void progress_end (void)
 {
@@ -65,6 +76,11 @@ static void progress_end (void)
                  keycount,
                  blobcount);
     }
+#if HAVE_LIBSYSTEMD
+    if (sd_notify_flag) {
+        sd_notifyf (0, "STATUS=flux-restore(1) has restored %d keys", keycount);
+    }
+#endif
 }
 
 static struct archive *restore_create (const char *infile)
@@ -377,6 +393,11 @@ static int cmd_restore (optparse_t *p, int ac, char *av[])
     blob_size_limit = optparse_get_size_int (p, "size-limit", "0");
 
     h = builtin_get_flux_handle (p);
+
+    const char *s;
+    if ((s = flux_attr_get (h, "broker.sd-notify")) && !streq (s, "0"))
+        sd_notify_flag = true;
+
     ar = restore_create (infile);
 
     if (optparse_hasopt (p, "checkpoint")) {
diff --git a/src/cmd/builtin/shutdown.c b/src/cmd/builtin/shutdown.c
@@ -74,7 +74,9 @@ static bool gc_threshold_check (flux_t *h, optparse_t *p)
     get_gc_threshold (h, &gc_threshold);
 
     if (gc_threshold > 0 && version > gc_threshold) {
-        if (optparse_hasopt (p, "yes") || optparse_hasopt (p, "no")) {
+        if (optparse_hasopt (p, "yes")
+            || optparse_hasopt (p, "no")
+            || optparse_hasopt (p, "skip-gc")) {
             if (optparse_hasopt (p, "yes"))
                 rc = true;
             else
@@ -143,6 +145,11 @@ static int subcmd (optparse_t *p, int ac, char *av[])
     if (optparse_hasopt (p, "background"))
         flags &= ~FLUX_RPC_STREAMING;
 
+    if (optparse_hasopt (p, "skip-gc")) {
+        if (flux_attr_set (h, "content.dump", "") < 0)
+            log_err_exit ("error clearing content.dump attribute");
+    }
+
     if (optparse_hasopt (p, "gc")
         || optparse_hasopt (p, "dump")
         || gc_threshold_check (h, p)) {
@@ -176,6 +183,9 @@ static int subcmd (optparse_t *p, int ac, char *av[])
 }
 
 static struct optparse_option opts[] = {
+    { .name = "skip-gc", .has_arg = 0,
+      .usage = "Skip KVS garbage collection this time, if already enabled",
+    },
     { .name = "gc", .has_arg = 0,
       .usage = "Garbage collect KVS (short for --dump=auto)",
     },
diff --git a/t/system/0004-recovery.t b/t/system/0004-recovery.t
@@ -20,3 +20,12 @@ test_expect_success 'flux start --recover works from dump file' '
 test_expect_success 'restart flux' '
         sudo systemctl start flux
 '
+get_uptime_state () {
+	local state=$(flux uptime | cut -d' ' -f3) || state=unknown
+	echo $state
+}
+test_expect_success 'wait for flux to reach run state' '
+	while test $(get_uptime_state) != run; do \
+	    sleep 1; \
+	done
+'
diff --git a/t/t2808-shutdown-cmd.t b/t/t2808-shutdown-cmd.t
@@ -315,5 +315,20 @@ test_expect_success 'clean up dump files from previous tests' '
 	rm -f dump.tgz &&
 	rm -f dump/RESTORE
 '
+test_expect_success 'submit batch with dump=auto and wait for it to start (8)' '
+	cat >batch.sh <<-EOT &&
+	#!/bin/sh
+	touch job8-has-started
+	flux run sleep 300
+	EOT
+	chmod +x batch.sh &&
+	flux batch -t30m -n1 \
+	    --broker-opts=-Scontent.dump=auto batch.sh >jobid8 &&
+	$waitfile job8-has-started
+'
+test_expect_success 'shutdown --skip-gc does not produce dump' '
+	FLUX_URI=$(flux uri --local $(cat jobid8)) flux shutdown --skip-gc &&
+	test_must_fail tar tvf dump/RESTORE
+'
 
 test_done
diff --git a/t/t9000-system.t b/t/t9000-system.t
@@ -13,6 +13,8 @@ if test -n "$FLUX_ENABLE_SYSTEM_TESTS"; then
 		FLUX_TEST_INSTALLED_PATH=${FLUX_TEST_INSTALLED_PATH:-/usr/bin}
 	fi
 fi
+# Append --logfile option if FLUX_TESTS_LOGFILE is set in environment:
+test -n "$FLUX_TESTS_LOGFILE" && set -- "$@" --logfile
 . `dirname $0`/sharness.sh
 
 #  Do not run system tests by default unless FLUX_ENABLE_SYSTEM_TESTS
