Environment variables #289
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsibley
force-pushed
the
trs/env
branch
4 times, most recently
from
6c0b676 to
908a985
Compare
|
Looks like CI caught an issue on Python 3.8 and up in some new doctests that don't explicitly cleanup a temp dir (c.f.): This warning is turned into an error by our pytest config, and only happens because of some doctest code anyway, so it's a CI-specific failure. I'll address it nevertheless. Hilariously, the warning-turned-error can't be raised, presumably because the warning is emitted during global destruction, and this causes another warning, |
|
A couple other CI issues cropping up. Not sure why they're happening now, but I'll dig. Seem unrelated to this PR.
|
|
CI now passes! I addressed the Windows issue by checking for directories explicitly so it's compat across platforms, and the macOS bz2 issue was apparently indeed transient. |
|
One thing I want to consider (and think we should do) to help deprecate and remove the automatic hostenv forwarding is to disable it when these new |
|
I've done that now in fa2633d. |
|
CI failures appear unrelated: one of the Markdown image inlining tests is failing and started failing with yesterday's scheduled CI. I assume some new release of a dep broke something. |
This was referenced Jun 20, 2023
Merged
For compat on systems that don't have GNU coreutils, like macOS by default. Motivated by noticing in macOS CI jobs that devel/pytest was reporting "command not found: realpath" which results in `cd ""`, which results in no change of directory. But that's ok in the CI context so tests still worked. Anyhow, the usage in devel/pytest was copypasta (from me, two weeks ago!) that never warranted realpath.
…menting it The use of hostenv.forwarded_names was because that's what the Docker runner does and this Singularity runner was based on that. But the reasons Docker does so don't apply here to Singularity.
Most usages weren't annotated, and so weren't type checked. Type checking adds confidence in the correctness of future changes to how extra_env is handled. Such changes are something I'm about to do, and so using an alias makes it easier to make those changes.
Caught by the type checker now that extra_env is consistently annotated with a type.
This will be used to support the semantics of 0 byte files in envdirs when I add envdir support next.
These fix the lack of easy, supported ways to pass thru, set, and otherwise manage the variables present in the runtime environments. Previous workarounds for these missing features include a) hardcoding a list of often-crucial variables from the host that are always passed thru (nextstrain.cli.hostenv) and b) manually overriding --exec to `envdir` thru a provided directory. The new env support should let us eliminate or reduce the scope of those workarounds over time. For now the "hostenv" concept sticks around for compatibility reasons.
Passing an additional --env=HOST=0.0.0.0 arg to `docker run` is a hacky implementation encapsulation break. It only works to override the HOST=127.0.0.1 value provided via extra_env because the Docker runner translates extra_env to --env options that come before the additional args. The implementation of extra_env is about to change, however, and the hacky --env=HOST=0.0.0.0 arg here would no longer override it. All this would be simpler if the container shared the host's network interfaces (i.e. --network=host) instead of using separate interfaces and bridging to the host, but doing so assumes the container (i.e. dockerd) is running on the same host as we are. True most of the time, but not guaranteed. This is the more minimal change anyway.
…n/env.d… …when the image is new enough to support that. This ensures the env values aren't visible in the container's config (e.g. `docker inspect`, `aws batch describe-jobs`, the AWS web console). Temporary env.d directories (or ZIP archives of such) are written, given to the container, and then removed by the container's entrypoint after use. This minimizes the amount of time values exist outside of memory, persisted to storage (disk, S3). Other runners/runtimes don't have visibility or persistence issues because they directly exec the runtime process and thus can directly manipulate its environment without any intermediary.
The hostenv concept in the runner code predates the extra_env concept, and, unlike extra_env which is centralized thru the runner dispatcher, hostenv was handled independently by each runner.¹ By centralizing hostenv handling in the runner dispatcher and making it just another (automatic) part of extra_env, we simplify the runners, improve the security/visibility of the values in hostenv thru the recent improvements to extra_env handling, and make it easier to eventually deprecate/remove hostenv entirely. ¹ The reasoning at the time was that hostenv forwarding was only needed for containerized runners, and so the ambient (née native) runner didn't need to use it.
These new options are a good place to attach this related backwards incompatible change, since their usage acts as an opt-in. Eliminating automatic forwarding also makes it clearer what's getting passed when combined with explicit forwarding. We can (and should) still fully remove automatic hostenv in the future.
|
Rebased onto latest master after merge of #294. |
This was referenced Jun 21, 2023
victorlin
approved these changes
Jun 21, 2023
Comment on lines
+25
to
+29
| When either of these options are given, the default behaviour of | ||
| automatically passing thru several "well-known" environment variables is | ||
| disabled. That is, the following "well-known" environment variables are only | ||
| automatically passed thru when the new `--env` and `--envdir` options are | ||
| _not_ used: |
There was a problem hiding this comment.
One thing I want to consider (and think we should do) to help deprecate and remove the automatic hostenv forwarding is to disable it when these new
--envor--envdiroptions are used. Since they're new, their use is a good time/indicator to push folks to spell out what they need without hostenv. Thoughts?
Makes sense to me!
…-env Lists formatted as if they're for computers not humans are a pet peeve of mine, so address it here. This copy of prose_list() will also make it quick to improve other lists in this codebase in the future.
… Docker and AWS Batch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages.
Other context in my notes:
Testing