Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bash login shell can undo/override Dockerfile ENV PATH #603

Closed
mlin opened this issue Sep 21, 2022 · 11 comments
Closed

bash login shell can undo/override Dockerfile ENV PATH #603

mlin opened this issue Sep 21, 2022 · 11 comments
Labels
interop Bears on spec compatibility
Projects

Comments

@mlin
Copy link
Collaborator

mlin commented Sep 21, 2022

We invoke commands with bash -l to create a login shell, apparently following Cromwell. This evaluates /etc/profile , /etc/profile.d/*, ~/.profile, and ~/.bash_profile which wouldn't occur in a non-login shell.

In some distros /etc/profile initializes PATH absolutely, overwriting any existing setting of PATH, such as a value possibly set by an ENV directive in the Dockerfile.

The same applies to any environment variable set in the Dockerfile that's originally initialized in any of those profiles.

@mlin mlin added the interop Bears on spec compatibility label Sep 21, 2022
@mlin mlin added this to Backlog in miniwdl via automation Sep 21, 2022
@adthrasher
Copy link

Hi @mlin, I have been working on a miniwdl extension to enable usage of LSF+Singularity based on the miniwdl-slurm extension. It was straightforward to get basic functionality, but I have run into the issue described here for more complex workflows. In our workflows, we specify the appropriate environment variables when the container images are built. We typically build on a Ubuntu base image. So when the login shell is launched, the environment variables, such as PATH, are overwritten, leading to workflow failures. Dropping the login shell flag seems appropriate as it requires the developer to explicitly set the environment, rather than relying on a particular shell to invoke an environment for them.

@mlin
Copy link
Collaborator Author

mlin commented Jan 10, 2023

Thanks @adthrasher, that's very exciting! cc @rhpvorderman

I'm concerned about this too; bit of a rock & hard place because the login shell flag was added for reasons (#568 cc @nh13) and also apparently to match Cromwell, though I haven't personally confirmed that second part.

It may be necessary to pick one behavior or the other as the default, and also allow you to specify exceptions using a wildcard pattern on the docker image name/tag. I'm inclined to say the default should be historically 'whatever Cromwell does' but need to investigate that further (I can imagine it not necessarily being the same across backends).

And I'll also raise this in the OpenWDL forum as needing spec clarification.

@mlin mlin moved this from Backlog to Next in miniwdl Jan 10, 2023
@adthrasher
Copy link

Thanks for the quick reply (and all your work on miniwdl, itself)! I reviewed #568 (and the associated PR) prior to commenting. I will note that the workflows that I'm testing with all run correctly in Cromwell. I haven't dug into the Cromwell code deeply, but I'm not aware of it executing in a login shell (as I would expect similar errors if it did). Similar to the user in #568, we are using conda environments, but only a single environment per container image. We then use the ENV directive to add the relevant conda paths to the environment in the container at build time (example using samtools), rather than relying on conda itself.

As an example, I took the following toy WDL example and ran it with Cromwell 83 (cromwell run test.wdl) and miniwdl 1.8.0 (miniwdl run test.wdl). Both are running locally with their respective Docker runtimes.

version 1.0

workflow run {

  call conda

  output {
    Array[String] out = conda.out
  }

}

task conda {

  command <<<
    env > out.txt
  >>>

  runtime {
    docker: 'ghcr.io/stjudecloud/samtools:1.0.2'
  }

  output {
    Array[String] out = read_lines("out.txt")
  }

}

From Cromwell, I get:

{
  "outputs": {
    "run.out": [
      "_=/usr/bin/env",
      "OLDPWD=/cromwell-executions/run/279700b8-2633-4ce9-bfea-dad4b1ebc59d/call-conda/execution",
      "_JAVA_OPTIONS=-Djava.io.tmpdir=/cromwell-executions/run/279700b8-2633-4ce9-bfea-dad4b1ebc59d/call-conda/tmp.2a99c656",
      "PATH=/opt/conda/envs/samtools/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
      "SHLVL=1",
      "TMPDIR=/cromwell-executions/run/279700b8-2633-4ce9-bfea-dad4b1ebc59d/call-conda/tmp.2a99c656",
      "HOME=/root",
      "PWD=/cromwell-executions/run/279700b8-2633-4ce9-bfea-dad4b1ebc59d/call-conda/execution",
      "HOSTNAME=3931bfcc1cd9"
    ]
  },
  "id": "279700b8-2633-4ce9-bfea-dad4b1ebc59d"
}

And from miniwdl, I get:

{
  "dir": "/Users/athrashe/Documents/test/20230110_152412_run",
  "outputs": {
    "run.out": [
      "_=/usr/bin/env",
      "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
      "SHLVL=2",
      "HOME=/root",
      "PWD=/mnt/miniwdl_task_container/work",
      "HOSTNAME=141de05a5e77"
    ]
  }
}

Focusing just on the PATH variables, they differ between the two runs. The cromwell result reflects what is defined in the Docker image (ENV PATH /opt/conda/envs/samtools/bin:$PATH) and the miniwdl result reflects the default path without that directive.

Happy to discuss this further and provide additional details if needed.

@mlin
Copy link
Collaborator Author

mlin commented Jan 10, 2023

Thanks @adthrasher for the details

@nh13 seems like this is trending towards backing out the login shell change, but definitely interested in your POV...did I misunderstand that the change promoted getting your stuff that was working with Cromwell work with miniwdl too?

@nh13
Copy link
Contributor

nh13 commented Jan 11, 2023

@mlin I think we wanted a login shell so that bashed would be sourced as an example in #568 we have this in the dockerfile: https://github.com/broadinstitute/viral-core/blob/master/Dockerfile#L31

We then avoid boilerplate in every WDL Atala, centralizing it in the dockerfile. It works with Cromwell so that must mean they probably source bashrc.

I’d be happy if this is changed to opt-in.

@mlin
Copy link
Collaborator Author

mlin commented Jan 11, 2023

Thanks both for input. @adthrasher since you were just running little experiments, could I trouble you to determine whether ~/.bashrc is indeed evaluated on Cromwell task startup?

If so, then we have the curiosity that it's not supposed to be a login shell, but is supposed to evaluate ~/.bashrc. Referring to Bash Startup Files [*], I see a few possible ways there:

  1. "interactive non-login shell" -- seems inappropriate since it's never interactive
  2. set BASH_ENV=$HOME/.bashrc: I think this would work, but I can't find any evidence Cromwell does this
  3. "Invoked by remote shell daemon": plausible, but the doc is vague about how this is determined

[*] The doc doesn't mention ~/.bashrc for login shells, but, usually ~/.profile sources ~/.bashrc.

@mlin
Copy link
Collaborator Author

mlin commented Jan 11, 2023

Ah, here's some evidence that Cromwell on local defaults runs docker containers in interactive mode (docker run -i). Perhaps that propagates to bash "interactive non-login shell", and thus to evaluating ~/.bashrc.

I'm kind of doubtful that this route to evaluating ~/.bashrc was intentional, or that it applies consistently across Cromwell's own backends...

@adthrasher
Copy link

I am unclear from the documentation on the full effect of Docker's interactive mode. The help string does not seem like it is truly interactive.

  -i, --interactive                    Keep STDIN open even if not attached

I used the image provided above by @nh13 with my test WDL script.

version 1.0

workflow run {

  call conda

  output {
    Array[String] out = conda.out
  }

}

task conda {

  command <<<
    env > out.txt
  >>>

  runtime {
    docker: 'quay.io/broadinstitute/viral-core:latest'
  }

  output {
    Array[String] out = read_lines("out.txt")
  }

}

Running with cromwell run test.wdl, I have the following:

{
  "outputs": {
    "run.out": [
      "_=/usr/bin/env",
      "CONDA_DEFAULT_ENV=viral-ngs-env",
      "PATH=/opt/viral-ngs/source:/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
      "PYTHONPATH=/opt/viral-ngs/source",
      "INSTALL_PATH=/opt/viral-ngs",
      "LANGUAGE=en_US:en",
      "SHLVL=1",
      "_JAVA_OPTIONS=-Djava.io.tmpdir=/cromwell-executions/run/3187d922-7d05-4742-85d9-e1140aeff877/call-conda/tmp.f8fa826e",
      "TMPDIR=/cromwell-executions/run/3187d922-7d05-4742-85d9-e1140aeff877/call-conda/tmp.f8fa826e",
      "HOME=/root",
      "PWD=/cromwell-executions/run/3187d922-7d05-4742-85d9-e1140aeff877/call-conda/execution",
      "MINICONDA_PATH=/opt/miniconda",
      "VIRAL_NGS_PATH=/opt/viral-ngs/source",
      "JAVA_HOME=/opt/miniconda",
      "CONDA_PREFIX=/opt/miniconda/envs/viral-ngs-env",
      "OLDPWD=/cromwell-executions/run/3187d922-7d05-4742-85d9-e1140aeff877/call-conda/execution",
      "HOSTNAME=328a71be4343",
      "LANG=en_US.UTF-8",
      "LC_ALL=en_US.UTF-8"
    ]
  },
  "id": "3187d922-7d05-4742-85d9-e1140aeff877"
}

If I instead run the container image interactively (docker run -it quay.io/broadinstitute/viral-core:latest) or with miniwdl run test.wdl, I get different PATH results. Here are all three combined.

Cromwell:
/opt/viral-ngs/source:/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Docker -it:
/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/condabin:/opt/viral-ngs/source:/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

Miniwdl:
/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/condabin:/opt/viral-ngs/source:/opt/miniconda/envs/viral-ngs-env/bin:/opt/miniconda/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin

The Cromwell output of env is also missing Conda environment variables that would be set if the .bashrc file were sourced, such as CONDA_EXE and CONDA_PYTHON_EXE. The linked Dockerfile only explicitly sets CONDA_DEFAULT_ENV, CONDA_PREFIX, and MINICONDA_PATH which reflects the environment when run with Cromwell.

@mlin
Copy link
Collaborator Author

mlin commented Jan 11, 2023

And now I'm stumped 😅

@dpark01
Copy link

dpark01 commented Jan 12, 2023

I'm stumped too. And I do suspect that Cromwell's behavior actually depends a bit on the backend in question (at least that's certainly the case when a Dockerfile's ENTRYPOINT and bash override don't get along with each other).

Agree that maybe some path that allows an override of default behavior (login vs non-login) might be helpful.

Definitely agree that there's a discussion to be had in the OpenWDL forum more broadly, and I certainly have thoughts there (I never understood why WDL task command blocks were so bash-centric... why can't they just be in python or julia or http whatever the docker's native entrypoint is?)

@mlin
Copy link
Collaborator Author

mlin commented Jan 15, 2023

I've found that even with docker run -it, it still matters whether the usage is actually interactive or not. That is, with this setup

echo 'FROM quay.io/broadinstitute/viral-core:2.1.33
RUN echo "echo .bashrc evaluated" | cat - /root/.bashrc > /root/.bashrc.tmp
RUN mv /root/.bashrc.tmp /root/.bashrc' | docker build -t test-bashrc -

If we docker run --rm -it test-bashrc and enter an actual interactive terminal, then .bashrc is evaluated.

.bashrc is not evaluated with either of the following:

docker run --rm -it test-bashrc echo goodbye
docker run --rm -it test-bashrc /bin/bash -c 'echo goodbye'

But .bashrc is evaluated with either of:

docker run --rm -it test-bashrc /bin/bash -ic 'echo goodbye'
docker run --rm -it test-bashrc /bin/bash -lc 'echo goodbye'

So, it seems you have to go rather out of your way to get .bashrc evaluated non-interactively, and thus I don't think a WDL task ought to assume that it is. This leaves mysterious (given @adthrasher complementary experiments above) why adding a command to .bashrc was effective when @nh13 @dpark01 were working with Cromwell last year.

mlin added a commit that referenced this issue Jan 28, 2023
New config options [task_runtime] command_shell and command_preamble allow customization of the shell interpreter used for task commands.

Also remove the default -l (login) flag from the default shell, which was first set in v1.5.3 (#569) but later proved problematic (#603). The new config options can be used to add this flag back if absolutely needed.
@mlin mlin moved this from Next to Pending release in miniwdl Jan 28, 2023
@mlin mlin closed this as completed Jan 28, 2023
miniwdl automation moved this from Pending release to Done Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Bears on spec compatibility
Projects
miniwdl
  
Done
Development

No branches or pull requests

4 participants