Merge pull request #6452 from grondo/flux-start-newargs

add `-S, --setattr` and `-c, --config-path` options directly to `flux start`

doc/man1/flux-start.rst

@@ -122,6 +122,15 @@ as long as the Flux instance is running.  It covers the following use cases:
 
 OPTIONS
 =======
+.. option:: -S, --setattr=ATTR=VAL
+
+   Set broker attribute *ATTR* to *VAL*. This is equivalent to
+   :option:`-o,-SATTR=VAL`.
+
+.. option:: -c, --config-path=PATH
+
+   Set the *PATH* for broker configuration. See :man1:`flux-broker` for
+   option details. This is equivalent to :option:`-o,-cPATH`.
 
 .. option:: -o, --broker-opts=OPTIONS
 
@@ -282,7 +291,7 @@ If :program:`flux start` appears to hang, the following tips may be helpful:
    See :man7:`flux-environment`.
 
 #. More logging can be enabled by adding the
-   :option:`flux start -o,-Slog-stderr-level=7` option, which instructs the
+   :option:`flux start -Slog-stderr-level=7` option, which instructs the
    broker to forward its internal log buffer to stderr.  See
    :man7:`flux-broker-attributes`.
 

doc/test/spell.en.pws

@@ -936,3 +936,5 @@ cgroups
 fs
 misconfigured
 aTGTz
+cPATH
+SATTR

src/cmd/flux-start.c

@@ -99,6 +99,11 @@ const char *default_statedir = "/var/lib/flux";
 
 const char *usage_msg = "[OPTIONS] command ...";
 static struct optparse_option opts[] = {
+    { .name = "setattr",    .key = 'S', .has_arg = 1, .arginfo = "ATTR=VAL",
+      .flags = OPTPARSE_OPT_AUTOSPLIT,
+      .usage = "Set broker attribute", },
+    { .name = "config-path",.key = 'c', .has_arg = 1, .arginfo = "PATH",
+      .usage = "Set broker config from PATH (default: none)", },
     { .name = "recovery",   .key = 'r', .has_arg = 2, .arginfo = "[TARGET]",
       .flags = OPTPARSE_OPT_SHORTOPT_OPTIONAL_ARG,
       .usage = "Start instance in recovery mode with dump file or statedir", },
@@ -309,7 +314,9 @@ char *find_broker (const char *searchpath)
     return dir ? xstrdup (path) : NULL;
 }
 
-void exit_timeout (flux_reactor_t *r, flux_watcher_t *w, int revents, void *arg)
+void exit_timeout (flux_reactor_t *r,
+                   flux_watcher_t *w,
+                   int revents, void *arg)
 {
     struct client *cli;
 
@@ -447,28 +454,30 @@ void channel_cb (flux_subprocess_t *p, const char *stream)
     }
 }
 
-void add_args_list (char **argz,
-                    size_t *argz_len,
-                    optparse_t *opt,
-                    const char *name)
-{
-    const char *arg;
-    optparse_getopt_iterator_reset (opt, name);
-    while ((arg = optparse_getopt_next (opt, name)))
-        if (argz_add  (argz, argz_len, arg) != 0)
-            log_err_exit ("argz_add");
-}
-
 void add_argzf (char **argz, size_t *argz_len, const char *fmt, ...)
 {
     va_list ap;
-    char arg[1024];
+    char *arg = NULL;
 
     va_start (ap, fmt);
-    (void)vsnprintf (arg, sizeof (arg), fmt, ap);
+    if (vasprintf (&arg, fmt, ap) < 0)
+        log_err_exit ("vasprintf");
     va_end (ap);
     if (argz_add (argz, argz_len, arg) != 0)
         log_err_exit ("argz_add");
+    free (arg);
+}
+
+void add_args_list (char **argz,
+                    size_t *argz_len,
+                    optparse_t *opt,
+                    const char *name,
+                    const char *prepend)
+{
+    const char *arg;
+    optparse_getopt_iterator_reset (opt, name);
+    while ((arg = optparse_getopt_next (opt, name)))
+        add_argzf (argz, argz_len, "%s%s", prepend, arg);
 }
 
 char *create_rundir (void)
@@ -602,6 +611,39 @@ void process_recovery_option (char **argz,
         add_argzf (argz, argz_len, "-Scontent.restore=%s", optarg);
 }
 
+int add_args_common (char **argz,
+                     size_t *argz_len,
+                     const char *broker_path)
+{
+    bool system_recovery = false;
+    const char *config_path = NULL;
+
+    add_args_list (argz, argz_len, ctx.opts, "wrap", "");
+    if (argz_add (argz, argz_len, broker_path) != 0) {
+        errno = ENOMEM;
+        return -1;
+    }
+    add_args_list (argz, argz_len, ctx.opts, "setattr", "-S");
+    add_args_list (argz, argz_len, ctx.opts, "broker-opts", "");
+
+    if (optparse_hasopt (ctx.opts, "recovery"))
+        process_recovery_option (argz, argz_len, &system_recovery);
+
+    if (system_recovery || optparse_hasopt (ctx.opts, "sysconfig"))
+        config_path = default_config_path;
+    if (optparse_hasopt (ctx.opts, "config-path")) {
+        const char *conf = optparse_get_str (ctx.opts, "config-path", NULL);
+        if (config_path)
+            log_msg ("Warning: overriding recovery/--sysconfig path with %s",
+                     conf);
+        config_path = conf;
+    }
+    if (config_path)
+        add_argzf (argz, argz_len, "-c%s", config_path);
+
+    return 0;
+}
+
 /* Directly exec() a single flux broker.  It is assumed that we
  * are running in an environment with an external PMI service, and the
  * broker will figure out how to bootstrap without any further aid from
@@ -613,18 +655,10 @@ int exec_broker (const char *cmd_argz,
 {
     char *argz = NULL;
     size_t argz_len = 0;
-    bool system_recovery = false;
-
-    add_args_list (&argz, &argz_len, ctx.opts, "wrap");
-    if (argz_add (&argz, &argz_len, broker_path) != 0)
-        goto nomem;
-    add_args_list (&argz, &argz_len, ctx.opts, "broker-opts");
 
-    if (optparse_hasopt (ctx.opts, "recovery"))
-        process_recovery_option (&argz, &argz_len, &system_recovery);
+    if (add_args_common (&argz, &argz_len, broker_path) < 0)
+        goto error;
 
-    if (system_recovery || optparse_hasopt (ctx.opts, "sysconfig"))
-        add_argzf (&argz, &argz_len, "-c%s", default_config_path);
     if (cmd_argz) {
         if (argz_append (&argz, &argz_len, cmd_argz, cmd_argz_len) != 0)
             goto nomem;
@@ -660,23 +694,18 @@ struct client *client_create (const char *broker_path,
 {
     struct client *cli = xzmalloc (sizeof (*cli));
     char *arg;
-    char * argz = NULL;
+    char *argz = NULL;
     size_t argz_len = 0;
-    bool system_recovery = false;
 
     cli->rank = rank;
-    add_args_list (&argz, &argz_len, ctx.opts, "wrap");
-    argz_add (&argz, &argz_len, broker_path);
-    char *dir_arg = xasprintf ("--setattr=rundir=%s", rundir);
-    argz_add (&argz, &argz_len, dir_arg);
-    free (dir_arg);
-    if (optparse_hasopt (ctx.opts, "recovery") && rank == 0)
-        process_recovery_option (&argz, &argz_len, &system_recovery);
-    if (system_recovery || optparse_hasopt (ctx.opts, "sysconfig"))
-        add_argzf (&argz, &argz_len, "-c%s", default_config_path);
-    add_args_list (&argz, &argz_len, ctx.opts, "broker-opts");
-    if (rank == 0 && cmd_argz)
-        argz_append (&argz, &argz_len, cmd_argz, cmd_argz_len); /* must be last arg */
+
+    if (add_args_common (&argz, &argz_len, broker_path) < 0)
+        goto fail;
+
+    add_argzf (&argz, &argz_len, "--setattr=rundir=%s", rundir);
+
+    if (rank == 0 && cmd_argz) /* must be last arg */
+        argz_append (&argz, &argz_len, cmd_argz, cmd_argz_len);
 
     if (!(cli->cmd = flux_cmd_create (0, NULL, environ)))
         goto fail;
@@ -705,8 +734,8 @@ struct client *client_create (const char *broker_path,
             log_err_exit ("error setting up environment for rank %d", rank);
     return cli;
 fail:
-    free (argz);
-    client_destroy (cli);
+    ERRNO_SAFE_WRAP (free, argz);
+    ERRNO_SAFE_WRAP (client_destroy, cli);
     return NULL;
 }
 
