fix SINGULARITYENV_PATH

[GodloveD]

Description of the Pull Request (PR):

In the days of yore, a user could specify a value of SINGULARITYENV_PATH and this would be prepended to the PATH inside their container at runtime. Some time ago, this option was broken by the addition of a file in /.singularity.d/env called 10-docker.sh which overwrites the base path set in c with whatever path is set in Docker. This is necessary because the user may have added custom paths to the container in the Docker file. But it breaks SINGULARITYENV_PATH.

This is a tough problem. The 10-docker.sh file should overwrite the PATH and afterward, any value of SINGULARITYENV_PATH should be prepended to the result. But by the time 10-docker.sh is running, we have already entered the container and any SINGULARITYENV_PATH variable is long gone. The issue is even more problematic because a bunch of containers already exist that have the 10-docker.sh file in their meta-data.

This PR fixes the problem by adding an ephemeral file to /.singularity.d/env at runtime on demand. If, (and only if) a user has SINGULARITYENV_PATH set to some path in their environment it will be added to a new file called 11-user_defined_SINGULARITYENV_PATH.sh to be sourced following 10-docker. This file will not actually be added to the container (of course) and will disappear upon exit.

For example, if the user does this:

$ export SINGULARITYENV_PATH=/foo

They will get this:

$ singularity exec docker://godlovedc/lolcow printenv | grep PATH
PATH=/foo:/usr/games:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Because of this:

$ singularity exec docker://godlovedc/lolcow cat /.singularity.d/env/11-user_defined_SINGULARITYENV_PATH.sh
[snip]
export PATH=/foo:$PATH

This fixes or addresses the following GitHub issues:

• fixes SINGULARITYENV_PATH doesn't appear to be propagated into the runtime container environment #1352
• see also fixing issue with user custom environment not being set with docker #1363

Checkoff for all PRs:

• I have read the Guidelines for Contributing, and this PR conforms to the stated requirements.
• I have added changes to the CHANGELOG and and documentation updates to the singularityware documentation base.
• I have tested this PR locally with a make test
• This PR is NOT against the project's master branch
• I have added myself as a contributor to the contributors's file
• This PR is ready for review and/or merge

Attn: @singularityware-admin

[moskalenko]

Conceptually, it looks good to me!

[jmstover]

Looks good here.

[jmstover]

Add: *.o

[jmstover]

Just to add in here as well... Change this to:

EXTRA_DIST = 11_udsep.h

[cclerget]

If overlayfs is not supported or not enabled in singularity.conf and if image is not writable, test will always fail

[cclerget]

@GodloveD I just comment about a concern for creation of the environment file into container when there is no overlay or if overlay is disabled in configuration file. The other option for most setup is to copy /.singularity.d/env folder into session folder, add the environment file into session/env and bind it into container over /.singularity.d/env

[cclerget]

Potential problem with overlayfs presence

[GodloveD]

Thank you for the thorough review @cclerget! I'll work to update this so that it works without overlayfs.

[GodloveD]

Heya @cclerget I've tested this on CentOS6 and it seems to be working as expected now. Let me know what you think. Thanks!

[cclerget]

@GodloveD Looks good

[dtrudg]

This is giving correct behaviour for me in all 3 situations.

No Dockerfile custom path, No SINGULARITYENV_PATH

9:11 $ singularity exec docker://alpine env | grep ^PATH=
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

No Dockerfile custom path, Set SINGULARITYENV_PATH

9:11 $ SINGULARITYENV_PATH=/opt singularity exec docker://alpine env | grep ^PATH= 
PATH=/opt:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Dockerfile custom path, No SINGULARITYENV_PATH

9:13 $ singularity exec docker://continuumio/miniconda env | grep ^PATH= 
PATH=/opt/conda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Dockerfile custom path, Set SINGULARITYENV_PATH

9:13 $ SINGULARITYENV_PATH=/testpath singularity exec docker://continuumio/miniconda env | grep ^PATH=
PATH=/testpath:/opt/conda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Also confirmed this behaviour on a .img built under a previous version of Singularity.

dave@fedora:~/Git/singularity
9:14 $ SINGULARITYENV_PATH=/testpath singularity exec digits-6.0.simg env | grep ^PATH=                                
PATH=/testpath:/usr/local/nvidia/bin:/usr/local/cuda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
dave@fedora:~/Git/singularity
9:14 $ singularity exec digits-6.0.simg env | grep ^PATH= 
PATH=/usr/local/nvidia/bin:/usr/local/cuda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

[dtrudg]

Have tested the latest version of this PR and confirm it works as expected to handle Dockerfile and SINGULARITYENV_PATH paths on new and pre-existing containers.

[moskalenko]

I also tested this PR on EL6 and EL7 along the same lines as @dctrud and can confirm that the PR fixes the issue. Thank you!

[gmkurtzer]

Hey everyone,

Apologies for my latent involvement but I just saw this now, and I have some strong feelings about this.

Singularity was creating containers with a "bug" in how we dealt with overriding the contained PATH. A contained environment should be 100% reproducible (to the fullest extent of the realm of possibilities) which means, a container image that has a bug, should be consistent about reproducing that bug between Singularity versions. This means it should be fixed at the build stage so new containers are reproducibly working.

Another issue I have with this implementation is that it injects a runtime based modification into the inner-workings of how the contained "driver" subsystem works. This "driver" subsystem is how the Singularity runtime interfaces with the container runtime (/.singularity.d/drivers/), and the contents of everything within /.singularity.d/ are dependent on how the container was built and what version of Singularity built it; meaning this directory might not even be present in many containers.

I have ideas on how to make this much better for Singularity v3, but I have not had the chance to articulate or code them yet.

At this point, the best way to solve this issue is to provide a fix for new containers being created and not try to kludge for existing (broken) containers. I realize this is not ideal for some people, but from the architectural and consistency perspectives, it is the right way to handle.

Greg

[GodloveD]

Thanks for the comment @gmkurtzer. If we go this route instead of fixing the container at build time we will also need to maintain a similar workflow in future versions. After talking with @gmkurtzer and thinking it over a bit I agree that this is not a good way to fix the issue and we should not try to fix containers that have already been built at runtime. Sorry @moskalenko. I think I'm just going to go ahead and implement a method for overwriting, prepending, or appending to the container path at runtime with env vars like you suggested in #1363 @moskalenko. But this is only going to work for newly created containers. I'll submit a new PR shortly.
EDIT: see #1430

[dtrudg]

This PR is closed in favour of the approach implemented in #1430