Skip to content

NodeUtils: export upcall variables to the environment #705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thiell wants to merge 1 commit into
base: master
Choose a base branch
from
Open

NodeUtils: export upcall variables to the environment #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions conf/groups.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ autodir: /etc/clustershell/groups.d $CFGDIR/groups.d
# doesn't exist, the "local.cfg" file from autodir will be used.
#
# See the documentation for $CFGDIR, $SOURCE, $GROUP and $NODE upcall special
# variables. Please remember that they are substituted before the shell command
# is effectively executed.
# variables. They are substituted before the shell command is executed and
# also exported as environment variables.
#
[local]
# flat file "group: nodeset" based group source using $CFGDIR/groups.d/local.cfg
# with backward support for /etc/clustershell/groups
map: [ -f $CFGDIR/groups ] && f=$CFGDIR/groups || f=$CFGDIR/groups.d/local.cfg; sed -n 's/^$GROUP:\(.*\)/\1/p' $f
all: [ -f $CFGDIR/groups ] && f=$CFGDIR/groups || f=$CFGDIR/groups.d/local.cfg; sed -n 's/^all:\(.*\)/\1/p' $f
list: [ -f $CFGDIR/groups ] && f=$CFGDIR/groups || f=$CFGDIR/groups.d/local.cfg; sed -n 's/^\([0-9A-Za-z_-]*\):.*/\1/p' $f
map: [ -f "$CFGDIR/groups" ] && f="$CFGDIR/groups" || f="$CFGDIR/groups.d/local.cfg"; sed -n "s/^$GROUP:\(.*\)/\1/p" "$f"
all: [ -f "$CFGDIR/groups" ] && f="$CFGDIR/groups" || f="$CFGDIR/groups.d/local.cfg"; sed -n 's/^all:\(.*\)/\1/p' "$f"
list: [ -f "$CFGDIR/groups" ] && f="$CFGDIR/groups" || f="$CFGDIR/groups.d/local.cfg"; sed -n 's/^\([0-9A-Za-z_-]*\):.*/\1/p' "$f"
2 changes: 1 addition & 1 deletion conf/groups.conf.d/genders.conf.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
# LLNL genders bindings
#
[genders]
map: nodeattr -n $GROUP
map: nodeattr -n "$GROUP"
all: nodeattr -n ALL
list: nodeattr -l

26 changes: 13 additions & 13 deletions conf/groups.conf.d/slurm.conf.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@
# SLURM partition bindings
#
[slurmpart,sp]
map: sinfo -h -o "%N" -p $GROUP
map: sinfo -h -o "%N" -p "$GROUP"
mapall: sinfo -h -o "%R:%N"
all: sinfo -h -o "%N"
list: sinfo -h -o "%R"
reverse: sinfo -h -N -o "%R" -n $NODE
reverse: sinfo -h -N -o "%R" -n "$NODE"

#
# SLURM node-reservation bindings
#
[slurmresv,sr]
map: scontrol -o show reservation $GROUP | grep -Po 'Nodes=\K[^ ]+'
map: scontrol -o show reservation "$GROUP" | grep -Po 'Nodes=\K[^ ]+'
mapall: scontrol -o show reservation | sed -n 's/^ReservationName=\([^ :]*\) .* Nodes=\([^ ]*\).*/\1:\2/p'
all: scontrol -o show reservation | grep -Po 'Nodes=\K[^ ]+'
list: scontrol -o show reservation | grep -Po 'ReservationName=\K[^ ]+'
Expand All @@ -27,49 +27,49 @@ cache_time: 60
# SLURM state bindings
#
[slurmstate,st]
map: sinfo -h -o "%N" -t $GROUP
map: sinfo -h -o "%N" -t "$GROUP"
mapall: sinfo -h -o "%T:%N" | sed 's/[*~#!%$@+^-]*:/:/'
all: sinfo -h -o "%N"
list: sinfo -h -o "%T" | tr -d '*~#!%$@+^-'
reverse: sinfo -h -N -o "%T" -n $NODE | tr -d '*~#!%$@+^-'
reverse: sinfo -h -N -o "%T" -n "$NODE" | tr -d '*~#!%$@+^-'
cache_time: 60

#
# SLURM job bindings
#
[slurmjob,sj]
map: squeue -h -j $GROUP -o "%N"
map: squeue -h -j "$GROUP" -o "%N"
mapall: squeue -h -o "%i:%N" -t R
list: squeue -h -o "%i" -t R
reverse: squeue -h -w $NODE -o "%i"
reverse: squeue -h -w "$NODE" -o "%i"
cache_time: 60

#
# SLURM user bindings for running jobs
#
[slurmuser,su]
map: squeue -h -u $GROUP -o "%N" -t R
map: squeue -h -u "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%u:%N" -t R
list: squeue -h -o "%u" -t R
reverse: squeue -h -w $NODE -o "%u"
reverse: squeue -h -w "$NODE" -o "%u"
cache_time: 60

#
# SLURM account bindings for running jobs
#
[slurmaccount,sa]
map: squeue -h -A $GROUP -o "%N" -t R
map: squeue -h -A "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%a:%N" -t R
list: squeue -h -o "%a" -t R
reverse: squeue -h -w $NODE -o "%a" 2>/dev/null || true
reverse: squeue -h -w "$NODE" -o "%a" 2>/dev/null || true
cache_time: 60

#
# SLURM QOS bindings for running jobs
#
[slurmqos,sq]
map: squeue -h -q $GROUP -o "%N" -t R
map: squeue -h -q "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%q:%N" -t R
list: squeue -h -o "%q" -t R
reverse: squeue -h -w $NODE -o "%q" 2>/dev/null || true
reverse: squeue -h -w "$NODE" -o "%q" 2>/dev/null || true
cache_time: 60
15 changes: 12 additions & 3 deletions doc/man/man5/groups.conf.5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,15 @@ shell commands:
\fI$SOURCE\fP is replaced by current source name
.UNINDENT
.sp
These variables are also exported as environment variables to the executed
command, taking precedence over any variables of the same name from the
calling environment. Shell parameter expansions that are not replaced by the
library, like \fI${GROUP:\-default}\fP, are then expanded by the shell using the
same values. The context\-dependent \fI$GROUP\fP and \fI$NODE\fP variables are exported
with an empty value when they do not apply to the upcall. It is recommended
to write upcall commands in plain shell style, with these variables
double\-quoted (e.g. \fI\(dq$GROUP\(dq\fP).
.sp
Each external command might return a non\-zero return code when the operation
is not doable. But if the call returns zero, for instance, for a non\-existing
group, the user will not receive any error when trying to resolve such an
Expand All @@ -196,14 +205,14 @@ confdir: /etc/clustershell/groups.conf.d $CFGDIR/groups.conf.d
autodir: /etc/clustershell/groups.d $CFGDIR/groups.d

[local]
map: sed \-n \(aqs/^$GROUP:\e(.*\e)/\e1/p\(aq /etc/clustershell/groups
map: sed \-n \(dqs/^$GROUP:\e(.*\e)/\e1/p\(dq /etc/clustershell/groups
list: sed \-n \(aqs/^\e([0\-9A\-Za\-z_\-]*\e):.*/\e1/p\(aq /etc/clustershell/groups

[slurm]
map: sinfo \-h \-o \(dq%N\(dq \-p $GROUP
map: sinfo \-h \-o \(dq%N\(dq \-p \(dq$GROUP\(dq
all: sinfo \-h \-o \(dq%N\(dq
list: sinfo \-h \-o \(dq%P\(dq
reverse: sinfo \-h \-N \-o \(dq%P\(dq \-n $NODE
reverse: sinfo \-h \-N \-o \(dq%P\(dq \-n \(dq$NODE\(dq
.fi
.sp
.SH FILES
Expand Down
55 changes: 32 additions & 23 deletions doc/sphinx/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,15 +246,15 @@ groups are bound to the source named *genders* by default::
autodir: /etc/clustershell/groups.d $CFGDIR/groups.d

[genders]
map: nodeattr -n $GROUP
map: nodeattr -n "$GROUP"
all: nodeattr -n ALL
list: nodeattr -l

[slurm]
map: sinfo -h -o "%N" -p $GROUP
map: sinfo -h -o "%N" -p "$GROUP"
all: sinfo -h -o "%N"
list: sinfo -h -o "%P"
reverse: sinfo -h -N -o "%P" -n $NODE
reverse: sinfo -h -N -o "%P" -n "$NODE"

The *groups.conf* files are parsed with Python's `ConfigParser`_. The first
section whose name is *Main* accepts the settings described in the following
Expand Down Expand Up @@ -504,6 +504,15 @@ replaced before executing shell commands:
* *$SOURCE* is replaced by the current source name (see a usage example just
below)

These variables are also exported as environment variables to the executed
command, taking precedence over any variables of the same name from the
calling environment. Shell parameter expansions that are not replaced by
the library, like ``${GROUP:-default}``, are then expanded by the shell
using the same values. The context-dependent ``$GROUP`` and ``$NODE``
variables are exported with an empty value when they do not apply to the
upcall. It is recommended to write upcall commands in plain shell style,
with these variables double-quoted (e.g. ``"$GROUP"``).

Upcall commands are executed with their standard input connected to
``/dev/null``, so they must not expect any input on stdin.

Expand Down Expand Up @@ -535,24 +544,24 @@ variable ``$SOURCE`` is always replaced by the source name before command
execution (here `cluster`, `racks` and `cpu`), for example::

[cluster,racks,cpu]
map: get_nodes_from_source.sh $SOURCE $GROUP
all: get_all_nodes_from_source.sh $SOURCE
list: list_nodes_from_source.sh $SOURCE
map: get_nodes_from_source.sh "$SOURCE" "$GROUP"
all: get_all_nodes_from_source.sh "$SOURCE"
list: list_nodes_from_source.sh "$SOURCE"

is equivalent to::

[cluster]
map: get_nodes_from_source.sh cluster $GROUP
map: get_nodes_from_source.sh cluster "$GROUP"
all: get_all_nodes_from_source.sh cluster
list: list_nodes_from_source.sh cluster

[racks]
map: get_nodes_from_source.sh racks $GROUP
map: get_nodes_from_source.sh racks "$GROUP"
all: get_all_nodes_from_source.sh racks
list: list_nodes_from_source.sh racks

[cpu]
map: get_nodes_from_source.sh cpu $GROUP
map: get_nodes_from_source.sh cpu "$GROUP"
all: get_all_nodes_from_source.sh cpu
list: list_nodes_from_source.sh cpu

Expand Down Expand Up @@ -586,11 +595,11 @@ partitions. Each group is named after the partition name and contains the
partition's nodes::

[slurmpart,sp]
map: sinfo -h -o "%N" -p $GROUP
map: sinfo -h -o "%N" -p "$GROUP"
mapall: sinfo -h -o "%R:%N"
all: sinfo -h -o "%N"
list: sinfo -h -o "%R"
reverse: sinfo -h -N -o "%R" -n $NODE
reverse: sinfo -h -N -o "%R" -n "$NODE"

.. highlight:: console

Expand All @@ -608,7 +617,7 @@ reservations. Each group is based on a different reservation and contains
the nodes currently in that reservation::

[slurmresv,sr]
map: scontrol -o show reservation $GROUP | grep -Po 'Nodes=\K[^ ]+'
map: scontrol -o show reservation "$GROUP" | grep -Po 'Nodes=\K[^ ]+'
mapall: scontrol -o show reservation | sed -n 's/^ReservationName=\([^ :]*\) .* Nodes=\([^ ]*\).*/\1:\2/p'
all: scontrol -o show reservation | grep -Po 'Nodes=\K[^ ]+'
list: scontrol -o show reservation | grep -Po 'ReservationName=\K[^ ]+'
Expand All @@ -630,11 +639,11 @@ node states. Each group is based on a different state name and contains the
nodes currently in that state::

[slurmstate,st]
map: sinfo -h -o "%N" -t $GROUP
map: sinfo -h -o "%N" -t "$GROUP"
mapall: sinfo -h -o "%T:%N" | sed 's/[*~#!%$@+^-]*:/:/'
all: sinfo -h -o "%N"
list: sinfo -h -o "%T" | tr -d '*~#$@+'
reverse: sinfo -h -N -o "%T" -n $NODE | tr -d '*~#$@+'
reverse: sinfo -h -N -o "%T" -n "$NODE" | tr -d '*~#$@+'
cache_time: 60

Here, :ref:`cache_time <group-external-caching>` is set to 60 seconds instead
Expand All @@ -657,21 +666,21 @@ Each group is based on a running job ID and contains the nodes currently
allocated for this job::

[slurmjob,sj]
map: squeue -h -j $GROUP -o "%N"
map: squeue -h -j "$GROUP" -o "%N"
mapall: squeue -h -o "%i:%N" -t R
list: squeue -h -o "%i" -t R
reverse: squeue -h -w $NODE -o "%i"
reverse: squeue -h -w "$NODE" -o "%i"
cache_time: 60

The next section **slurmuser,su** defines a group source based on Slurm users.
Each group is based on a username and contains the nodes currently
allocated for jobs belonging to the username::

[slurmuser,su]
map: squeue -h -u $GROUP -o "%N" -t R
map: squeue -h -u "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%u:%N" -t R
list: squeue -h -o "%u" -t R
reverse: squeue -h -w $NODE -o "%i"
reverse: squeue -h -w "$NODE" -o "%u"
cache_time: 60

.. highlight:: console
Expand All @@ -694,10 +703,10 @@ accounts. Each group is based on an account and contains the nodes where there
are running jobs under this account::

[slurmaccount,sa]
map: squeue -h -A $GROUP -o "%N" -t R
map: squeue -h -A "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%a:%N" -t R
list: squeue -h -o "%a" -t R
reverse: squeue -h -w $NODE -o "%a" 2>/dev/null || true
reverse: squeue -h -w "$NODE" -o "%a" 2>/dev/null || true
cache_time: 60

.. highlight:: console
Expand All @@ -714,10 +723,10 @@ Each group is based on a qos and contains the nodes where there are running
jobs under this qos::

[slurmqos,sq]
map: squeue -h -q $GROUP -o "%N" -t R
map: squeue -h -q "$GROUP" -o "%N" -t R
mapall: squeue -h -o "%q:%N" -t R
list: squeue -h -o "%q" -t R
reverse: squeue -h -w $NODE -o "%q" 2>/dev/null || true
reverse: squeue -h -w "$NODE" -o "%q" 2>/dev/null || true
cache_time: 60

.. highlight:: console
Expand Down Expand Up @@ -753,7 +762,7 @@ The section **xcat** defines a group source based on xCAT static node groups::
[xcat]

# list the nodes in the specified node group
map: lsdef -s -t node $GROUP | cut -d' ' -f1
map: lsdef -s -t node "$GROUP" | cut -d' ' -f1

# list all the nodes defined in the xCAT tables
all: lsdef -s -t node | cut -d' ' -f1
Expand Down
15 changes: 12 additions & 3 deletions doc/txt/groups.conf.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ shell commands:
* *$CFGDIR* is replaced by groups.conf highest priority base directory path
* *$SOURCE* is replaced by current source name

These variables are also exported as environment variables to the executed
command, taking precedence over any variables of the same name from the
calling environment. Shell parameter expansions that are not replaced by the
library, like *${GROUP:-default}*, are then expanded by the shell using the
same values. The context-dependent *$GROUP* and *$NODE* variables are exported
with an empty value when they do not apply to the upcall. It is recommended
to write upcall commands in plain shell style, with these variables
double-quoted (e.g. *"$GROUP"*).

Each external command might return a non-zero return code when the operation
is not doable. But if the call returns zero, for instance, for a non-existing
group, the user will not receive any error when trying to resolve such an
Expand Down Expand Up @@ -168,14 +177,14 @@ Simple configuration file for local groups and Slurm partitions binding.
| autodir: /etc/clustershell/groups.d $CFGDIR/groups.d
|
| [local]
| map: sed -n 's/^$GROUP:\\(.*\\)/\\1/p' /etc/clustershell/groups
| map: sed -n "s/^$GROUP:\\(.*\\)/\\1/p" /etc/clustershell/groups

@degremont degremont Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of just changing this single quotes to double quotes actually :) not adding double quotes everywhere. That may be zealous.

| list: sed -n 's/^\\([0-9A-Za-z\_-]\*\\):.*/\\1/p' /etc/clustershell/groups
|
| [slurm]
| map: sinfo -h -o "%N" -p $GROUP
| map: sinfo -h -o "%N" -p "$GROUP"
| all: sinfo -h -o "%N"
| list: sinfo -h -o "%P"
| reverse: sinfo -h -N -o "%P" -n $NODE
| reverse: sinfo -h -N -o "%P" -n "$NODE"



Expand Down
5 changes: 4 additions & 1 deletion lib/ClusterShell/NodeUtils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,11 @@ def _upcall_read(self, cmdtpl, args=dict()):
"""
cmdline = Template(self.upcalls[cmdtpl]).safe_substitute(args)

@degremont degremont Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we start supporting variable interpolation through real shell variables (through environment), it seems we do not need the template anymore? The shell variable supports below will handle everything, no?

@thiell thiell Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. The shell doesn't expand variables inside single quotes, and single-quoted $GROUP is used in the field (just look at man groups.conf, single quotes are used in the example). So the environment export complements the template (for shell-side expansions like ${GROUP:-default}) rather than replacing it.

@degremont degremont Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, a bit weird, as now the expansion will happen sometimes under single quote, and sometimes not. User may be surprised.

@thiell thiell Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template.safe_substitute() is pure text replacement and has always ignored shell quoting. $GROUP is always replaced if found like that in the command text. The only surprise might come from existing ${GROUP:-default} patterns not in single quote that were not resolved and that will then be resolved with this patch. It's actually the bug I am trying to fix here as I saw it in the field. :)

That said, I agree a single mechanism (shell-only) would probably be cleaner, but that breaks single-quoted $GROUP configs, including provided example configs and man pages. If we think it's a big deal (not sure), it's something we could consider to deprecate in a 2.0 maybe.

@degremont degremont Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so, what would be the less surprising to people over time would be for the variable to be 100% shell variables. So, let's keep the patch as-is, but, in the same time, we must change examples to use double quotes where appropriate. Let's write them as we were only supporting shell variables. We could deprecate that later.

@thiell thiell Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me and actually that's a very good point: it would be possible to have an empty variable with this I think, so it is highly recommended to double-quote variables in upcalls. Best practice anyway (SC2086). Thus, this will start a clean migration to shell-only variables in the future. I'll update this PR (docs, examples) with this.

self.logger.debug("EXEC '%s'", cmdline)
# also export variables so shell expansions like ${GROUP:-x} work
env = dict(os.environ, GROUP='', NODE='')
env.update((var, str(value)) for var, value in args.items())
proc = Popen(cmdline, stdin=DEVNULL, stdout=PIPE, shell=True,
cwd=self.cfgdir, universal_newlines=True)
cwd=self.cfgdir, universal_newlines=True, env=env)
output = proc.communicate()[0].strip()
self.logger.debug("READ '%s'", output)
if proc.returncode != 0:
Expand Down
32 changes: 32 additions & 0 deletions tests/NodeSetGroupTest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,38 @@ def testConfigCFGDIR(self):
self.assertEqual(str(NodeSet("@%s" % tmpgroup, resolver=res)),
"example[1-100]")

def testConfigUpcallEnv(self):
"""test upcall variables exported in the command environment"""
f = make_temp_file(dedent("""
[Main]
default: local

[local]
map: echo "n-${GROUP:-all}"
all: echo "a-${SOURCE:-none}${GROUP:-}"
list: basename "${CFGDIR:-/none}"
reverse: echo "g-${NODE:-none}"
""").encode('ascii'))
res = GroupResolverConfig(f.name)
# ${VAR:-default} is not replaced by the library: the shell expands
# it from the exported environment variables
self.assertEqual(str(NodeSet("@foo", resolver=res)), "n-foo")
# empty group name: the shell uses the default value
self.assertEqual(res.group_nodes(''), ["n-all"])
self.assertEqual(res.all_nodes(), ["a-local"])
tmpgroup = os.path.basename(os.path.dirname(f.name))
self.assertEqual(res.grouplist(), [tmpgroup])
self.assertEqual(res.node_groups("node1"), ["g-node1"])
# exported variables take precedence over the caller's environment,
# and are exported empty to upcalls they do not apply to
os.environ['GROUP'] = 'callerenv'
try:
res = GroupResolverConfig(f.name)
self.assertEqual(str(NodeSet("@bar", resolver=res)), "n-bar")
self.assertEqual(res.all_nodes(), ["a-local"])
finally:
del os.environ['GROUP']

def test_fromall_grouplist(self):
"""test NodeSet.fromall() without all upcall"""
# Group Source that has no all upcall and that can handle special char
Expand Down
Loading