@@ -739,7 +768,9 @@ void client_dumpargs (struct client *cli)
 void pmi_server_initialize (int flags)
 {
     struct taskmap *map;
-    const char *mode = optparse_get_str (ctx.opts, "test-pmi-clique", "single");
+    const char *mode = optparse_get_str (ctx.opts,
+                                         "test-pmi-clique",
+                                         "single");
     struct pmi_simple_ops ops = {
         .abort = pmi_abort,
         .kvs_put = pmi_kvs_put,
@@ -1089,8 +1120,10 @@ int start_session (const char *cmd_argz,
     if (!(ctx.reactor = flux_reactor_create (FLUX_REACTOR_SIGCHLD)))
         log_err_exit ("flux_reactor_create");
     if (!(ctx.timer = flux_timer_watcher_create (ctx.reactor,
-                                                 ctx.exit_timeout, 0.,
-                                                 exit_timeout, NULL)))
+                                                 ctx.exit_timeout,
+                                                 0.,
+                                                 exit_timeout,
+                                                 NULL)))
         log_err_exit ("flux_timer_watcher_create");
     if (!(ctx.clients = zlist_new ()))
         log_err_exit ("zlist_new");

t/fluxometer.lua

@@ -133,7 +133,7 @@ function fluxTest.init (...)
     end
 
     test.log_file = "lua-"..test.prog..".broker.log"
-    test.start_args = { "-o,-Slog-filename=" .. test.log_file }
+    test.start_args = { "-Slog-filename=" .. test.log_file }
 
     local path = fluxTest.fluxbindir .. "/flux"
     local mode = posix.stat (path, 'mode')

t/issues/t3906-job-exec-exception.sh

@@ -7,7 +7,7 @@
 #  3. Ensure job exception is raised and includes appropriate error note
 #
 export startctl="flux python ${SHARNESS_TEST_SRCDIR}/scripts/startctl.py"
-SHELL=/bin/sh flux start -s 4 -o,-Stbon.topo=kary:4 --test-exit-mode=leader '\
+SHELL=/bin/sh flux start -s 4 -Stbon.topo=kary:4 --test-exit-mode=leader '\
    id=$(flux submit -n4 -N4 sleep 300) \
 && flux job wait-event $id start \
 && $startctl kill 3 9 \

t/issues/t3960-job-exec-ehostunreach.sh

@@ -12,7 +12,7 @@
 #
 #
 export startctl="flux python ${SHARNESS_TEST_SRCDIR}/scripts/startctl.py"
-SHELL=/bin/sh flux start -s 4 -o,-Stbon.topo=kary:4 --test-exit-mode=leader '\
+SHELL=/bin/sh flux start -s 4 -Stbon.topo=kary:4 --test-exit-mode=leader '\
    id=$(flux submit -n4 -N4 sleep 300) \
 && flux job wait-event $id start \
 && $startctl kill 3 19 \

t/issues/t4182-resource-rerank.sh

@@ -40,7 +40,7 @@ echo "resource.noverify = true" >t4182-resource.toml
 
 #  ensure R rerank failure is ignored (i.e. job completes successfully)
 flux run -o per-resource.type=node -o cpu-affinity=off -n 11 \
-	flux start -o,--config-path=t4182-resource.toml \
+	flux start --config-path=t4182-resource.toml \
 	flux getattr hostlist
 
 #  ensure R is reranked based on hostlist attribute:

t/issues/t4465-job-list-use-after-free.sh

@@ -22,10 +22,10 @@ mkdir ${STATEDIR}
 # a different order than submitted, hence number of jobs submitted equals
 # number of broker ranks.
 #
-flux start --test-size=8 -o,-Sstatedir=${STATEDIR} \
+flux start --test-size=8 -Sstatedir=${STATEDIR} \
     bash -c "flux submit --cc 1-8 --quiet /bin/true && flux queue drain"
 
-flux start --test-size=1 -o,-Sstatedir=${STATEDIR} \
+flux start --test-size=1 -Sstatedir=${STATEDIR} \
     --wrap=libtool,e,${VALGRIND} \
     --wrap=--tool=memcheck \
     --wrap=--trace-children=no \

