flux-mini: add environment manipulation options #3150
Conversation
|
I like that a lot. Even if the technical rule is that one can put those in there, almost everyone treats them as though you cant, and as you say it's possible to work around anyway. The ability to do appends or prepends like that makes me really happy, it handles the corner case of having other separators without breaking a sweat even for programs written for windows, and those that should have known better like lua. One question I couldn't parse out in a quick glance. Is there a way to express a negative rather than positive pattern? Like, I want to remove all variables matching |
Sorry for not being clear @trws. The current functionality So this would work, without affecting previous functionality: $ flux mini run --dry-run --env-filter=* --env='FLUX_*_PATH' hostname | jq .attributes.system.environment
{
"FLUX_CONNECTOR_PATH": "/home/grondo/git/flux-core.git/src/connectors",
"FLUX_MODULE_PATH": "/home/grondo/git/flux-core.git/src/modules",
"FLUX_EXEC_PATH": "/home/grondo/git/flux-core.git/src/broker:/home/grondo/git/flux-core.git/src/cmd",
"FLUX_PMI_LIBRARY_PATH": "/home/grondo/git/flux-core.git/src/common/.libs/libpmi.so"
}
|
|
Ah, ok, sorry I had it completely backwards (this is what happens when I try to do too many things at once 🤦 ). So we have negative patterns, but not positive ones. Is that true for the regular expression form as well? I need to think about this a bit, but "filter" in most places I know of usually takes a pattern or lambda or whatever that when true keeps the item, so it might be worth considering another name, or having both positive and negative filters like you mentioned originally? The C++ std lib is the one place that does the negative pattern thing, they call it |
|
Sigh, and I didn’t read this even as well as I should, I like that
idea of how to do the positive pattern, would we then be able to have
the regex version be `—env=/<exp>/` for positive rather than
`—env=-/` for the negative?
…On 20 Aug 2020, at 11:19, Mark Grondona wrote:
> One question I couldn't parse out in a quick glance. Is there a way
to express a negative rather than positive pattern? Like, I want to
remove all variables matching pattern?
Sorry for not being clear @trws.
The current functionality `--env-filter=PATTERN` or `--env=-PATTERN`
removes all variables matching a pattern. However, it occurs to me we
could treat the normal `VAR` usage, e.g. `--env=VAR` with no "=" as a
pattern rule instead of just an exact match.
So this would work, without affecting previous functionality:
```console
$ flux mini run --dry-run --env-filter=* --env='FLUX_*_PATH' hostname
| jq .attributes.system.environment
{
"FLUX_CONNECTOR_PATH":
"/home/grondo/git/flux-core.git/src/connectors",
"FLUX_MODULE_PATH": "/home/grondo/git/flux-core.git/src/modules",
"FLUX_EXEC_PATH":
"/home/grondo/git/flux-core.git/src/broker:/home/grondo/git/flux-core.git/src/cmd",
"FLUX_PMI_LIBRARY_PATH":
"/home/grondo/git/flux-core.git/src/common/.libs/libpmi.so"
}
```
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#3150 (comment)
|
|
Thanks! I was having the same thought about the name! I'm fine with Note that in my current proposal A subtlety though, |
That's an interesting point. It kidna gets back to my earlier thought/question, if a user did I think the |
Yes, though I was thinking of keeping both, unless you don't think we need a negative match? |
Or, rather, I guess it is more technically "remove all matching entries" (I'm getting confused on my terminology) |
|
Yeah I was thinking have both, shouldn’t have said it as “rather
than” more meant “as opposed to” or “in addition to” or
something.
…On 20 Aug 2020, at 11:37, Mark Grondona wrote:
> would we then be able to have
the regex version be `—env=/<exp>/` for positive rather than
`—env=-/` for the negative?
Yes, though I was thinking of keeping both, unless you don't think we
need a negative match?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#3150 (comment)
|
Ok. Let me push that small change here so people could play with it if they want. I'll also change |
In the current implementation it would be the latter. Both the "positive match" and the template expansion of
If I understand you correctly, this is pretty much what is done now. I think it successfully follows the principle of least surprise, except where it might not be obvious that we always start with a full environment. (so |
|
I think this is fine, doing it that way is pretty expressive, and for me
it’s what I’m used to since the env command uses `-i ONLY_VAR_1=meh`
to clear and set. I suppose we could have a shortcut for “clear
all” like that, but I’m not sure it’s really necessary.
…On 20 Aug 2020, at 12:29, Mark Grondona wrote:
> That's an interesting point. It kidna gets back to my earlier
thought/question, if a user did --env-remove=* --env=OMP_* would that
remove everything and start with no environment, or remove everything
then whitelist the ones starting with OMP_?
In the current implementation it would be the latter. Both the
"positive match" and the template expansion of `$VAR` use a
`ChainMap(env, os.environ)`, so lookups are from the built environment
first, then fall back to the process environment. The "rules" are
stored in a list and applied in the order they are found on the
cmdline, with the first implied rule being `*` (so we start with a
copy of the current environment).
> The former is maybe easier to describe, but the latter is what things
> like .gitignore do, and might be quite useful. It would probably mean
> it would have to process as: "filter into or out of effective
> environment from original environment" rather than from current state
> of effective environment... Not sure.
If I understand you correctly, this is pretty much what is done now. I
think it successfully follows the principle of least surprise, except
where it might not be obvious that we always start with a full
environment. (so `--env=FOO` alone actually does nothing -- maybe we
need an `--env-only=PATTERN` option that is effectively the same as
`--env-remove=* --env=PATTERN`?)
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#3150 (comment)
|
|
Great, thanks for the valuable feedback @trws! |
|
Thanks @SteVwonder and @trws! I'll apply @SteVwonder's suggestions, then if the interface looks basically Ok, I'll expand tests and then amend |
|
Ok, I've force pushed another version here with some documentation, and the |
grondo
changed the title
|
I've pushed the changes to throw an exception when variable substitution fails as noted by @SteVwonder (thanks for catching that!!), added tests and an additional I've removed WIP for now as this is ready for a real review. |
Problem: flux-mini always copies the entire current environment into
jobspec, and there is no way to filter, modify, or amend it.
Add a new function get_filtered_environment() to flux-mini.py which
accepts and array of environment modification "rules" to apply to the
current process environment in order to form the final environment
copied into the jobspec.
The rules are
* If a rule begins with '-', then the rest of the rule is a pattern
which removes matching environment variables. If the pattern
starts with '/', it is a regex (optionally ending with '/'),
otherwise the pattern is considered a shell glob(7).
Example: "-*" filters all variables, creating an empty environment.
* If a rule begins with '^' then the rest of the rule is a filename
from which to read more rules, one per line. The ~ character is
expanded in filename to be users directory.
Example: "^~/envfile" reads from file $HOME/envfile.
* If a rule is of the form `VAR=VAL`, then the variable VAR is set
to VAL. Before being set, however, variables in VAL are expanded
via string.Template().substitute, so any $VARIABLE within VAL will
be expanded using first the generated environment, falling back
to os.environ.
Example: "PATH=/bin", "PATH=$PATH:/bin"
* Otherwise, the rule is considered a pattern from which to match
variables from os.environ if they do not alread exist in the
generated environment. E.g. "PATH" will export PATH from the
current environment (if it has not already been set in the
generated environment), and "OMP*" would copy all environment
variables that start with "OMP" and are not already set in the
generated environment.
Example: "PATH", "FLUX_*_PATH"
Rules are specified on the command line with a new `--env` option.
For convenience, `--env-remove=PATTERN` and `--env-file=FILE` are also
provided, which are equivalent to `--env=-PATTERN` and `--env=^FILE`,
respectively. All rules are applied in the order in which they are
specified.
The first implied rule is always `--env=*`, i.e. the generated
environment always starts as a copy of the current environment.
Since an env file is just a set of rules, it can be used in place of
multiple command line arguments, and can even load other env files:
-FLUX_URI
-LS_COLORS
-OMPI_*
PATH=$PATH:/some/other/path
^~/.flux/some-other-env-file
grondo
force-pushed
the
issue#3141
branch
3 times, most recently
from
5c25354 to
988190d
Compare
Add a set of basic functionality tests for the flux-mini(1) --env, --env-remove, and --env-file options.
|
Thanks! |
Add an ENVIRONMENT section to flux-mini(1) documenting the --env, --env-remove, and --env-file options along with how the environment is copied into the jobspec before job submission.
Codecov Report
@@ Coverage Diff @@
## master #3150 +/- ##
==========================================
- Coverage 81.18% 81.14% -0.04%
==========================================
Files 286 285 -1
Lines 44538 44514 -24
==========================================
- Hits 36159 36122 -37
- Misses 8379 8392 +13
|
Ok, I'm parking this experimental PR here before moving on to other things.
This PR adds
--env=RULE,--env-filter=PATTERN, and--env-file=FILEoptions toflux-minias discussed in #3141.The PR is marked WIP for now because there were some concerns about the interface. Namely this interface doesn't allow exporting environment variables that begin with
-or^explictly to jobs, but you can't do that with the shell anyway (at least most shells). As @trws demonstrated a user could runflux-miniunder/usr/bin/envif an environment variable not already set in the environment needs to start with a-or^.That being said, this interface was just an experiment so I'd be fine trying something else.
Note that env variable values are expanded before being set by
string.Templateusing aChainMapof the current jobspec environment andos.environ, so this works as you'd expect:Unfortunately, this requires that the variable expressions be quoted from the shell or else you only get the current environment expanded. Maybe that is too subtle for users?
It is nice when using an env file though, as it allows this to work: