Merge pull request #4248 from garlick/statedir

broker: add statedir attribute, drop content.backing-path

doc/man7/flux-broker-attributes.rst

@@ -33,15 +33,23 @@ version
    The version of flux-core that was used to build this broker.
 
 rundir [Updates: C]
-   A temporary directory where UNIX domain sockets and the default
-   content.backing-path are located (see below).  By default, each broker
-   rank creates a unique rundir in $TMPDIR and removes it on exit.  If
-   rundir is set on the command line, beware exceeding the UNIX domain socket
-   path limit described in :linux:man7:`unix`, as low as 92 bytes on
-   some systems.  If rundir is set to a pre-existing directory, the
-   directory is not removed on exit; if the broker has to create the
-   directory, it is removed.  In most cases this attribute should not
-   be set by users.
+   A temporary directory where the broker's UNIX domain sockets are located.
+   In addition, if ``statedir`` is not set, this directory is used by the
+   content backing store (if applicable).  By default, each broker rank creates
+   a unique ``rundir`` in ``$TMPDIR`` and removes it on exit.  If ``rundir`` is
+   set on the command line, beware exceeding the UNIX domain socket path limit
+   described in :linux:man7:`unix`, as low as 92 bytes on some systems.  To
+   support the :man1:`flux-start` ``--test-size`` option where multiple brokers
+   share a ``rundir``, if ``rundir`` is set to a pre-existing directory, the
+   directory is not removed by the broker on exit.  In most cases this
+   attribute should not be set by users.
+
+statedir [Updates: C]
+   A directory in which persistent state is stored by the Flux broker.  For
+   example, content backing store data is stored here to facilitate restarts.
+   If unset, this data goes to ``rundir`` where it is cleaned up on instance
+   shutdown.  If set, this directory must exist and be owned by the instance
+   owner.  Default: unset.
 
 security.owner
    The numeric userid of the owner of this Flux instance.
@@ -239,14 +247,6 @@ content.backing-module (Updates: C)
    on rank 0 where the content backing store is active.  Default:
    ``content-sqlite``.
 
-content.backing-path (Updates: C)
-   The path to the content backing store file(s). If this is set on the
-   broker command line, the backing store uses this path instead of
-   a temporary one, and content is preserved on instance exit.
-   If file exists, its content is imported into the instance.
-   If it doesn't exist, it is created.  Default: ``content.sqlite``
-   located within ``rundir``.
-
 content.blob-size-limit (Updates: C, R)
    The maximum size of a blob, the basic unit of content storage.
    Default: ``1073741824``.

doc/test/spell.en.pws

@@ -621,3 +621,4 @@ POSIX
 ustar
 xz
 validator
+statedir

etc/flux.service.in

@@ -13,10 +13,10 @@ ExecStart=/bin/bash -c '\
   -Scron.directory=@X_SYSCONFDIR@/flux/system/cron.d \
   -Stbon.fanout=256 \
   -Srundir=@X_RUNSTATEDIR@/flux \
+  -Sstatedir=${STATE_DIRECTORY:-/var/lib/flux} \
   -Slocal-uri=local://@X_RUNSTATEDIR@/flux/local \
   -Slog-stderr-level=6 \
   -Slog-stderr-mode=local \
-  -Scontent.backing-path=${STATE_DIRECTORY:-/var/lib/flux}/content.sqlite \
   -Sbroker.rc2_none \
   -Sbroker.quorum=0 \
   -Sbroker.quorum-timeout=none \

src/broker/broker.c

