Adds a new --bank/-B flag to Flux scheduling commands to simplify setting a bank

[ilumsden]

Currently, when Flux is running in system mode, users have to use --setattr=system.bank=<BANK_NAME> to select a bank. This PR adds a new --bank/-B flag to the following Flux commands to simplify this:

• flux alloc
• flux batch
• flux run
• flux submit
• flux bulksubmit

This new flag is essentially synonymous to Slurm's --account/-A flag.

There are still three outstanding things to do in this PR:

1. Test it
2. Decide what to do in situations where flux-accounting does not exist
3. Correct the commit message format since I wasn't really sure what the correct prefix is for the part of the code I'm editing

[garlick]

Thanks Ian!

On the commit form: the title should be <= 50 chars written in the imperative voice. The prefix is optional. A body is also required and we prefer the problem/solution form. For example here it could be something like:

add -B,--bank option to flux job submission commands

Problem: to specify the flux-accounting bank at job submission time,
users must remember --setattr=system.bank=NAME.

Add a -B,--bank=NAME to make it easier on users.

The fact that either the long form or the short form are ignored when flux-accounting is not present is maybe a different problem that doesn't necessarily have to be solved at the same time. It's really one of a class of problems that we have. For example, you can accidentally specify -ompi=pmi2 (like slurm wants) instead of -opmi=pmi2 (like flux wants) and flux will just ignore you.

For discussion, I guess my one thought about the proposed change is since these are equivalent:

-o KEY=VALUE
--setopt system.shell.options.KEY=VALUE

would adding a short option for setting system attributes be a reasonable compromise that keeps flux-core agnostic the bank concept?

-S bank=NAME
--setopt system.bank=NAME

If options are changed, we'll need a man page update also (in a separate commit preferably) and probably a couple tests just to get coverage for the option parsing.

[grondo]

I think what we'd planned here was to have a way for sites to add new options to the submission commands via plugins, then flux-accounting could provide a plugin to add --bank.

@trws suggested that all plugin-enabled options should have a specific prefix or other way to make it obvious that the option is provided by a plugin. This is a good idea, but might nullify some of the convenience of adding new options that are just shorthand for --setattr=KEY=VAL (or as Jim suggests, -S KEY=VAL.), and adding new short options (e.g. -B) would be right out.

Maybe if we could add a short option for --setattr as suggested by @garlick, and add a way to easily auto-document supported attributes in flux run/submit/etc. --help or similar, then this could go a long way to fixing this particular problem. E.g. a new section in the --help output that would list supported attributes with a way to extend this list by sub-projects, plugins or sites?

[wihobbs]

and add a way to easily auto-document supported attributes in flux run/submit/etc. --help or similar, then this could go a long way to fixing this particular problem. E.g. a new section in the --help output that would list supported attributes with a way to extend this list by sub-projects, plugins or sites?

The related issue to what Mark is describing is #2875.

I think a good first step is what @garlick describes, setting a short-option for --setattr as -S. Maybe the --help/man description could include examples of attributes of which bank is one? Since bank is a popular attribute, if people grep'ed for it in the manpage they would get an idea of how to set it, but as only an example it maintains flux-core's agnosticism.

[trws]

I agree in general that we want plugin-provided options, and documenting setattr targets would be fantastic, but at the same time banks are pretty deeply baked into most people's notion of what a resource manager does. Enough so that I'm honestly wondering if it's worth just always having the option, and having there be a logical bank, "default" or something, even when we're running without accounting. Most of the system need not care, but it would mean we could provide better failure modes if someone specifies the flag and there's nothing that can do anything about it. Having it do nothing silently is only a minor issue when there's no accounting plugin, you probably just don't care, but --setattr=system.banc=aalsdhf being silently ignored is substantially less ideal. Honestly I think that would be the biggest win is if we could make it so we can tell people when, under some conditions, they've set an attribute that can't possibly be right. Maybe just for attributes under system. or something but still.

[grondo]

@trws brings up some really great points. Currently we have maximum flexibility in that other framework projects, shell plugins, jobtap plugins, etc. can easily support new attributes in jobspec without any prior support in flux-core. However, the drawback is, as you say, there is currently no way to detect typos or malformed attributes until it is too late. Additionally, there is no auto-documentation of these attributes, instead the documentation, if any, is spread throughout the implementation(s). I'll open an issue on this.

For this specific PR/issue, I wonder if we should just officially support bank and project attributes in RFC 14. We already do support bank and project in job-list anyway:

flux-core/src/modules/job-list/job_data.c

Lines 191 to 199 in 14c64d2

	if (json_unpack_ex (job->jobspec, &error, 0,
	"{s:{s?{s?s s?s s:F s?s s?s}}}",
	"attributes",
	"system",
	"cwd", &job->cwd,
	"queue", &job->queue,
	"duration", &job->duration,
	"project", &job->project,
	"bank", &job->bank) < 0) {

So the ship on those two attributes has already sailed...

[grondo]

This commit was merged in #6195. Closing. Thanks @ilumsden!

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.36%. Comparing base (8a2d204) to head (7351856).
Report is 591 commits behind head on master.

Files with missing lines	Patch %	Lines
src/bindings/python/flux/cli/base.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6157      +/-   ##
==========================================
- Coverage   83.37%   83.36%   -0.01%     
==========================================
  Files         521      521              
  Lines       84653    84656       +3     
==========================================
  Hits        70577    70577              
- Misses      14076    14079       +3     

Files with missing lines	Coverage Δ
src/bindings/python/flux/cli/base.py	95.52% <66.66%> (-0.13%)	⬇️

... and 9 files with indirect coverage changes