Repository: DataBiosphere/toil

Issue:
Allow CWL and WDL workflows to minimize access to a shared-filesystem file job store
We're having some trouble on our Phoenix Slurm cluster, where Toil workflows are putting a high load on the NFS share where the input data and/or file job store are kept.

This is probably due to using symlinks and hardlinks as much as we can by default, and due to the caching file store refusing to enable on Slurm still. So we possibly symlink the input files to the job store, then we follow symlinks to mount files straight from the job store or the original input locations into the containers for jobs, and then the jobs all read directly against the NFS share instead of from local copies, even if many jobs are running at the same time on the same machine.

The NFS implementation in the kernel doesn't do anything smart about caching to local disk for us, so this results in a lot of network traffic and a lot of IO on the NFS server.

We probably could reduce this load and improve performance if we got caching working on Slurm, and allowed multiple jobs on the same node using the same file at the same time to copy it once and work on it from the local filesystem.

We also might get better results, depending on workload structure, if we had a way to lock out links into/mounting from the file job store. In the worst case each job would make a full copy of its input files to read even a single byte, but if the job needs to go through the input file 10 times or does a lot of random access jumping all over it, that could still be more efficient than working over NFS.



┆Issue is synchronized with this [Jira Story](https://ucsc-cgl.atlassian.net/browse/TOIL-1441)
┆Issue Number: TOIL-1441


Discussion:

Patch:
diff --git a/docs/running/cliOptions.rst b/docs/running/cliOptions.rst
index c66c69c4..e4a8a96e 100644
--- a/docs/running/cliOptions.rst
+++ b/docs/running/cliOptions.rst
@@ -286,6 +286,13 @@ Allows configuring Toil's data storage.
                         to use a batch system that does not support
                         cleanup. Set to "true" if caching
                         is desired.
+  --symlinkJobStoreReads BOOL
+                        Allow reads and container mounts from a JobStore's
+                        shared filesystem directly via symlink. Can be turned
+                        off if the shared filesystem can't support the IO load
+                        of all the jobs reading from it at once, and you want
+                        to use ``--caching=True`` to make jobs on each node
+                        read from node-local cache storage. (Default=True)
 
 **Autoscaling Options**
 Allows the specification of the minimum and maximum number of nodes in an
diff --git a/src/toil/common.py b/src/toil/common.py
index a446cc2e..5c4083ee 100644
--- a/src/toil/common.py
+++ b/src/toil/common.py
@@ -169,6 +169,7 @@ class Config:
     caching: Optional[bool]
     symlinkImports: bool
     moveOutputs: bool
+    symlink_job_store_reads: bool
 
     # Autoscaling options
     provisioner: Optional[str]
@@ -338,6 +339,7 @@ class Config:
         set_option("symlinkImports", old_names=["linkImports"])
         set_option("moveOutputs", old_names=["moveExports"])
         set_option("caching", old_names=["enableCaching"])
+        set_option("symlink_job_store_reads")
 
         # Autoscaling options
         set_option("provisioner")
diff --git a/src/toil/jobStores/fileJobStore.py b/src/toil/jobStores/fileJobStore.py
index 0a732747..4d8e94fa 100644
--- a/src/toil/jobStores/fileJobStore.py
+++ b/src/toil/jobStores/fileJobStore.py
@@ -104,6 +104,7 @@ class FileJobStore(AbstractJobStore):
 
         self.linkImports = None
         self.moveExports = None
+        self.symlink_job_store_reads = None
 
     def __repr__(self):
         return f'FileJobStore({self.jobStoreDir})'
@@ -123,6 +124,7 @@ class FileJobStore(AbstractJobStore):
         os.makedirs(self.sharedFilesDir, exist_ok=True)
         self.linkImports = config.symlinkImports
         self.moveExports = config.moveOutputs
+        self.symlink_job_store_reads = config.symlink_job_store_reads
         super().initialize(config)
 
     def resume(self):
@@ -314,8 +316,8 @@ class FileJobStore(AbstractJobStore):
             atomic_copy(srcPath, dst_path)
 
     def _import_file(self, otherCls, uri, shared_file_name=None, hardlink=False, symlink=True):
-        # symlink argument says whether the caller can take symlinks or not
-        # ex: if false, it implies the workflow cannot work with symlinks and thus will hardlink imports
+        # symlink argument says whether the caller can take symlinks or not.
+        # ex: if false, it means the workflow cannot work with symlinks and we need to hardlink or copy.
         # default is true since symlinking everything is ideal
         uri_path = unquote(uri.path)
         if issubclass(otherCls, FileJobStore):
@@ -515,8 +517,9 @@ class FileJobStore(AbstractJobStore):
             # one over the other will fail.
             return
 
-        if symlink:
-            # If the reader will accept a symlink, so always give them one.
+        if symlink and self.symlink_job_store_reads:
+            # If the reader will accept a symlink, and we are willing to
+            # symlink into the jobstore, always give them one.
             # There's less that can go wrong.
             try:
                 os.symlink(jobStoreFilePath, local_path)
diff --git a/src/toil/options/common.py b/src/toil/options/common.py
index 97754009..b4f6e25d 100644
--- a/src/toil/options/common.py
+++ b/src/toil/options/common.py
@@ -349,7 +349,7 @@ def add_base_toil_options(parser: ArgumentParser, jobstore_as_flag: bool = False
     link_imports_help = ("When using a filesystem based job store, CWL input files are by default symlinked in.  "
                          "Setting this option to True instead copies the files into the job store, which may protect "
                          "them from being modified externally.  When set to False, as long as caching is enabled, "
-                         "Toil will protect the file automatically by changing the permissions to read-only."
+                         "Toil will protect the file automatically by changing the permissions to read-only. "
                          "default=%(default)s")
     link_imports.add_argument("--symlinkImports", dest="symlinkImports", type=strtobool, default=True,
                               metavar="BOOL", help=link_imports_help)
@@ -357,7 +357,7 @@ def add_base_toil_options(parser: ArgumentParser, jobstore_as_flag: bool = False
     move_exports_help = ('When using a filesystem based job store, output files are by default moved to the '
                          'output directory, and a symlink to the moved exported file is created at the initial '
                          'location.  Setting this option to True instead copies the files into the output directory.  '
-                         'Applies to filesystem-based job stores only.'
+                         'Applies to filesystem-based job stores only. '
                          'default=%(default)s')
     move_exports.add_argument("--moveOutputs", dest="moveOutputs", type=strtobool, default=False, metavar="BOOL",
                               help=move_exports_help)
@@ -368,6 +368,11 @@ def add_base_toil_options(parser: ArgumentParser, jobstore_as_flag: bool = False
                          help=caching_help)
     # default is None according to PR 4299, seems to be generated at runtime
 
+    file_store_options.add_argument("--symlinkJobStoreReads", dest="symlink_job_store_reads", type=strtobool, default=True,
+                                    metavar="BOOL",
+                                    help="Allow reads and container mounts from a JobStore's shared filesystem directly "
+                                         "via symlink. default=%(default)s")
+
     # Auto scaling options
     autoscaling_options = parser.add_argument_group(
         title="Toil options for autoscaling the cluster of worker nodes.",


Test patch:
diff --git a/src/toil/test/jobStores/jobStoreTest.py b/src/toil/test/jobStores/jobStoreTest.py
index b7123067..33453f92 100644
--- a/src/toil/test/jobStores/jobStoreTest.py
+++ b/src/toil/test/jobStores/jobStoreTest.py
@@ -1226,6 +1226,32 @@ class FileJobStoreTest(AbstractJobStoreTest.Test):
                 os.remove(filename)
         os.remove(srcUrl[7:])
 
+    def test_symlink_read_control(self):
+        """
+        Test that files are read by symlink when expected
+        """
+        
+        for should_link in (False, True):
+            # Configure a jobstore to symlink out reads or not, as appropriate
+            config = self._createConfig()
+            config.symlink_job_store_reads = should_link
+            store = FileJobStore(self.namePrefix + ("-link" if should_link else "-nolink"))
+            store.initialize(config)
+            
+            # Put something in the job store
+            src_url, _ = self._prepareTestFile(self._externalStore(), 1)
+            file_id = store.import_file(src_url, symlink=False)
+
+            # Read it out, accepting a symlink
+            dest_dir = self._createTempDir()
+            dest_path = os.path.join(dest_dir, "file.dat")
+            store.read_file(file_id, dest_path, symlink = True)
+
+            # Make sure we get a symlink exactly when configured to
+            assert os.path.exists(dest_path)
+            assert os.path.islink(dest_path) == should_link
+            
+
 
 @needs_google_project
 @needs_google_storage