Merge pull request #3159 from grondo/plugin-improvement-project

improve support for shell plugin developers

doc/man1/flux-shell.rst

@@ -119,11 +119,19 @@ name may be loaded at runtime.
    to Flux services may be restricted as a guest.
 
 C plugins are defined using the Flux standard plugin format. A shell C
-plugin should therefore export a single symbol ``flux_plugin_init``, in
+plugin should therefore export a single symbol ``flux_plugin_init()``, in
 which calls to ``flux_plugin_add_handler(3)`` should be used to register
-functions which will be invoked at defined points. These callbacks are
-defined by "topic strings" to which plugins can "subscribe" by calling
-with a topic glob string.
+functions which will be invoked at defined points during shell execution.
+These callbacks are defined by "topic strings" to which plugins can
+"subscribe" by calling ``flux_plugin_add_handler(3)`` and/or
+``flux_plugin_register(3)`` with topic ``glob(7)`` strings.
+
+.. note::
+   ``flux_plugin_init(3)`` is not called for builtin shell plugins. If
+   a dynamically loaded plugin wishes to set shell options to influence
+   a shell builtin plugin (e.g. to disable its operation), it should
+   therefore do so in ``flux_plugin_init()`` in order to guarantee that
+   the shell option is set before the builtin attempts to read them.
 
 Simple plugins may also be developed directly in the shell ``initrc.lua``
 file itself (see INITRC section, ``plugin.register()`` below)

etc/flux-core.pc.in

@@ -2,6 +2,17 @@ prefix=@prefix@
 exec_prefix=@exec_prefix@
 libdir=@libdir@
 includedir=@includedir@
+datarootdir=@datarootdir@
+mandir=@mandir@
+fluxrcdir=@fluxrcdir@
+fluxrc1dir=@fluxrc1dir@
+fluxrc3dir=@fluxrc3dir@
+fluxcmddir=@fluxcmddir@
+fluxlibdir=@fluxlibdir@
+fluxmoddir=@fluxmoddir@
+fluxcmdhelpdir=@datadir@/flux/help.d
+fluxshellrcdir=@fluxrcdir@/shell
+fluxshellpluginpath=@fluxlibdir@/shell/plugins
 
 Name: flux-core
 Description: Flux Resource Manager Framework Core

src/broker/broker.c

@@ -714,6 +714,20 @@ static void init_attrs_rc_paths (attr_t *attrs)
         log_err_exit ("attr_add rc3_path");
 }
 
+static void init_attrs_shell_paths (attr_t *attrs)
+{
+    if (attr_add (attrs,
+                  "conf.shell_pluginpath",
+                  flux_conf_builtin_get ("shell_pluginpath", FLUX_CONF_AUTO),
+                  0) < 0)
+        log_err_exit ("attr_add conf.shell_pluginpath");
+    if (attr_add (attrs,
+                  "conf.shell_initrc",
+                  flux_conf_builtin_get ("shell_initrc", FLUX_CONF_AUTO),
+                  0) < 0)
+        log_err_exit ("attr_add conf.shell_initrc");
+}
+
 static void init_attrs (attr_t *attrs, pid_t pid)
 {
     /* Initialize config attrs from environment set up by flux(1)
@@ -724,6 +738,7 @@ static void init_attrs (attr_t *attrs, pid_t pid)
      */
     init_attrs_broker_pid (attrs, pid);
     init_attrs_rc_paths (attrs);
+    init_attrs_shell_paths (attrs);
 
     if (attr_add (attrs, "version", FLUX_CORE_VERSION_STRING,
                                             FLUX_ATTRFLAG_IMMUTABLE) < 0)

src/common/libflux/plugin.c

@@ -29,6 +29,7 @@ struct flux_plugin {
     char *path;
     char *name;
     json_t *conf;
+    char *conf_str;
     struct aux_item *aux;
     void *dso;
     zlistx_t *handlers;
@@ -105,6 +106,7 @@ void flux_plugin_destroy (flux_plugin_t *p)
         int saved_errno = errno;
         json_decref (p->conf);
         zlistx_destroy (&p->handlers);
+        free (p->conf_str);
         free (p->path);
         free (p->name);
         aux_destroy (&p->aux);
@@ -223,9 +225,36 @@ int flux_plugin_set_conf (flux_plugin_t *p, const char *json_str)
                                 "parse error: col %d: %s",
                                  err.column, err.text);
     }
+    if (p->conf_str) {
+        free (p->conf_str);
+        p->conf_str = NULL;
+    }
     return 0;
 }
 
