add `--conf` option for registering instance config in `flux-batch` and `flux-alloc` #5232

grondo · 2023-06-06T23:10:21Z

This PR adds a --conf option to flux-alloc and flux-batch which allows users to iteratively build a JSON config file in the jobspec file archive (i.e. .attributes.system.files["conf.json"].data). If this option is used once, then -c{{tmpdir}}/conf.json is appended to the broker options so that the config becomes the child instance config. The --conf=ARG option takes several possible types of arguments:

If ARG contains a newline, then it is assume to be a multiline configuration and is parsed as JSON or TOML. This allows config to be embedded directly in a batch directive
O/w, if ARG contains a =, the argument is assumed to be key=value where key can be dotted to set hierarchical values, and value is parsed as JSON -- e.g. --conf=resource.noverify=true
O/w, if ARG ends in .json or .toml then the argument is assumed to be a path to a file in JSON or TOML.
O/w, an error is raised, but eventually the argument may refer to a "named config" file registered in the system or user Flux config hierarchy. (This is planned as future work).

The --conf option may be used multiple times to iteratively build a config on the command line.

I'm actually tempted to add the --conf option to all submission commands, e.g. it could be useful with flux run OPTIONS.. flux start -o,-c{{tmpdir}}/conf.json.

I also wonder if might be useful to add an argument that initializes a job's config from the enclosing instance config.

src/bindings/python/flux/cli/base.py

chu11 · 2023-06-07T00:36:07Z

doc/man1/common/submit-other-options.rst

+      # flux: --conf="""
+      # flux: [resource]
+      # flux: noverify = true
+      # flux: """


i just noticed that noverify is not documented. Perhaps should pick something else for the user facing docs?

Oh, yeah, I probably meant to use norestrict

Went with resource.exclude which might actually be useful. 🤷

grondo · 2023-06-07T00:40:10Z

I'm addressing some of the issues caught by CodeQL and will do a force-push shortly...

Problem: There is no way to easily create a subinstance jobspec with an embedded alternate config file via the from_nest_command() or from_batch_command() constructors. Add a `conf` parameter to these two Jobspec class factory methods. If present, this argument will place a `conf.json` entry in the RFC 37 `files` dictionary of the resulting jobspec, and will append -c {{tmpdir}}/conf.json To the broker command line.

Problem: There are no options to specify a config file for subinstances created by `flux-batch(1)` and `flux-alloc(1)`, but it would be extremely useful to be able to do so. Add a `--conf=CONF` option to these commands. The option supports several types of arguments, and may be specified multiple times to update the built configuration: - If `CONF` is a multiline string, then parse `CONF` as JSON or TOML and update config. - If `CONF` contains an `=`, then parse `CONF` as config `KEY`=`VAL` and update config `KEY` with `VAL`, where `VAL` is parsed as JSON. - If `CONF` ends in `.json` or `.toml`, then treat `CONF` as a path to a config file to load as JSON or TOML. - Future extension: Otherwise, load a "named configuration" file from a local repository of config files. The config is built iteratively using a new BatchConfig class in cli/base.py

grondo · 2023-06-07T16:34:23Z

It was actually straightforward to add the "named" config support, so I've tentatively done that here. (Still need to document it, but thought I'd see what reviewers thought first).

The named config support allows a set of <name>.toml or name.<json> files to be put in the XDG specfied path for flux, e.g. /etc/xdg/flux/config/<name>.toml or ~/.config/flux/config/<name>.toml, and then refer to these files with --conf=name.

The intent is that cluster admins could put, for example, a node-exclusive config in /etc/xdg/flux/config/node-exclusive.toml and users can run with it via flux batch --conf=node-exclusive ....

grondo · 2023-06-07T16:39:22Z

Hm, github isn't displaying the new commits perhaps there is an issue b/c I swear I've pushed the latest...

garlick

I've played with this a bit manually and I'm not spotting anything in a (admittedly cursory) review of the code, so tentatively approving! Perhaps @chu11 could give the python a look before merging.

grondo · 2023-06-07T20:12:03Z

Just pushed updated documentation for the --conf=NAME usage. I could optionally squash the incremental development of the --conf option if that is desired.

chu11

just noticed a few small things

chu11 · 2023-06-07T20:36:42Z

t/t2716-python-cli-batch-conf.t

+test_expect_success 'flux-batch --conf=FILE detects invalid syntax' '
+	cat <<-EOF >conf-bad.toml &&
+	[resource]
+	noverify = foo
+	EOF
+	test_must_fail flux batch --conf=conf-bad.toml -n1 --dry-run \
+		--wrap hostname >parse-error.out 2>&1 &&
+	test_debug "cat parse-error.out" &&
+	grep "parse error" parse-error.out
+'


