Isolate singularity containers more thoroughly for better reproducibility. #18628

rhpvorderman · 2024-07-31T07:37:16Z

This fixes #18620 and several other issues.

singularity exec is now always passed the --contain flag. This ensures only volumes explicitly requested by galaxy are mounted. Singularity mounts $HOME and $PWD by default, but that is redundant since galaxy also mounts the relevant directories.
singularity exec is now passed the --ipc and --pid flags that isolate the IPC and PID namespace respectively. This does not matter much for reproducibility but may fix issues that can occur in tools using the Python multiprocessing module, such as cutadapt. This behavior can be turned of by setting singularity_ipc and singularity_pid to false.
$HOME is no longer mounted by default, see Mounting home in singularity containers is bad for security, reproducibility and creates race conditions #18620. This can be re-enabled by setting singularity_mount_home to true.
By default galaxy ensures a read-only /tmp directory, while simultaneously mounting a specific job temporary directory and setting the appropriate TMP and TMPDIR environment variables. Unfortunately some problematic tools (I am looking at you FastQC!) always write to /tmp and crash as a result. Setting the job tmp_dir variable to true alleviates this problem. This is now documented in the advanced job xml conf sample.
EDIT
The work_directory variable is now actually used, ensuring that the work is executed in the Per-job work directory rather than HOME.
/EDIT

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.

Previously the parameter was unused

…nt_home is true

…ntainer work

bernt-matthias · 2024-08-05T08:43:06Z

lib/galaxy/config/sample/job_conf.xml.sample_advanced

@@ -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


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

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

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 .

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?

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).

galaxyproject/tools-iuc#6207

bernt-matthias · 2024-08-05T08:43:34Z

lib/galaxy/config/sample/job_conf.xml.sample_advanced

+              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


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

What does this mean?

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.

rhpvorderman · 2024-08-05T08:56:16Z

I see now that the mount_home setting is wrong. On newer tool_profiles galaxy will set the $HOME environment variable to point to the correct per job directory. On older tool profiles, home should actually be mounted as this is mentioned in the documentation: https://docs.galaxyproject.org/en/master/dev/schema.html#id2.
So I will fix that.

This should be determined by the tool profile

xref galaxyproject/galaxy#18628

lib/galaxy/tool_util/deps/singularity_util.py

nsoranzo · 2024-08-05T16:26:50Z

lib/galaxy/config/sample/job_conf.xml.sample_advanced

+          <!-- 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> -->


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 ?

--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.

* fastqc: set tmpdir xref galaxyproject/galaxy#18628 * Update tools/fastqc/rgFastQC.xml Co-authored-by: Marius van den Beek <[email protected]> --------- Co-authored-by: Marius van den Beek <[email protected]>

Co-authored-by: Nicola Soranzo <[email protected]>

* fastqc: set tmpdir xref galaxyproject/galaxy#18628 * Update tools/fastqc/rgFastQC.xml Co-authored-by: Marius van den Beek <[email protected]> --------- Co-authored-by: Marius van den Beek <[email protected]>

rhpvorderman added 6 commits July 31, 2024 08:54

Set the singularity working directory to the job working directory

98ab3fc

Previously the parameter was unused

Run singularity containers with maximum isolation by default

b1e5ee4

Singularity containers only mount home directory when singularity_mou…

eb31c51

…nt_home is true

Add documentation for new singularity settings

d6e7c53

Document that tmp_dir should be set to true for proper singularity co…

4c5a860

…ntainer work

Remove redundant --contain switch

74e5612

github-actions bot added area/tool-framework area/tool-dependencies labels Jul 31, 2024

github-actions bot added this to the 24.2 milestone Jul 31, 2024

fix tests for singularity

dc07e50

bernt-matthias reviewed Aug 5, 2024

View reviewed changes

rhpvorderman added 2 commits August 5, 2024 11:10

Remove erroneous mount home functionality

7cff7b4

This should be determined by the tool profile

Remove redundant comment and default

639fd54

bernt-matthias added a commit to bernt-matthias/tools-iuc that referenced this pull request Aug 5, 2024

fastqc: set tmpdir

0417b3d

xref galaxyproject/galaxy#18628

bernt-matthias mentioned this pull request Aug 5, 2024

fastqc: set tmpdir galaxyproject/tools-iuc#6207

Merged

5 tasks

nsoranzo reviewed Aug 5, 2024

View reviewed changes

lib/galaxy/tool_util/deps/singularity_util.py Outdated Show resolved Hide resolved

nsoranzo reviewed Aug 5, 2024

View reviewed changes

shlex quote the working directory

a66b09c

Co-authored-by: Nicola Soranzo <[email protected]>

mvdbeek requested a review from natefoo November 12, 2024 15:47

jdavcs modified the milestones: 24.2, 25.0 Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isolate singularity containers more thoroughly for better reproducibility. #18628

Isolate singularity containers more thoroughly for better reproducibility. #18628

rhpvorderman commented Jul 31, 2024 •

edited

Loading

bernt-matthias Aug 5, 2024

rhpvorderman Aug 5, 2024

rhpvorderman Aug 5, 2024

bernt-matthias Aug 5, 2024

bernt-matthias Aug 5, 2024

bernt-matthias Aug 5, 2024

rhpvorderman Aug 5, 2024

rhpvorderman commented Aug 5, 2024

nsoranzo Aug 5, 2024

rhpvorderman Aug 7, 2024

		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

Isolate singularity containers more thoroughly for better reproducibility. #18628

Are you sure you want to change the base?

Isolate singularity containers more thoroughly for better reproducibility. #18628

Conversation

rhpvorderman commented Jul 31, 2024 • edited Loading

How to test the changes?

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhpvorderman commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhpvorderman commented Jul 31, 2024 •

edited

Loading