+const char *flux_plugin_get_conf (flux_plugin_t *p)
+{
+    if (!p) {
+        plugin_seterror (p, EINVAL, NULL);
+        return NULL;
+    }
+    if (!p->conf_str) {
+        if (!p->conf) {
+            plugin_seterror (p, ENOENT, "No plugin conf set");
+            return NULL;
+        }
+        p->conf_str = json_dumps (p->conf, JSON_ENCODE_ANY|JSON_COMPACT);
+        if (!p->conf_str) {
+            plugin_seterror (p,
+                             errno,
+                             "json_dumps failed: %s",
+                             strerror (errno));
+            return NULL;
+        }
+    }
+    return p->conf_str;
+}
+
 int flux_plugin_conf_unpack (flux_plugin_t *p, const char *fmt, ...)
 {
     json_error_t err;

src/common/libflux/plugin.h

@@ -102,6 +102,11 @@ void * flux_plugin_aux_get (flux_plugin_t *p, const char *key);
  */
 int flux_plugin_set_conf (flux_plugin_t *p, const char *json_str);
 
+/*  Get the current JSON string value of config for plugin 'p'.
+ *  Returns NULL on failure.
+ */
+const char *flux_plugin_get_conf (flux_plugin_t *p);
+
 /*  Read configuration for plugin 'p' using jansson style unpack args */
 int flux_plugin_conf_unpack (flux_plugin_t *p, const char *fmt, ...);
 

src/common/libflux/test/plugin.c

@@ -78,6 +78,11 @@ void test_invalid_args ()
     like (flux_plugin_strerror (p), "^parse error: col 1:.*",
         "flux_plugin_last_error returns error text");
 
+    ok (flux_plugin_get_conf (NULL) == NULL && errno == EINVAL,
+        "flux_plugin_get_conf () with NULL arg returns EINVAL");
+    ok (flux_plugin_get_conf (p) == NULL && errno == ENOENT,
+        "flux_plugin_get_conf () with no conf returns ENOENT");
+
     ok (flux_plugin_conf_unpack (p, "{s:i}", "bar", &i) < 0 && errno == ENOENT,
         "flux_plugin_conf_unpack () with no conf returns ENOENT");
 
@@ -395,6 +400,11 @@ void test_load ()
 
     ok (flux_plugin_set_conf (p, "{\"foo\":\"bar\"}") == 0,
         "flux_plugin_set_conf (): %s", flux_plugin_strerror (p));
+    ok ((result = flux_plugin_get_conf (p)) != NULL,
+        "flux_plugin_get_conf () works");
+    ok (result != NULL,
+        "conf = %s", result);
+
     ok (flux_plugin_load_dso (p, "test/.libs/plugin_foo.so") == 0,
         "flux_plugin_load worked");
     is (flux_plugin_get_name (p), "plugin-test",

src/shell/affinity.c

@@ -271,8 +271,10 @@ static int affinity_init (flux_plugin_t *p,
 
     if (!shell)
         return shell_log_errno ("flux_plugin_get_shell");
-    if (!affinity_getopt (shell, &option))
+    if (!affinity_getopt (shell, &option)) {
+        shell_debug ("disabling affinity due to cpu-affinity=off");
         return 0;
+    }
     if (!(sa = shell_affinity_create (shell)))
         return shell_log_errno ("shell_affinity_create");
 

src/shell/gpubind.c

@@ -109,8 +109,17 @@ static int gpubind_init (flux_plugin_t *p,
         /* gpu-affinity defaults to "on" */
         opt = "on";
     }
-    if (strcmp (opt, "off") == 0)
+    if (strcmp (opt, "off") == 0) {
+        shell_debug ("disabling affinity due to gpu-affinity=off");
         return 0;
+    }
+
+    /*  Set default CUDA_VISIBLE_DEVICES to an invalid id, -1, so that
+     *  jobs which are not assigned any GPUs do not use GPUs which
+     *  happen to be available on the current node.
+     */
+    flux_shell_setenvf (shell, 0, "CUDA_VISIBLE_DEVICES", "%d", -1);
+
     if (get_shell_gpus (shell, &ntasks, &gpus) < 0)
         return -1;
     if (flux_plugin_aux_set (p, NULL, gpus, (flux_free_f)idset_destroy) < 0) {

src/shell/jobspec.c

@@ -117,6 +117,15 @@ struct jobspec *jobspec_parse (const char *jobspec, json_error_t *error)
         set_error (error, "attributes.system.environment is not object type");
         goto error;
     }
+    /* Ensure that shell options is never NULL, but instead is an empty
+     * object. This ensures that if a shell component or plugin wants to
+     * set a new option, that will work.
+     */
+    if (!job->options && !(job->options = json_object ())) {
+        set_error (error, "unable to create empty jobspec options");
+        goto error;
+    }
+
     /* For jobspec version 1, expect either:
      * - node->slot->core->NIL
      * - slot->core->NIL

src/shell/shell.c

@@ -95,12 +95,6 @@ int flux_shell_setopt (flux_shell_t *shell,
 {
     json_error_t err;
     json_t *o;
-    if (!shell->info->jobspec->options) {
-        if (!(shell->info->jobspec->options = json_object ())) {
-            errno = ENOMEM;
-            return -1;
-        }
-    }
     /* If flux_shell_setopt (shell, name, NULL), delete option:
      */
     if (!json_str)
@@ -955,7 +949,7 @@ static int shell_barrier (flux_shell_t *shell, const char *name)
     return rc;
 }
 
-static int load_initrc (flux_shell_t *shell)
+static int load_initrc (flux_shell_t *shell, const char *default_rcfile)
 {
     bool required = false;
     const char *rcfile = NULL;
@@ -967,7 +961,7 @@ static int load_initrc (flux_shell_t *shell)
         || flux_shell_getopt_unpack (shell, "initrc", "s", &rcfile) > 0)
         required = true;
     else
-        rcfile = shell_conf_get ("shell_initrc");
+        rcfile = default_rcfile;
 
     /* Skip loading initrc file if it is not required and either the shell
      *  is running in standalone mode, or the file isn't readable.
@@ -990,9 +984,22 @@ static int load_initrc (flux_shell_t *shell)
 
 static int shell_init (flux_shell_t *shell)
 {
+    const char *default_rcfile = shell_conf_get ("shell_initrc");
+
+    /*  Override pluginpath, default rcfile from broker attribute
+     *   if not in standalone mode.
+     */
+    if (!shell->standalone) {
+        const char *result;
+        if ((result = flux_attr_get (shell->h, "conf.shell_pluginpath")))
+            plugstack_set_searchpath (shell->plugstack, result);
+        if ((result = flux_attr_get (shell->h, "conf.shell_initrc")))
+            default_rcfile = result;
+    }
+
     /*  Load initrc file if necessary
      */
-    if (load_initrc (shell) < 0)
+    if (load_initrc (shell, default_rcfile) < 0)
         return -1;
 
     /* Change current working directory once before all tasks are

t/Makefile.am

@@ -296,6 +296,7 @@ check_LTLIBRARIES = \
 	shell/plugins/conftest.la \
 	shell/plugins/invalid-args.la \
 	shell/plugins/getopt.la \
+	shell/plugins/setopt.la \
 	shell/plugins/log.la \
 	shell/plugins/test-event.la
 
@@ -576,6 +577,13 @@ shell_plugins_getopt_la_LIBADD = \
 	$(top_builddir)/src/common/libtap/libtap.la \
         $(top_builddir)/src/common/libflux-core.la
 
+shell_plugins_setopt_la_SOURCES = shell/plugins/setopt.c
+shell_plugins_setopt_la_CPPFLAGS = $(test_cppflags)
+shell_plugins_setopt_la_LDFLAGS = -module -rpath /nowhere
+shell_plugins_setopt_la_LIBADD = \
+	$(top_builddir)/src/common/libtap/libtap.la \
+        $(top_builddir)/src/common/libflux-core.la
+
 shell_plugins_log_la_SOURCES = shell/plugins/log.c
 shell_plugins_log_la_CPPFLAGS = $(test_cppflags)
 shell_plugins_log_la_LDFLAGS = -module -rpath /nowhere

t/shell/plugins/setopt.c

@@ -0,0 +1,73 @@
+/************************************************************\
+ * Copyright 2020 Lawrence Livermore National Security, LLC
+ * (c.f. AUTHORS, NOTICE.LLNS, COPYING)
+ *
+ * This file is part of the Flux resource manager framework.
+ * For details, see https://github.com/flux-framework.
+ *
+ * SPDX-License-Identifier: LGPL-3.0
+\************************************************************/
+
+#if HAVE_CONFIG_H
+#include "config.h"
+#endif
+#include <stdio.h>
+#include <stdarg.h>
+#include <string.h>
+#include <errno.h>
+
+#include <jansson.h>
+#include <flux/core.h>
+#include <flux/shell.h>
+
+#include "src/common/libtap/tap.h"
+
+static int die (const char *fmt, ...)
+{
+    va_list ap;
+    va_start (ap, fmt);
+    vfprintf (stderr, fmt, ap);
+    va_end (ap);
+    return -1;
+}
+
+static int check_setopt (flux_plugin_t *p,
+                         const char *topic,
+                         flux_plugin_arg_t *args,
+                         void *data)
+{
+    json_t *options = NULL;
+
+    flux_shell_t *shell = flux_plugin_get_shell (p);
+    if (!p)
+        return die ("flux_plugin_get_shell\n");
+
+    ok (flux_shell_info_unpack (shell,
+                                "{s:{s:{s:{s:{s:o}}}}}",
+                                "jobspec",
+                                "attributes",
+                                "system",
+                                "shell",
+                                "options", &options) < 0 && errno == ENOENT,
+        "flux_shell_info_unpack shell options returns ENOENT");
+
+    /*  A shell plugin should be able to call setopt even though
+     *   no shell options were currently set in jobspec.
+     */
+    ok (flux_shell_setopt (shell, "new", "42") == 0,
+        "flux_shell_setopt of new option works");
+
+    return exit_status () == 0 ? 0 : -1;
+}
+
+int flux_plugin_init (flux_plugin_t *p)
+{
+    plan (NO_PLAN);
+    ok (flux_plugin_add_handler (p, "shell.init", check_setopt, NULL) == 0,
+        "flux_plugin_add_handler works");
+    return 0;
+}
+
+/*
+ * vi: ts=4 sw=4 expandtab
+ */