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
6 changes: 3 additions & 3 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
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
9 changes: 8 additions & 1 deletion 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,13 @@ 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.
.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,7 +203,7 @@ 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]
Expand Down
10 changes: 9 additions & 1 deletion doc/sphinx/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,14 @@ 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.

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 @@ -671,7 +679,7 @@ allocated for jobs belonging to the username::
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 Down
9 changes: 8 additions & 1 deletion 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,13 @@ 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.

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,7 +175,7 @@ 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]
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