@@ -91,6 +91,7 @@ static int unload_module_byname (broker_ctx_t *ctx, const char *name,
 static void set_proctitle (uint32_t rank);
 
 static int create_rundir (attr_t *attrs);
+static int check_statedir (attr_t *attrs);
 
 static int create_runat_phases (broker_ctx_t *ctx);
 
@@ -308,6 +309,8 @@ int main (int argc, char *argv[])
 
     if (create_rundir (ctx.attrs) < 0)
         goto cleanup;
+    if (check_statedir (ctx.attrs) < 0)
+        goto cleanup;
 
     /* Record the broker start time.  This time will also be used to
      * capture how long network bootstrap takes.
@@ -735,6 +738,11 @@ static int checkdir (const char *name, const char *path)
         log_err ("cannot stat %s %s", name, path);
         return -1;
     }
+    if (sb.st_uid != getuid ()) {
+        errno = EPERM;
+        log_err ("%s %s is not owned by instance owner", name, path);
+        return -1;
+    }
     if (!S_ISDIR (sb.st_mode)) {
         errno = ENOTDIR;
         log_err ("%s %s", name, path);
@@ -748,6 +756,29 @@ static int checkdir (const char *name, const char *path)
     return 0;
 }
 
+/* Validate statedir, if set.
+ * Ensure that the attribute cannot change from this point forward.
+ */
+static int check_statedir (attr_t *attrs)
+{
+    const char *statedir;
+
+    if (attr_get (attrs, "statedir", &statedir, NULL) < 0) {
+        if (attr_add (attrs, "statedir", NULL, FLUX_ATTRFLAG_IMMUTABLE) < 0) {
+            log_err ("error creating statedir broker attribute");
+            return -1;
+        }
+    }
+    else {
+        if (checkdir ("statedir", statedir) < 0)
+            return -1;
+        if (attr_set_flags (attrs, "statedir", FLUX_ATTRFLAG_IMMUTABLE) < 0) {
+            log_err ("error setting statedir broker attribute flags");
+            return -1;
+        }
+    }
+    return 0;
+}
 
 /*  Handle global rundir attribute.
  */

src/modules/content-files/content-files.c

@@ -256,7 +256,7 @@ static const struct flux_msg_handler_spec htab[] = {
 static struct content_files *content_files_create (flux_t *h)
 {
     struct content_files *ctx;
-    const char *backing_path;
+    const char *dbdir;
 
     if (!(ctx = calloc (1, sizeof (*ctx))))
         return NULL;
@@ -271,33 +271,19 @@ static struct content_files *content_files_create (flux_t *h)
         goto error;
     }
 
-    /* If 'content.backing-path' attribute is already set, then:
-     * - value is the db directory
-     * - if it exists, preserve existing content; else create empty
-     * Otherwise:
-     * - ${rundir}/content.files is the backing path
-     * - set 'content.backing-path' to this name
-     * - ${rundir} is cleaned up recursively by broker atexit(3) handler
+    /* Prefer 'statedir' as the location for the content.files directory,
+     * if set.  Otherwise use 'rundir'.  If the directory exists, the
+     * instance is restarting.
      */
-    backing_path = flux_attr_get (h, "content.backing-path");
-    if (backing_path) {
-        if (!(ctx->dbpath = strdup (backing_path)))
-            goto error;
-        if (mkdir (ctx->dbpath, 0700) < 0 && errno != EEXIST)
-            goto error;
-    }
-    else {
-        const char *rundir = flux_attr_get (h, "rundir");
-        if (!rundir) {
-            flux_log_error (h, "rundir");
-            goto error;
-        }
-        if (asprintf (&ctx->dbpath, "%s/content.files", rundir) < 0)
-            goto error;
-        if (flux_attr_set (h, "content.backing-path", ctx->dbpath) < 0)
-            goto error;
-        if (mkdir (ctx->dbpath, 0700) < 0)
-            goto error;
+    if (!(dbdir = flux_attr_get (h, "statedir")))
+        dbdir = flux_attr_get (h, "rundir");
+    if (!dbdir)
+        flux_log_error (h, "neither statedir nor rundir are set");
+    if (asprintf (&ctx->dbpath, "%s/content.files", dbdir) < 0)
+        goto error;
+    if (mkdir (ctx->dbpath, 0700) < 0 && errno != EEXIST) {
+        flux_log_error (h, "could not create %s", ctx->dbpath);
+        goto error;
     }
     if (flux_msg_handler_addvec (h, htab, ctx, &ctx->handlers) < 0)
         goto error;

src/modules/content-sqlite/content-sqlite.c

@@ -592,7 +592,7 @@ static const struct flux_msg_handler_spec htab[] = {
 static struct content_sqlite *content_sqlite_create (flux_t *h)
 {
     struct content_sqlite *ctx;
-    const char *backing_path;
+    const char *dbdir;
 
     if (!(ctx = calloc (1, sizeof (*ctx))))
         return NULL;
@@ -611,36 +611,26 @@ static struct content_sqlite *content_sqlite_create (flux_t *h)
         goto error;
     }
 
-    /* If 'content.backing-path' attribute is already set, then:
-     * - value is the sqlite backing file
-     * - if it exists, preserve existing content; else create empty
-     * Otherwise:
-     * - ${rundir}/content.sqlite is the backing file
-     * - ${rundir} is normally recursively cleaned up when the instance exits
-     * - set 'content.backing-path' to this name
+    /* Prefer 'statedir' as the location for content.sqlite file, if set.
+     * Otherwise use 'rundir'.
      */
-    backing_path = flux_attr_get (h, "content.backing-path");
-    if (backing_path) {
-        if (!(ctx->dbfile = strdup (backing_path)))
-            goto error;
-        if (access (ctx->dbfile, F_OK) == 0) {
-            if (access (ctx->dbfile, R_OK | W_OK) < 0) {
-                flux_log_error (h, "%s", ctx->dbfile);
-                goto error;
-            }
-        }
-    }
-    else {
-        const char *rundir = flux_attr_get (h, "rundir");
-        if (!rundir) {
-            flux_log_error (h, "rundir");
+    if (!(dbdir = flux_attr_get (h, "statedir")))
+        dbdir = flux_attr_get (h, "rundir");
+    if (!dbdir)
+        flux_log_error (h, "neither statedir nor rundir are set");
+    if (asprintf (&ctx->dbfile, "%s/content.sqlite", dbdir) < 0)
+        goto error;
+
+    /* If dbfile exists, we are restarting.
+     * If existing dbfile does not have the right permissions, fail early.
+     */
+    if (access (ctx->dbfile, F_OK) == 0) {
+        if (access (ctx->dbfile, R_OK | W_OK) < 0) {
+            flux_log_error (h, "%s", ctx->dbfile);
             goto error;
         }
-        if (asprintf (&ctx->dbfile, "%s/content.sqlite", rundir) < 0)
-            goto error;
-        if (flux_attr_set (h, "content.backing-path", ctx->dbfile) < 0)
-            goto error;
     }
+
     if (flux_msg_handler_addvec (h, htab, ctx, &ctx->handlers) < 0)
         goto error;
     return ctx;
@@ -657,7 +647,7 @@ int mod_main (flux_t *h, int argc, char **argv)
         flux_log_error (h, "content_sqlite_create failed");
         return -1;
     }
-    if (content_sqlite_opendb(ctx) < 0)
+    if (content_sqlite_opendb (ctx) < 0)
         goto done;
     if (content_register_backing_store (h, "content-sqlite") < 0)
         goto done;

t/issues/t4222-kvs-assume-empty-dir.sh

@@ -8,7 +8,7 @@ flux content flush
 flux content dropcache
 flux module remove content-sqlite
 
-sqlitepath=`flux getattr content.backing-path`
+sqlitepath=$(flux getattr rundir)/content.sqlite
 mv $sqlitepath $sqlitepath.bak
 
 flux module load content-sqlite

t/t0001-basic.t

@@ -402,19 +402,44 @@ test_expect_success 'broker fails gracefully on unwriteable rundir' '
 		/bin/true 2>privdir.err &&
 	grep "permissions" privdir.err
 '
+# statedir created here is reused in the next several tests
+test_expect_success 'broker statedir is not cleaned up' '
+	mkdir -p statedir &&
+	flux start ${ARGS} -o,-Sstatedir=$(pwd)/statedir /bin/true &&
+	test -d statedir
+'
+test_expect_success 'broker statedir cannot be changed at runtime' '
+	test_must_fail flux start ${ARGS} -o,-Sstatedir=$(pwd)/statedir \
+		flux setattr statedir $(pwd)/statedir 2>rostatedir.err &&
+	grep "Operation not permitted" rostatedir.err
+'
+test_expect_success 'broker statedir cannot be set at runtime' '
+	test_must_fail flux start ${ARGS} \
+		flux setattr statedir $(pwd)/statedir 2>rostatedir2.err &&
+	grep "Operation not permitted" rostatedir2.err
+'
+test_expect_success 'broker fails when statedir does not exist' '
+	rm -rf statedir &&
+	test_must_fail flux start ${ARGS} -o,-Sstatedir=$(pwd)/statedir \
+		/bin/true 2>nostatedir.err &&
+	grep "cannot stat" nostatedir.err
+'
 # Use -eq hack to test that BROKERPID is a number
 test_expect_success 'broker broker.pid attribute is readable' '
 	BROKERPID=`flux start ${ARGS} flux getattr broker.pid` &&
 	test -n "$BROKERPID" &&
 	test "$BROKERPID" -eq "$BROKERPID"
 '
+
 test_expect_success 'local-uri override works' '
-	newsock=local:///tmp/meep &&
+	sockdir=$(mktemp -d) &&
+	newsock=local://$sockdir/meep &&
 	echo $newsock >uri.exp &&
 	flux start ${ARGS} \
 		-o,-Slocal-uri=$newsock \
 		printenv FLUX_URI >uri.out &&
-	test_cmp uri.exp uri.out
+	test_cmp uri.exp uri.out &&
+	rm -rf $sockdir
 '
 test_expect_success 'broker fails gracefully when local-uri is malformed' '
 	test_must_fail flux start ${ARGS} -o,-Slocal-uri=baduri \

t/t0012-content-sqlite.t

@@ -201,7 +201,7 @@ test_expect_success 'remove heartbeat module' '
 
 # test for issue #4210
 test_expect_success 'remove read permission from content.sqlite file' '
-	chmod u-w $(flux getattr content.backing-path) &&
+	chmod u-w $(flux getattr rundir)/content.sqlite &&
 	test_must_fail flux module load content-sqlite
 '
 

t/t0018-content-files.t

@@ -8,7 +8,7 @@ if test "$TEST_LONG" = "t"; then
     test_set_prereq LONGTEST
 fi
 
-test_under_flux 1 minimal
+test_under_flux 1 minimal -o,-Sstatedir=$(pwd)
 
 BLOBREF=${FLUX_BUILD_DIR}/t/kvs/blobref
 RPC=${FLUX_BUILD_DIR}/t/request/rpc
@@ -76,15 +76,10 @@ test_expect_success 'load content-files module' '
 	flux module load content-files testing
 '
 
-test_expect_success 'content.backing-path attribute is set' '
-	FILEDB=$(flux getattr content.backing-path) &&
-	test -d ${FILEDB}
-'
-
 test_expect_success 'load/store/verify key-values stored directly' '
 	make_blob 140 >rawblob.140 &&
-	$TEST_STORE $FILEDB testkey1 <rawblob.140 &&
-	$TEST_LOAD $FILEDB testkey1 >rawblob.140.out &&
+	$TEST_STORE $(pwd)/content.files testkey1 <rawblob.140 &&
+	$TEST_LOAD $(pwd)/content.files testkey1 >rawblob.140.out &&
 	test_cmp rawblob.140 rawblob.140.out
 '
 

t/t0024-content-s3.t

@@ -280,13 +280,13 @@ EOF
         chmod +x rc3-content-s3
 '
 
-test_expect_success 'run instance with content.backing-path set (s3)' '
+test_expect_success 'run instance with content-s3 module loaded' '
 	flux start -o,--setattr=broker.rc1_path=$(pwd)/rc1-content-s3 \
                    -o,--setattr=broker.rc3_path=$(pwd)/rc3-content-s3 \
 	           flux kvs put testkey=43
 '
 
-test_expect_success 're-run instance with content.backing-path set (s3)' '
+test_expect_success 're-run instance with content-s3 module loaded' '
 	flux start -o,--setattr=broker.rc1_path=$(pwd)/rc1-content-s3 \
                    -o,--setattr=broker.rc3_path=$(pwd)/rc3-content-s3 \
 	           flux kvs get testkey >gets3.out

t/t2008-althash.t

@@ -30,7 +30,7 @@ test_expect_success 'Started instance with content.hash=sha256' '
 test_expect_success 'Started instance with content.hash=sha256,content-files' '
     OUT=$(flux start -o,-Scontent.hash=sha256 \
           -o,-Scontent.backing-module=content-files \
-          -o,-Scontent.backing-path=$(pwd)/content.files \
+          -o,-Sstatedir=$(pwd) \
           flux getattr content.hash) && test "$OUT" = "sha256" &&
     ls -1 content.files | tail -1 | grep sha256
 '