-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: dev
Are you sure you want to change the base?
Changes from 7 commits
98ab3fc
b1e5ee4
eb31c51
d6e7c53
4c5a860
74e5612
dc07e50
7cff7b4
639fd54
a66b09c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What does this mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_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. | ||
|
@@ -629,6 +635,24 @@ | |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We have never experienced problems with |
||
<!-- Singularity mounts $HOME by default. This can be problematic if home | ||
contains sensitive data, is shared across nodes or has configuration | ||
files affecting reproducibility. Therefore Galaxy passes the `contain` | ||
argument to isolate $HOME. If $HOME must be mounted, it can be | ||
turned on by setting singularity_mount_home to `true`. --> | ||
<!-- <param id="singularity_mount_home">false</param> --> | ||
<!-- Pass extra arguments to the singularity exec command not covered by the | ||
above options. --> | ||
<!-- <param id="singularity_run_extra_arguments"></param> --> | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 .There was a problem hiding this comment.
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
I found that this /tmp mount is only provided when
tmp_dir
is set to true. Is that intentional behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
galaxyproject/tools-iuc#6207