From 0d5359daf31b710d1602545637ade034bdefa9d6 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Thu, 6 Apr 2017 09:40:29 -0400 Subject: [PATCH 1/5] RF: Disable symlinks if destination is on CIFS --- nipype/utils/filemanip.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index cbf32392eb..08fca4d05b 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -12,6 +12,7 @@ import sys import pickle +import subprocess import gzip import hashlib from hashlib import md5 @@ -237,6 +238,30 @@ def hash_timestamp(afile): return md5hex +def _on_cifs(fname): + """ Checks whether a PATH is on a CIFS filesystem mounted in a POSIX + host (i.e., has the "mount" command). + + CIFS shares are how Docker mounts Windows host directories into containers. + CIFS has partial support for symlinks that can break, causing misleading + errors, so this test is used to disable them on such systems. + """ + exit_code, output = subprocess.getstatusoutput("mount") + # Not POSIX + if exit_code != 0: + return False + + # (path, fstype) tuples, sorted by path length (longest first) + paths_types = sorted((line.split()[2:5:2] for line in output.splitlines()), + key=lambda x: len(x[0]), + reverse=True) + # Only the first match counts + for fspath, fstype in paths_types: + if fname.startswith(fspath): + return fstype == 'cifs' + return False + + def copyfile(originalfile, newfile, copy=False, create_new=False, hashmethod=None, use_hardlink=False, copy_related_files=True): @@ -288,6 +313,10 @@ def copyfile(originalfile, newfile, copy=False, create_new=False, if hashmethod is None: hashmethod = config.get('execution', 'hash_method').lower() + # Don't try creating symlinks on CIFS + if _on_cifs(newfile): + copy = True + # Existing file # ------------- # Options: From dafb5eeb213aed18437a8011bec3f10e48dfb6f5 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Thu, 6 Apr 2017 16:40:11 -0400 Subject: [PATCH 2/5] RF: Precompute CIFS table to minimize overhead Only check if symlinks might be tried --- nipype/utils/filemanip.py | 45 ++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 08fca4d05b..07d635af2d 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -238,25 +238,46 @@ def hash_timestamp(afile): return md5hex -def _on_cifs(fname): - """ Checks whether a PATH is on a CIFS filesystem mounted in a POSIX - host (i.e., has the "mount" command). +def _generate_cifs_table(): + """Construct a reverse-length ordered list of mount points that + fall under a CIFS mount. - CIFS shares are how Docker mounts Windows host directories into containers. - CIFS has partial support for symlinks that can break, causing misleading - errors, so this test is used to disable them on such systems. + This precomputation allows efficient checking for whether a given path + would be on a CIFS filesystem. + + On systems without a ``mount`` command, or with no CIFS mounts, returns an + empty list. """ exit_code, output = subprocess.getstatusoutput("mount") # Not POSIX if exit_code != 0: - return False + return [] # (path, fstype) tuples, sorted by path length (longest first) - paths_types = sorted((line.split()[2:5:2] for line in output.splitlines()), - key=lambda x: len(x[0]), - reverse=True) + mount_info = sorted((line.split()[2:5:2] for line in output.splitlines()), + key=lambda x: len(x[0]), + reverse=True) + cifs_paths = [path for path, fstype in mount_info if fstype == 'cifs'] + if cifs_paths == []: + return [] + + return [mount for mount in mount_info + if any(mount[0].startswith(path) for path in cifs_paths)] + + +_cifs_table = _generate_cifs_table() + + +def on_cifs(fname): + """ Checks whether a PATH is on a CIFS filesystem mounted in a POSIX + host (i.e., has the "mount" command). + + CIFS shares are how Docker mounts Windows host directories into containers. + CIFS has partial support for symlinks that can break, causing misleading + errors, so this test is used to disable them on such systems. + """ # Only the first match counts - for fspath, fstype in paths_types: + for fspath, fstype in _cifs_table: if fname.startswith(fspath): return fstype == 'cifs' return False @@ -314,7 +335,7 @@ def copyfile(originalfile, newfile, copy=False, create_new=False, hashmethod = config.get('execution', 'hash_method').lower() # Don't try creating symlinks on CIFS - if _on_cifs(newfile): + if copy is False and on_cifs(newfile): copy = True # Existing file From a4dd6085cd36664b89513c74ee634f69e340fcd4 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Fri, 7 Apr 2017 09:18:58 -0400 Subject: [PATCH 3/5] DOC: Update documentation Remove unnecessary optimization --- nipype/utils/filemanip.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 07d635af2d..59b269e943 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -239,7 +239,7 @@ def hash_timestamp(afile): def _generate_cifs_table(): - """Construct a reverse-length ordered list of mount points that + """Construct a reverse-length-ordered list of mount points that fall under a CIFS mount. This precomputation allows efficient checking for whether a given path @@ -258,8 +258,6 @@ def _generate_cifs_table(): key=lambda x: len(x[0]), reverse=True) cifs_paths = [path for path, fstype in mount_info if fstype == 'cifs'] - if cifs_paths == []: - return [] return [mount for mount in mount_info if any(mount[0].startswith(path) for path in cifs_paths)] @@ -269,14 +267,19 @@ def _generate_cifs_table(): def on_cifs(fname): - """ Checks whether a PATH is on a CIFS filesystem mounted in a POSIX - host (i.e., has the "mount" command). + """ Checks whether a file path is on a CIFS filesystem mounted in a POSIX + host (i.e., has the ``mount`` command). + + On Windows, Docker mounts host directories into containers through CIFS + shares, which has support for Minshall+French symlinks, or text files that + the CIFS driver exposes to the OS as symlinks. + We have found that under concurrent access to the filesystem, this feature + can result in failures to create or read recently-created symlinks, + leading to inconsistent behavior and ``FileNotFoundError``s. - CIFS shares are how Docker mounts Windows host directories into containers. - CIFS has partial support for symlinks that can break, causing misleading - errors, so this test is used to disable them on such systems. + This check is written to support disabling symlinks on CIFS shares. """ - # Only the first match counts + # Only the first match (most recent parent) counts for fspath, fstype in _cifs_table: if fname.startswith(fspath): return fstype == 'cifs' From a9cb28cc1d6f283a61c0856317b691fd6252845f Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Fri, 7 Apr 2017 09:21:53 -0400 Subject: [PATCH 4/5] TEST: Check on_cifs with simulated mount table --- nipype/utils/tests/test_filemanip.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index eba0794e52..04627a078e 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -15,6 +15,7 @@ from ...utils.filemanip import (save_json, load_json, fname_presuffix, fnames_presuffix, hash_rename, check_forhash, + _cifs_table, on_cifs, copyfile, copyfiles, filename_to_list, list_to_filename, check_depends, @@ -334,3 +335,28 @@ def test_related_files(file, length, expected_files): for ef in expected_files: assert ef in related_files + +def test_cifs_check(): + assert isinstance(_cifs_table, list) + assert isinstance(on_cifs('/'), bool) + fake_table = [('/scratch/tmp', 'ext4'), ('/scratch', 'cifs')] + cifs_targets = [('/scratch/tmp/x/y', False), + ('/scratch/tmp/x', False), + ('/scratch/x/y', True), + ('/scratch/x', True), + ('/x/y', False), + ('/x', False), + ('/', False)] + + orig_table = _cifs_table[:] + _cifs_table.clear() + + for target, _ in cifs_targets: + assert on_cifs(target) is False + + _cifs_table.extend(fake_table) + for target, expected in cifs_targets: + assert on_cifs(target) is expected + + _cifs_table.clear() + _cifs_table.extend(orig_table) From ae5c6e1e46b9b9f33a498aacc3d3c3c352ef2e82 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Fri, 7 Apr 2017 10:36:20 -0400 Subject: [PATCH 5/5] PY2: Remove list.clear --- nipype/utils/tests/test_filemanip.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index 04627a078e..9e0f3abb78 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -349,7 +349,7 @@ def test_cifs_check(): ('/', False)] orig_table = _cifs_table[:] - _cifs_table.clear() + _cifs_table[:] = [] for target, _ in cifs_targets: assert on_cifs(target) is False @@ -358,5 +358,5 @@ def test_cifs_check(): for target, expected in cifs_targets: assert on_cifs(target) is expected - _cifs_table.clear() + _cifs_table[:] = [] _cifs_table.extend(orig_table)