From 25807f2690f52f9642310a51e3cc605617180d1f Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 8 Dec 2019 03:19:24 +0200 Subject: [PATCH] dvc: make flufl.lock opt-in and use zc.lockfile As it turned out (see issue numbers down below), we can't really take hardlinks for granted, so `flufl.lock` is not a panacea for all filesystems. Considering that the vast majority of filesystems that our users use support `zc.lockfile`(flock-based) and it has benefits like more reliable mechanism, auto-delete when process exits, more sturdy implementation, etc, it makes more sense to bring it back and use by default again. For filesystems that don't support `flock()`, users will be able to manually enable `flufl.lock` use through the config option. It would be ideal if we could auto-detect that flock is not supported, but in the real world, it turned out to be non-trivial, as it might hang forever in a kernel context, which makes the implementation way too complex for our purposes. So what we're doing instead is showing a message before locking with `zc.lockfile` that, under normal circumstances will disappear once the lock is taken or failed, otherwise it will point users to the related documentation where they can learn about how to opt-in for `flufl.lock`. Fixes #2831 Fixes #2897 Related #2860 --- dvc/command/base.py | 15 +++--- dvc/command/daemon.py | 7 ++- dvc/config.py | 2 + dvc/lock.py | 111 +++++++++++++++++++++++++----------------- dvc/repo/__init__.py | 7 ++- dvc/updater.py | 12 +++-- dvc/utils/__init__.py | 8 +++ setup.py | 8 +-- 8 files changed, 102 insertions(+), 68 deletions(-) diff --git a/dvc/command/base.py b/dvc/command/base.py index 56a9aed232..d8ccb2c8d5 100644 --- a/dvc/command/base.py +++ b/dvc/command/base.py @@ -2,8 +2,6 @@ import logging -import colorama - logger = logging.getLogger(__name__) @@ -24,15 +22,13 @@ def fix_subparsers(subparsers): def append_doc_link(help_message, path): + from dvc.utils import format_link + if not path: return help_message doc_base = "https://man.dvc.org/" - return "{message}\nDocumentation: <{blue}{base}{path}{nc}>".format( - message=help_message, - base=doc_base, - path=path, - blue=colorama.Fore.CYAN, - nc=colorama.Fore.RESET, + return "{message}\nDocumentation: {link}".format( + message=help_message, link=format_link(doc_base + path) ) @@ -44,7 +40,8 @@ def __init__(self, args): self.repo = Repo() self.config = self.repo.config self.args = args - updater = Updater(self.repo.dvc_dir) + hardlink_lock = self.config.config["core"].get("hardlink_lock", False) + updater = Updater(self.repo.dvc_dir, hardlink_lock=hardlink_lock) updater.check() @property diff --git a/dvc/command/daemon.py b/dvc/command/daemon.py index da92105847..26362287d0 100644 --- a/dvc/command/daemon.py +++ b/dvc/command/daemon.py @@ -12,11 +12,16 @@ class CmdDaemonUpdater(CmdDaemonBase): def run(self): import os from dvc.repo import Repo + from dvc.config import Config from dvc.updater import Updater root_dir = Repo.find_root() dvc_dir = os.path.join(root_dir, Repo.DVC_DIR) - updater = Updater(dvc_dir) + config = Config(dvc_dir, verify=False) + hardlink_lock = config.config.get("core", {}).get( + "hardlink_lock", False + ) + updater = Updater(dvc_dir, hardlink_lock=hardlink_lock) updater.fetch(detach=False) return 0 diff --git a/dvc/config.py b/dvc/config.py index 25b091c67b..c6ebc6399d 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -125,6 +125,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_CORE_INTERACTIVE = "interactive" SECTION_CORE_ANALYTICS = "analytics" SECTION_CORE_CHECKSUM_JOBS = "checksum_jobs" + SECTION_CORE_HARDLINK_LOCK = "hardlink_lock" SECTION_CACHE = "cache" SECTION_CACHE_DIR = "dir" @@ -160,6 +161,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes Optional(SECTION_CORE_INTERACTIVE, default=False): Bool, Optional(SECTION_CORE_ANALYTICS, default=True): Bool, SECTION_CORE_CHECKSUM_JOBS: All(Coerce(int), Range(1)), + Optional(SECTION_CORE_HARDLINK_LOCK, default=False): Bool, } # aws specific options diff --git a/dvc/lock.py b/dvc/lock.py index 147118c0fe..b7c7560f73 100644 --- a/dvc/lock.py +++ b/dvc/lock.py @@ -6,12 +6,13 @@ import time from datetime import timedelta +import zc.lockfile from funcy.py3 import lkeep from dvc.exceptions import DvcException -from dvc.utils import makedirs -from dvc.utils.compat import is_py3 - +from dvc.utils import makedirs, format_link +from dvc.utils.compat import is_py3, is_py2 +from dvc.progress import Tqdm DEFAULT_TIMEOUT = 5 @@ -26,10 +27,60 @@ class LockError(DvcException): """Thrown when unable to acquire the lock for dvc repo.""" +class Lock(object): + """Class for dvc repo lock. + + Uses zc.lockfile as backend. + """ + + def __init__(self, lockfile, friendly=False, **kwargs): + self._friendly = friendly + self.lockfile = lockfile + self._lock = None + + @property + def files(self): + return [self.lockfile] + + def _do_lock(self): + try: + with Tqdm( + bar_format="{desc}", + disable=not self._friendly, + desc=( + "If DVC froze, see `hardlink_lock` in {}".format( + format_link("man.dvc.org/config#core") + ) + ), + ): + self._lock = zc.lockfile.LockFile(self.lockfile) + except zc.lockfile.LockError: + raise LockError(FAILED_TO_LOCK_MESSAGE) + + def lock(self): + try: + self._do_lock() + return + except LockError: + time.sleep(DEFAULT_TIMEOUT) + + self._do_lock() + + def unlock(self): + self._lock.close() + self._lock = None + + def __enter__(self): + self.lock() + + def __exit__(self, typ, value, tbck): + self.unlock() + + if is_py3: import flufl.lock - class Lock(flufl.lock.Lock): + class HardlinkLock(flufl.lock.Lock): """Class for dvc repo lock. Args: @@ -38,7 +89,7 @@ class Lock(flufl.lock.Lock): tmp_dir (str): a directory to store claim files. """ - def __init__(self, lockfile, tmp_dir=None): + def __init__(self, lockfile, tmp_dir=None, **kwargs): import socket self._tmp_dir = tmp_dir @@ -101,44 +152,14 @@ def __del__(self): pass -else: - import zc.lockfile - - class Lock(object): - """Class for dvc repo lock. - - Uses zc.lockfile as backend. - """ - - def __init__(self, lockfile, tmp_dir=None): - self.lockfile = lockfile - self._lock = None - - @property - def files(self): - return [self.lockfile] - - def _do_lock(self): - try: - self._lock = zc.lockfile.LockFile(self.lockfile) - except zc.lockfile.LockError: - raise LockError(FAILED_TO_LOCK_MESSAGE) - - def lock(self): - try: - self._do_lock() - return - except LockError: - time.sleep(DEFAULT_TIMEOUT) - - self._do_lock() - - def unlock(self): - self._lock.close() - self._lock = None - - def __enter__(self): - self.lock() +def make_lock(lockfile, tmp_dir=None, friendly=False, hardlink_lock=False): + if hardlink_lock and is_py2: + raise DvcException( + "Hardlink locks are not supported on Python <3.5. " + "See `hardlink_lock` in {}".format( + format_link("man.dvc.org/config#core") + ) + ) - def __exit__(self, typ, value, tbck): - self.unlock() + cls = HardlinkLock if hardlink_lock else Lock + return cls(lockfile, tmp_dir=tmp_dir, friendly=friendly) diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index ac013313b0..86f03ac5e9 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -69,7 +69,7 @@ class Repo(object): def __init__(self, root_dir=None): from dvc.state import State - from dvc.lock import Lock + from dvc.lock import make_lock from dvc.scm import SCM from dvc.cache import Cache from dvc.data_cloud import DataCloud @@ -88,9 +88,12 @@ def __init__(self, root_dir=None): self.tree = WorkingTree(self.root_dir) - self.lock = Lock( + hardlink_lock = self.config.config["core"].get("hardlink_lock", False) + self.lock = make_lock( os.path.join(self.dvc_dir, "lock"), tmp_dir=os.path.join(self.dvc_dir, "tmp"), + hardlink_lock=hardlink_lock, + friendly=True, ) # NOTE: storing state and link_state in the repository itself to avoid # any possible state corruption in 'shared cache dir' scenario. diff --git a/dvc/updater.py b/dvc/updater.py index 77145ef9d4..6612219262 100644 --- a/dvc/updater.py +++ b/dvc/updater.py @@ -9,8 +9,7 @@ from packaging import version from dvc import __version__ -from dvc.lock import Lock -from dvc.lock import LockError +from dvc.lock import make_lock, LockError from dvc.utils import boxify from dvc.utils import env2bool from dvc.utils.pkg import PKG @@ -24,11 +23,14 @@ class Updater(object): # pragma: no cover TIMEOUT = 24 * 60 * 60 # every day TIMEOUT_GET = 10 - def __init__(self, dvc_dir): + def __init__(self, dvc_dir, friendly=False, hardlink_lock=False): self.dvc_dir = dvc_dir self.updater_file = os.path.join(dvc_dir, self.UPDATER_FILE) - self.lock = Lock( - self.updater_file + ".lock", tmp_dir=os.path.join(dvc_dir, "tmp") + self.lock = make_lock( + self.updater_file + ".lock", + tmp_dir=os.path.join(dvc_dir, "tmp"), + friendly=friendly, + hardlink_lock=hardlink_lock, ) self.current = version.parse(__version__).base_version diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index 68ba418778..a1d15aa0b3 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -420,3 +420,11 @@ def resolve_output(inp, out): if os.path.isdir(out): return os.path.join(out, name) return out + + +def format_link(link): + import colorama + + return "<{blue}{link}{nc}>".format( + blue=colorama.Fore.CYAN, link=link, nc=colorama.Fore.RESET + ) diff --git a/setup.py b/setup.py index d53fac5f6b..29a4a09363 100644 --- a/setup.py +++ b/setup.py @@ -79,6 +79,7 @@ def run(self): "shortuuid>=0.5.0", "tqdm>=4.40.0,<5", "packaging>=19.0", + "zc.lockfile>=1.2.1", "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", ] @@ -163,12 +164,7 @@ def run(self): "ssh_gssapi": ssh_gssapi, "hdfs": hdfs, # NOTE: https://github.com/inveniosoftware/troubleshooting/issues/1 - ":python_version=='2.7'": [ - "futures", - "pathlib2", - "contextlib2", - "zc.lockfile>=1.2.1", - ], + ":python_version=='2.7'": ["futures", "pathlib2", "contextlib2"], ":python_version>='3.0'": ["flufl.lock>=3.2"], "tests": tests_requirements, },