Docs and tests for environment variable setting in containerized execution

[bernt-matthias]

Seems that this never worked (for singularity it might work if cleanenv is disabled).

The environment variables that are passed explicitly to the container are constructed in the two for loops starting here and consider

1. Environment variables set in the tool.
2. Environment variables set with <param id="docker_env_...">...(resp.singularity_env_...`), which also needs docs.

The fix is done by passing the complete JobDestination to the container. Before the container only had the information on the params of the job destination, i.e. env was not accessible. This has the additional advantage (IMO) of replacing a few of the ominous destination_info: Dict[str, Any] objects where it was hard to find out what this actually is.

A workaround is to set environment variables via <param docker_env_...>...</param> (resp. singularity_env_...) in the job configuration.

• The added test may fix Add <environment_variables> section to job_properties test tool #11348 No it does not.

TODO:

• fix :)

Should we backport this? I would say "yes" and suggest 23.0.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[bernt-matthias]

From the dev channel on matrix:

@natefoo

Though we wouldn't want to force all vars to be defined as container vars, since the admin might need to set vars outside the container to set up the env for running containers (although you could cheat and use execute env statements). But a containerize: true|false (default: true) option for name/value env entries might be a good solution.

@bernt-matthias

Hmm, I see. I understand the idea. But I don't like the state of the art (yet :)) .. which might be due to a lack of understanding.

The docs state that variables defined by env are available to the job. But since we never defined what a job is, it would be fine if the variables were available to the container as well as if the variables were available in the container (i.e. to the underlying tool).

Since for non-containerized jobs env is available to the tool one might just expect the same for containerized jobs. But then immediately we have a problem with exec env statements (which are hard/impossible to get into the container).

For singularity this should have been the case anyway until we introduced cleanenv (or was this the case from the beginning).

The biggest advantage of env being available to the underlying tool might be that we can use it in TPV, like its already done in any case, i.e. for conda, docker, and singularity destinations. With the current state, I guess this only works for destinations using conda and we would need to set different params/env to cover the three, or?

@natefoo

The default when not using containers would be that vars are available both to the pre-and-post-tool-execution job environment and the tool itself. Right now, when you enable containers it means they're only available to the job environment. My suggestion to have the containerize option default to true would be to flip that so they're only available to the tool environment. But alternatively we could just make them available to both by default (with maybe the option to control which they are available to, but that option already does not exist for non-containerized tools, so maybe it is not necessary).

[mvdbeek]

A workaround is to set environment variables via <param docker_env_...>...</param> (resp. singularity_env_...) in the job configuration.

Isn't this a better option ? I worry about changing the defaults and leaking secrets into containers.

we can use it in TPV

IMO there needs to be a layer of indirection anyway when translating these (and other) values from the shared database, I don't think that's necessarily a reason to expose all env tags to the container.

[bernt-matthias]

Thanks for the feedback.

Isn't this a better option ?

At least a working option. And I share your concerns.

Let's ask the TPV developers if there is a way...

We should work on the docs for env and document how to set environment variables for containers.

[nuwang]

I had not realised, until @bernt-matthias pointed it out, that containers would not receive env vars available to non-containerized jobs. e.g.: https://github.com/galaxyproject/tpv-shared-database/blob/fd48e2d8970e672bd9cb56cbf5965af45c45f2b9/tools.yml#L2390.

IMO, part of the advantage of having the shared-database is to make it easier to bridge the divide between containerised and non-containerised tools. That way, if the settings are tweaked for a non-containerised tool, they would be tweaked for a containerised tool as well. This is specifically a problem for environments like AnVIL, because most of the tweaking happens on non-AnVIL/slurm based environments, and we do not have enough bandwidth to tweak it again for containerized environments.

I see the problem with security, and it looks like the essence of the problem here is that there are two types of envs - system and tool (more if we consider pre-job, post-job and tool - but that might be overkill). Perhaps the solution then is to explicitly acknowledge that divide? In TPV, we could have:

env:
   system:
       CUDA_VISIBLE_DEVICES: 0
   tool:
      _JAVA_OPTIONS: -Xmx{int(mem)}G -Xms1G

IMO there needs to be a layer of indirection anyway when translating these (and other) values from the shared database

@mvdbeek Can you elaborate on this?

[mvdbeek]

Sure, you want to be able to modify and filter environment variables in TPV. Take secrets for instance, those should not be passed to containers by default.

[bernt-matthias]

Take secrets for instance, those should not be passed to containers by default.

There is one thing that I do not get yet: For non-containerized destinations, there is currently no way to make this distinction. So if there are some variables for which we do not want the tool to see them, then there should be a way to do this for all kinds of destinations.

Currently, there is only a way to define variables that should only be available in containers (via docker_env_... and singularity_env). In light of the current discussion, it is as if these are just implemented the wrong way around.

[mvdbeek]

There is one thing that I do not get yet: For non-containerized destinations, there is currently no way to make this distinction. So if there are some variables for which we do not want the tool to see them, then there should be a way to do this for all kinds of destinations.

I think you're taking the wrong angle here. Right now it is possible to set variables that are not seen by containers. We cannot change this for anyone relying on this. It doesn't matter that there is no distinction for non-containerized tools.

Unless we collectively and consciously decide that this doesn't pose a risk ...

[bernt-matthias]

OK. Then along with the idea of @nuwang

env:
   system:
       SYSVAR: VAL
   tool:
      TOOLVAR: VAL

So on a conda destination, we would ignore SYSVAR (but the example could be better since CUDA variables should go to the tool) and set

<env id="TOOLVAR">VAL</env> 

And on a containerized destination, we would

<env id="SYSVAR">VAL</env> 
<param id="docker_env_TOOLVAR">VAL</env> 

?

For TPV 's shared database probably only tool variables are of interest since system variables likely depend on the respective Galaxy installations.

[mvdbeek]

That looks good, but I'd maybe call it global and job ?

[bernt-matthias]

OK. Cool. Then my suggestion for this PR would be:

• to revert the changes to the code
• Keep the test and extend it for the docker_env_ and singularity_env_ ...

[mvdbeek]

I guess we have to document this, but I don't think this is a good state of affairs. I think env, file and execute should be available in the containers. It's just the random stuff that might be floating around like PYTHONPATH and secrets that shouldn't automatically bleed into the container.

[mvdbeek]

Can you remove the JOBCONF_ENV_VAR tests ? We should fix this, not cement it with tests.

[bernt-matthias]

Can we leave this: It just tests the documented behavior (as per this PR :) ) and you gave good reasons for not changing it.

If at one time we allow certain env variables to be passed to the container, e.g. via ToolInfo then we can easily add another variable for this behavior.

[bernt-matthias]

I guess we have to document this, but I don't think this is a good state of affairs.

Then we should not merge it :)

I think env, file and execute should be available in the containers. It's just the random stuff that might be floating around like PYTHONPATH and secrets that shouldn't automatically bleed into the container.

Like what I have implemented in the commits that I reverted later on?

I guess we should adapt the docs and the tests to what it should be and then fix the implementation.

[mvdbeek]

Then we're back to passing all secrets an admin might have set to the container. I think a reasonable middle ground is to implement https://github.com/galaxyproject/galaxy/blob/release_24.0/lib/galaxy/tool_util/deps/dependencies.py#L52-L54 ? Those can be safely passed into the container.

[jmchilton]

Maybe if you mention this is a known bug - it would help alleviate @mvdbeek's concerns below somewhat?