should test invalid json syntax too?

chu11 · 2023-06-07T20:51:17Z

src/bindings/python/flux/cli/base.py

+            # Take the first matching filename preferring TOML:
+            for ext in (".toml", ".json"):
+                filename = f"{path}/{name}{ext}"
+                print(f"checking path {filename}", file=sys.stderr)


just want to make sure if this is not left over debugging? sort of feels like it could be.

Yes, thanks for catching that!

chu11 · 2023-06-07T20:54:58Z

src/bindings/python/flux/cli/base.py

@@ -388,6 +388,8 @@ class BatchConfig:
    Iteratively build a "config" dict from successive updates.
    """


commit message "useful to users under /etc/xdg/flux/config/*.toml" should be {*.toml, *.json}?

Problem: There are no tests that excercise the flux-batch/alloc --conf option. Add a new sharness test, t2716-python-cli-batch-conf.t, to test operation of this option and its various supported arguments.

Problem: The --conf option is undocumented. Add documentation for this option to man1/common/submit-other-options.rst.

Problem: Creation of a search path based on the XDG standard may be useful in multiple places, but the code to do this is currently embedded in the UtilConfig class. Move the construction of the XDG search path to its own function for reusability. Add an optional `subdir` parameter to the function so that callers which need to build a set of paths in a subdir of the XDG search path need not do this manually.

Problem: There is currently no way to register a set of named config files for use with the --conf=NAME job submission option. Add support for named configuration files which are loaded from a searchpath following the XDG specification. This allows system administrators to register a set of standard configurations that would be most useful to users under /etc/xdg/flux/config/*.{toml,json}, and users can easily select one by name with the --conf option. Users can also place their own configs uner ~/.config/flux/config and use those as named configs as well.

Problem: The tests in t2716-python-cli-batch-conf.t do not test named config file support. Add a set of tests to exercise this functionality.

Problem: The flux-jobs(1) CONFIGURATION section specifies that /etc/flux/xdg is the default search path when XDG_CONFIG_DIRS is not set, but it should be /etc/xdg/flux. Fix the incorrect path.

Problem: The use of "named" configuration with the submission command --conf option is not documented. Document the operation and use of the --conf=NAME option for flux submission commands.

grondo · 2023-06-07T22:34:31Z

Thanks @chu11! Made the suggested changes and forced a push.

codecov · 2023-06-07T22:35:50Z

Codecov Report

Merging #5232 (468b05e) into master (c624084) will decrease coverage by 0.02%.
The diff coverage is 93.25%.

❗ Current head 468b05e differs from pull request most recent head 1db7d1b. Consider uploading reports for the commit 1db7d1b to get more accurate results

@@            Coverage Diff             @@
##           master    #5232      +/-   ##
==========================================
- Coverage   83.79%   83.77%   -0.02%     
==========================================
  Files         448      448              
  Lines       75855    75940      +85     
==========================================
+ Hits        63559    63618      +59     
- Misses      12296    12322      +26

Impacted Files	Coverage Δ
src/bindings/python/flux/cli/alloc.py	`88.37% <ø> (ø)`
src/bindings/python/flux/cli/batch.py	`98.48% <ø> (ø)`
src/bindings/python/flux/cli/base.py	`97.55% <92.40%> (-0.59%)`	⬇️
src/bindings/python/flux/job/Jobspec.py	`90.02% <100.00%> (+0.08%)`	⬆️
src/bindings/python/flux/util.py	`95.44% <100.00%> (+0.01%)`	⬆️

... and 11 files with indirect coverage changes

chu11

LGTM!

grondo · 2023-06-07T23:14:08Z

Thanks @chu11 and @garlick! Setting MWP.

github-advanced-security bot found potential problems Jun 6, 2023

View reviewed changes

src/bindings/python/flux/cli/base.py Fixed Show fixed Hide fixed

src/bindings/python/flux/cli/base.py Fixed Show fixed Hide fixed

src/bindings/python/flux/cli/base.py Fixed Show fixed Hide fixed

grondo force-pushed the batch-conf branch 2 times, most recently from ebc67f3 to 7df2401 Compare June 7, 2023 00:08

chu11 reviewed Jun 7, 2023

View reviewed changes

grondo force-pushed the batch-conf branch 2 times, most recently from c2ef085 to 3773025 Compare June 7, 2023 02:30

grondo added 2 commits June 7, 2023 06:24

grondo force-pushed the batch-conf branch from 3773025 to f669768 Compare June 7, 2023 13:30

grondo force-pushed the batch-conf branch from d9fb9e8 to 8c25eaf Compare June 7, 2023 17:52

grondo added this to the flux-core v0.51.0 milestone Jun 7, 2023

garlick approved these changes Jun 7, 2023

View reviewed changes

grondo force-pushed the batch-conf branch from a0174cb to 468b05e Compare June 7, 2023 20:18

chu11 reviewed Jun 7, 2023

View reviewed changes

grondo added 7 commits June 7, 2023 15:31

testsuite: add tests for flux-batch --conf option

23accaa

Problem: There are no tests that excercise the flux-batch/alloc --conf option. Add a new sharness test, t2716-python-cli-batch-conf.t, to test operation of this option and its various supported arguments.

doc: document flux-batch(1) and flux-alloc(1) --conf option

671a8c2

Problem: The --conf option is undocumented. Add documentation for this option to man1/common/submit-other-options.rst.

testsuite: test named config support for --conf option

8840165

Problem: The tests in t2716-python-cli-batch-conf.t do not test named config file support. Add a set of tests to exercise this functionality.

doc: fix incorrect XDG path in flux-jobs(1)

b82a9d4

Problem: The flux-jobs(1) CONFIGURATION section specifies that /etc/flux/xdg is the default search path when XDG_CONFIG_DIRS is not set, but it should be /etc/xdg/flux. Fix the incorrect path.

doc: document named configs in --conf submission option

1db7d1b

Problem: The use of "named" configuration with the submission command --conf option is not documented. Document the operation and use of the --conf=NAME option for flux submission commands.

grondo force-pushed the batch-conf branch from 468b05e to 1db7d1b Compare June 7, 2023 22:33

chu11 approved these changes Jun 7, 2023

View reviewed changes

grondo added the merge-when-passing label Jun 7, 2023

mergify bot merged commit 54865cf into flux-framework:master Jun 7, 2023
30 of 32 checks passed

grondo deleted the batch-conf branch June 7, 2023 23:18

grondo mentioned this pull request Jan 9, 2024

capture instance config in jobspec for batch and alloc jobs #3679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `--conf` option for registering instance config in `flux-batch` and `flux-alloc` #5232

add `--conf` option for registering instance config in `flux-batch` and `flux-alloc` #5232

grondo commented Jun 6, 2023 •

edited

chu11 Jun 7, 2023

grondo Jun 7, 2023

grondo Jun 7, 2023

grondo commented Jun 7, 2023

grondo commented Jun 7, 2023

grondo commented Jun 7, 2023

garlick left a comment

grondo commented Jun 7, 2023

chu11 left a comment

chu11 Jun 7, 2023

chu11 Jun 7, 2023 •

edited

grondo Jun 7, 2023

chu11 Jun 7, 2023

grondo commented Jun 7, 2023

codecov bot commented Jun 7, 2023

chu11 left a comment

grondo commented Jun 7, 2023

		@@ -388,6 +388,8 @@ class BatchConfig:
		Iteratively build a "config" dict from successive updates.
		"""