t/issues/t4482-flush-list-corruption.sh

@@ -38,6 +38,6 @@ EOF
 chmod +x t4482.sh
 
 flux start -s 1 \
-    -o,--setattr=broker.rc1_path= \
-    -o,--setattr=broker.rc3_path= \
+    --setattr=broker.rc1_path= \
+    --setattr=broker.rc3_path= \
     ./t4482.sh

t/issues/t4583-free-range-test.sh

@@ -31,7 +31,7 @@ if test "$FREE_RANGE_TEST_ACTIVE" != "t"; then
     exec flux start -s 4 \
         --test-exit-mode=leader \
         --test-pmi-clique=per-broker \
-        -o -Stbon.topo=kary:0 $0
+        -Stbon.topo=kary:0 $0
 fi
 
 #  Start a job with tbon.topo=kary:0

t/issues/t4852-t_submit-legacy.sh

@@ -41,9 +41,9 @@ STATEDIR=issue4852-statedir
 mkdir issue4852-statedir
 
 flux start -s 1 \
-    -o,--setattr=statedir=${STATEDIR} \
+    --setattr=statedir=${STATEDIR} \
     ./t4852setup.sh
 
 flux start -s 1 \
-    -o,--setattr=statedir=${STATEDIR} \
+    --setattr=statedir=${STATEDIR} \
     ./t4852test.sh

t/issues/t5892-shutdown-no-epilog.sh

@@ -12,7 +12,7 @@ plugins = [
 command = [ "touch", "t5892-epilog-flag" ]
 EOF
 
-flux start -o,--config-path=t5892.toml \
+flux start --config-path=t5892.toml \
 	flux submit sleep inf
 
 rc=0

t/python/subflux.py

@@ -60,10 +60,10 @@ def rerun_under_flux(size=1, personality="full"):
         for rc_num in [1, 3]:
             attr = "broker.rc{}_path".format(rc_num)
             if personality == "minimal":
-                command.append("-o,-S{}=".format(attr))
+                command.append("-S{}=".format(attr))
             else:
                 path = "{}/t/rc/rc{}-{}".format(srcdir, rc_num, personality)
-                command.append("-o,-S{}={}".format(attr, path))
+                command.append("-S{}={}".format(attr, path))
                 if not is_exe(path):
                     print("cannot execute {}".format(path), file=sys.stderr)
                     sys.exit(1)

t/sharness.d/flux-sharness.sh

@@ -118,14 +118,14 @@ make_bootstrap_config() {
 	    path = "$workdir/R"
 	    noverify = true
 	EOT2
-    echo "--test-hosts=$fakehosts -o,-c$workdir/conf.d"
+    echo "--test-hosts=$fakehosts -c$workdir/conf.d"
     echo "--test-exit-mode=${TEST_UNDER_FLUX_EXIT_MODE:-leader}"
     echo "--test-exit-timeout=${TEST_UNDER_FLUX_EXIT_TIMEOUT:-0}"
-    echo "-o,-Sbroker.quorum=${TEST_UNDER_FLUX_QUORUM:-$size}"
+    echo "-Sbroker.quorum=${TEST_UNDER_FLUX_QUORUM:-$size}"
     echo "--test-start-mode=${TEST_UNDER_FLUX_START_MODE:-all}"
-    echo "-o,-Stbon.topo=${TEST_UNDER_FLUX_TOPO:-custom}"
-    echo "-o,-Stbon.zmqdebug=1"
-    echo "-o,-Sstatedir=$workdir/state"
+    echo "-Stbon.topo=${TEST_UNDER_FLUX_TOPO:-custom}"
+    echo "-Stbon.zmqdebug=1"
+    echo "-Sstatedir=$workdir/state"
 }
 
 #
@@ -263,7 +263,7 @@ test_under_flux() {
         # Set log_path for ASan o/w errors from broker may be lost
         ASAN_OPTIONS=${ASAN_OPTIONS}:log_path=${TEST_NAME}.asan
     fi
-    logopts="-o -Slog-filename=${log_file},-Slog-forward-level=7"
+    logopts="-o -Slog-filename=${log_file} -Slog-forward-level=7"
     TEST_UNDER_FLUX_ACTIVE=t \
     TERM=${ORIGINAL_TERM} \
     TEST_UNDER_FLUX_PERSONALITY="${personality:-default}" \