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

Isolate singularity containers more thoroughly for better reproducibility. #18628

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
18 changes: 18 additions & 0 deletions lib/galaxy/config/sample/job_conf.xml.sample_advanced
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,12 @@
</destination>
<destination id="singularity_local" runner="local">
<param id="singularity_enabled">true</param>
<!--Galaxy requests an isolated /tmp directory from singularity, which means
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the read-only part of this statement true)?

See discussion in #15673 and tests that have been added in #15614

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. No mount /tmp means /tmp is not mounted. There is still a /tmp dir in the container itself though, it is made read-only .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the discussion you shared

instead (by default) provide a writable /tmp via $_GALAXY_JOB_TMP_DIR/:/tmp:rw note that this dir is still also available as $_GALAXY_JOB_TMP_DIR:rw

I found that this /tmp mount is only provided when tmp_dir is set to true. Is that intentional behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that this /tmp mount is only provided when tmp_dir is set to true. Is that intentional behavior?

Guess so... and if one of TMP, TEMP, TMPDIR is /tmp, or?

Regarding FastQC: we should set --dir in the tool wrapper: https://github.com/s-andrews/FastQC/blob/1faeea0412093224d7f6a07f777fad60a5650795/fastqc#L480 .. the culprit here seems to be JAVA (just another reason to dislike it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be mounted as read-only. This can cause some problems with tools that
do not use the TMP, TEMP, TMPDIR variable family properly. Setting
Comment on lines +608 to +609
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools that do not use the TMP, TEMP, TMPDIR variable family properly

What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proper applications create a temporary directory/file in the directory designated by TMPDIR. Shitty applications just hardcode /tmp. Which causes problems when /tmp is mounted read-only if --no-mount tmp is chosen and no other directory is mounted to the /tmp path.

tmp_dir to true ensures that the /tmp directory in the container points
to the job specific working tmp directory.-->
<param id="tmp_dir">true</param>
<!-- See the above documentation for docker_volumes, singularity_volumes works
almost the same way. The only difference is that $default will expand with
rw directories that in Docker would expand as ro if any of subdirectories are rw.
Expand All @@ -629,6 +635,18 @@
argument by default. You can turn this off by setting singularity_cleanenv to `false`.
-->
<!-- <param id="singularity_cleanenv">true</param> -->
<!-- Singularity by default inherits the PID namespace, this can give
issues with multiprocessing, hence galaxy passes the `pid` argument
by default to isolate the PID namespace. You can turn this of by setting
singularity_pid to `false`.
-->
<!-- <param id="singularity_pid">true</param> -->
<!-- Singularity by default inherits the IPC namespace, this can give
issues with multiprocessing, hence galaxy passes the `ipc` argument
by default to isolate the PID namespace. You can turn this of by setting
singularity_ipc to `false`.
-->
<!-- <param id="singularity_ipc">true</param> -->
Comment on lines +638 to +649
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to expose these 2 arguments as options, or should we just always enabled them as you are proposing to do for --contain ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--contain is not an invasive param. You can undo its effects by simply manually adding the volumes.
This is not the case with --ipc and --pid (as far as I can see). So having an option to disable the flags might be very useful if they cause problems in some tools.

We have never experienced problems with --ipc and --pid at our institute, in fact, we experienced problems when they were not used. However I still take the conservative approach here to allow admins to at least turn it off if it gives them problems.

<!-- Pass extra arguments to the singularity exec command not covered by the
above options. -->
<!-- <param id="singularity_run_extra_arguments"></param> -->
Expand Down
2 changes: 2 additions & 0 deletions lib/galaxy/tool_util/deps/container_classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,8 @@ def containerize_command(self, command: str) -> str:
guest_ports=self.tool_info.guest_ports,
container_name=self.container_name,
cleanenv=asbool(self.prop("cleanenv", singularity_util.DEFAULT_CLEANENV)),
ipc=asbool(self.prop("ipc", singularity_util.DEFAULT_IPC)),
pid=asbool(self.prop("pid", singularity_util.DEFAULT_PID)),
no_mount=self.prop("no_mount", singularity_util.DEFAULT_NO_MOUNT),
**self.get_singularity_target_kwds(),
)
Expand Down
18 changes: 18 additions & 0 deletions lib/galaxy/tool_util/deps/singularity_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@

DEFAULT_WORKING_DIRECTORY = None
DEFAULT_SINGULARITY_COMMAND = "singularity"
# --pid isolates the pid namespace. --ipc isolates
# the ipc namespace, this fixes some issues with python multiprocessing.
# --cleanenv makes sure the current environment is not inherited.
DEFAULT_CLEANENV = True
DEFAULT_IPC = True
DEFAULT_PID = True
DEFAULT_NO_MOUNT = ["tmp"]
DEFAULT_SUDO = False
DEFAULT_SUDO_COMMAND = "sudo"
Expand Down Expand Up @@ -71,6 +76,8 @@ def build_singularity_run_command(
guest_ports: Union[bool, List[str]] = False,
container_name: Optional[str] = None,
cleanenv: bool = DEFAULT_CLEANENV,
ipc: bool = DEFAULT_IPC,
pid: bool = DEFAULT_PID,
no_mount: Optional[List[str]] = DEFAULT_NO_MOUNT,
) -> str:
volumes = volumes or []
Expand All @@ -89,8 +96,19 @@ def build_singularity_run_command(
)
command_parts.append("-s")
command_parts.append("exec")
# Singularity mounts some directories, such as $HOME and $PWD by default.
# using --contain disables this behaviour and only allows explicitly
# requested volumes to be mounted. This gives fully full-control over
# the mounting behavior.
command_parts.append("--contain")
if working_directory:
command_parts.extend(["--pwd", working_directory])
rhpvorderman marked this conversation as resolved.
Show resolved Hide resolved
if cleanenv:
command_parts.append("--cleanenv")
if ipc:
command_parts.append("--ipc")
if pid:
command_parts.append("--pid")
if no_mount:
command_parts.extend(["--no-mount", ",".join(no_mount)])
for volume in volumes:
Expand Down
8 changes: 5 additions & 3 deletions test/unit/tool_util/test_singularity_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ def test_build_singularity_run_command_defaults():
container_command="echo hi",
image="busybox",
)
assert cmd == "singularity -s exec --cleanenv --no-mount tmp busybox echo hi"
assert cmd == "singularity -s exec --contain --cleanenv --ipc --pid --no-mount tmp busybox echo hi"


def test_build_singularity_run_command_no_cleanenv():
def test_build_singularity_run_command_no_cleanenv_ipc_pid():
cmd = build_singularity_run_command(
container_command="echo hi",
image="busybox",
cleanenv=False,
pid=False,
ipc=False,
)
assert cmd == "singularity -s exec --no-mount tmp busybox echo hi"
assert cmd == "singularity -s exec --contain --no-mount tmp busybox echo hi"
Loading