add --conf option for registering instance config in flux-batch and flux-alloc #5232

add --conf option for registering instance config in flux-batch and flux-alloc #5232

Conversation

grondo commented Jun 6, 2023 • edited

chu11 Jun 7, 2023

Choose a reason for hiding this comment

grondo Jun 7, 2023

Choose a reason for hiding this comment

grondo Jun 7, 2023

Choose a reason for hiding this comment

grondo commented Jun 7, 2023

grondo commented Jun 7, 2023

grondo commented Jun 7, 2023

garlick left a comment

Choose a reason for hiding this comment

grondo commented Jun 7, 2023

chu11 left a comment

Choose a reason for hiding this comment

chu11 Jun 7, 2023

Choose a reason for hiding this comment

chu11 Jun 7, 2023 • edited

Choose a reason for hiding this comment

grondo Jun 7, 2023

Choose a reason for hiding this comment

chu11 Jun 7, 2023

Choose a reason for hiding this comment

grondo commented Jun 7, 2023

codecov bot commented Jun 7, 2023

Codecov Report

chu11 left a comment

Choose a reason for hiding this comment

grondo commented Jun 7, 2023

add `--conf` option for registering instance config in `flux-batch` and `flux-alloc` #5232

add `--conf` option for registering instance config in `flux-batch` and `flux-alloc` #5232

grondo commented Jun 6, 2023 •

edited

chu11 Jun 7, 2023 •

edited