From 54201e3c0216656b5b933fa2e5c36b9afcb1dc4f Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Fri, 18 May 2018 14:41:03 -0700
Subject: [PATCH] locks: add configuration and command-line options to
enable/disable locks (#7692)
- spack.util.lock behaves the same as llnl.util.lock, but Lock._lock and
Lock._unlock do nothing.
- can be disabled with a control variable.
- configuration options can enable/disable locking:
- `locks` option in spack configuration controls whether Spack will use filesystem locks or not.
- `-l` and `-L` command-line options can force-disable or force-enable locking.
- Spack will check for group- and world-writability before disabling
locks, and it will not allow a group- or world-writable instance to
have locks disabled.
- update documentation
---
etc/spack/defaults/config.yaml | 12 +++
lib/spack/docs/basic_usage.rst | 41 +++++++---
lib/spack/docs/config_yaml.rst | 11 +++
lib/spack/llnl/util/lock.py | 30 ++++---
lib/spack/spack/database.py | 2 +-
lib/spack/spack/main.py | 78 +++++++++++-------
lib/spack/spack/schema/config.py | 1 +
lib/spack/spack/stage.py | 4 +-
lib/spack/spack/test/llnl/util/lock.py | 105 ++++++++++++++++++++++++-
lib/spack/spack/util/file_cache.py | 2 +-
lib/spack/spack/util/lock.py | 91 +++++++++++++++++++++
11 files changed, 323 insertions(+), 54 deletions(-)
create mode 100644 lib/spack/spack/util/lock.py
diff --git a/etc/spack/defaults/config.yaml b/etc/spack/defaults/config.yaml
index b5e517c98d..ef9d288173 100644
--- a/etc/spack/defaults/config.yaml
+++ b/etc/spack/defaults/config.yaml
@@ -18,13 +18,16 @@ config:
# You can use $spack here to refer to the root of the spack instance.
install_tree: $spack/opt/spack
+
# Locations where templates should be found
template_dirs:
- $spack/templates
+
# default directory layout
directory_layout: "${ARCHITECTURE}/${COMPILERNAME}-${COMPILERVER}/${PACKAGE}-${VERSION}-${HASH}"
+
# Locations where different types of modules should be installed.
module_roots:
tcl: $spack/share/spack/modules
@@ -74,6 +77,15 @@ config:
dirty: false
+ # When set to true, concurrent instances of Spack will use locks to
+ # avoid modifying the install tree, database file, etc. If false, Spack
+ # will disable all locking, but you must NOT run concurrent instances
+ # of Spack. For filesystems that don't support locking, you should set
+ # this to false and run one Spack at a time, but otherwise we recommend
+ # enabling locks.
+ locks: true
+
+
# The default number of jobs to use when running `make` in parallel.
# If set to 4, for example, `spack install` will run `make -j4`.
# If not set, all available cores are used by default.
diff --git a/lib/spack/docs/basic_usage.rst b/lib/spack/docs/basic_usage.rst
index bb426b4378..1a7576c74e 100644
--- a/lib/spack/docs/basic_usage.rst
+++ b/lib/spack/docs/basic_usage.rst
@@ -1093,22 +1093,43 @@ several variants:
Filesystem requirements
-----------------------
-Spack currently needs to be run from a filesystem that supports
+By default, Spack needs to be run from a filesystem that supports
``flock`` locking semantics. Nearly all local filesystems and recent
-versions of NFS support this, but parallel filesystems may be mounted
-without ``flock`` support enabled. You can determine how your
-filesystems are mounted with ``mount -p``. The output for a Lustre
+versions of NFS support this, but parallel filesystems or NFS volumes may
+be configured without ``flock`` support enabled. You can determine how
+your filesystems are mounted with ``mount``. The output for a Lustre
filesystem might look like this:
.. code-block:: console
- $ mount -l | grep lscratch
- pilsner-mds1-lnet0@o2ib100:/lsd on /p/lscratchd type lustre (rw,nosuid,noauto,_netdev,lazystatfs,flock)
- porter-mds1-lnet0@o2ib100:/lse on /p/lscratche type lustre (rw,nosuid,noauto,_netdev,lazystatfs,flock)
+ $ mount | grep lscratch
+ mds1-lnet0@o2ib100:/lsd on /p/lscratchd type lustre (rw,nosuid,lazystatfs,flock)
+ mds2-lnet0@o2ib100:/lse on /p/lscratche type lustre (rw,nosuid,lazystatfs,flock)
-Note the ``flock`` option on both Lustre mounts. If you do not see
-this or a similar option for your filesystem, you may need ot ask your
-system administrator to enable ``flock``.
+Note the ``flock`` option on both Lustre mounts.
+
+If you do not see this or a similar option for your filesystem, you have
+a few options. First, you can move your Spack installation to a
+filesystem that supports locking. Second, you could ask your system
+administrator to enable ``flock`` for your filesystem.
+
+If none of those work, you can disable locking in one of two ways:
+
+ 1. Run Spack with the ``-L`` or ``--disable-locks`` option to disable
+ locks on a call-by-call basis.
+ 2. Edit :ref:`config.yaml <config-yaml>` and set the ``locks`` option
+ to ``false`` to always disable locking.
+
+.. warning::
+
+ If you disable locking, concurrent instances of Spack will have no way
+ to avoid stepping on each other. You must ensure that there is only
+ **one** instance of Spack running at a time. Otherwise, Spack may end
+ up with a corrupted database file, or you may not be able to see all
+ installed packages in commands like ``spack find``.
+
+ If you are unfortunate enough to run into this situation, you may be
+ able to fix it by running ``spack reindex``.
This issue typically manifests with the error below:
diff --git a/lib/spack/docs/config_yaml.rst b/lib/spack/docs/config_yaml.rst
index da760f05e3..35bc22d72e 100644
--- a/lib/spack/docs/config_yaml.rst
+++ b/lib/spack/docs/config_yaml.rst
@@ -150,6 +150,17 @@ checksum, and will refuse to build packages that it cannot verify. Set
to ``false`` to disable these checks. Disabling this can expose you to
attacks. Use at your own risk.
+--------------------
+``locks``
+--------------------
+
+When set to ``true``, concurrent instances of Spack will use locks to
+avoid modifying the install tree, database file, etc. If false, Spack
+will disable all locking, but you must **not** run concurrent instances
+of Spack. For filesystems that don't support locking, you should set
+this to ``false`` and run one Spack at a time, but otherwise we recommend
+enabling locks.
+
--------------------
``dirty``
--------------------
diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index ed797d2ebf..0d072418cc 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -195,11 +195,13 @@ def acquire_read(self, timeout=_default_timeout):
"""
if self._reads == 0 and self._writes == 0:
- tty.debug('READ LOCK: {0.path}[{0._start}:{0._length}] [Acquiring]'
- .format(self))
+ self._debug(
+ 'READ LOCK: {0.path}[{0._start}:{0._length}] [Acquiring]'
+ .format(self))
self._lock(fcntl.LOCK_SH, timeout=timeout) # can raise LockError.
- tty.debug('READ LOCK: {0.path}[{0._start}:{0._length}] [Acquired]'
- .format(self))
+ self._debug(
+ 'READ LOCK: {0.path}[{0._start}:{0._length}] [Acquired]'
+ .format(self))
self._reads += 1
return True
else:
@@ -218,12 +220,13 @@ def acquire_write(self, timeout=_default_timeout):
"""
if self._writes == 0:
- tty.debug(
+ self._debug(
'WRITE LOCK: {0.path}[{0._start}:{0._length}] [Acquiring]'
.format(self))
self._lock(fcntl.LOCK_EX, timeout=timeout) # can raise LockError.
- tty.debug('WRITE LOCK: {0.path}[{0._start}:{0._length}] [Acquired]'
- .format(self))
+ self._debug(
+ 'WRITE LOCK: {0.path}[{0._start}:{0._length}] [Acquired]'
+ .format(self))
self._writes += 1
return True
else:
@@ -243,8 +246,9 @@ def release_read(self):
assert self._reads > 0
if self._reads == 1 and self._writes == 0:
- tty.debug('READ LOCK: {0.path}[{0._start}:{0._length}] [Released]'
- .format(self))
+ self._debug(
+ 'READ LOCK: {0.path}[{0._start}:{0._length}] [Released]'
+ .format(self))
self._unlock() # can raise LockError.
self._reads -= 1
return True
@@ -265,8 +269,9 @@ def release_write(self):
assert self._writes > 0
if self._writes == 1 and self._reads == 0:
- tty.debug('WRITE LOCK: {0.path}[{0._start}:{0._length}] [Released]'
- .format(self))
+ self._debug(
+ 'WRITE LOCK: {0.path}[{0._start}:{0._length}] [Released]'
+ .format(self))
self._unlock() # can raise LockError.
self._writes -= 1
return True
@@ -274,6 +279,9 @@ def release_write(self):
self._writes -= 1
return False
+ def _debug(self, *args):
+ tty.debug(*args)
+
class LockTransaction(object):
"""Simple nested transaction context manager that uses a file lock.
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index 3c62cc588f..4bc6891ec8 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -52,7 +52,6 @@
import llnl.util.tty as tty
from llnl.util.filesystem import mkdirp
-from llnl.util.lock import Lock, WriteTransaction, ReadTransaction
import spack.store
import spack.repo
@@ -63,6 +62,7 @@
from spack.directory_layout import DirectoryLayoutError
from spack.error import SpackError
from spack.version import Version
+from spack.util.lock import Lock, WriteTransaction, ReadTransaction
# DB goes in this directory underneath the root
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index 740367168f..7af096c821 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -250,7 +250,7 @@ def add_subcommand_group(title, commands):
# epilog
formatter.add_text("""\
{help}:
- spack help --all list all available commands
+ spack help --all list all commands and options
spack help <command> help on a specific command
spack help --spec help on the spec syntax
spack docs open http://spack.rtfd.io/ in a browser"""
@@ -311,33 +311,50 @@ def make_argument_parser(**kwargs):
# stat names in groups of 7, for nice wrapping.
stat_lines = list(zip(*(iter(stat_names),) * 7))
- parser.add_argument('-h', '--help', action='store_true',
- help="show this help message and exit")
- parser.add_argument('--color', action='store', default='auto',
- choices=('always', 'never', 'auto'),
- help="when to colorize output (default: auto)")
- parser.add_argument('-d', '--debug', action='store_true',
- help="write out debug logs during compile")
- parser.add_argument('-D', '--pdb', action='store_true',
- help="run spack under the pdb debugger")
- parser.add_argument('-k', '--insecure', action='store_true',
- help="do not check ssl certificates when downloading")
- parser.add_argument('-m', '--mock', action='store_true',
- help="use mock packages instead of real ones")
- parser.add_argument('-p', '--profile', action='store_true',
- dest='spack_profile',
- help="profile execution using cProfile")
- parser.add_argument('-P', '--sorted-profile', default=None, metavar="STAT",
- help="profile and sort by one or more of:\n[%s]" %
- ',\n '.join([', '.join(line) for line in stat_lines]))
- parser.add_argument('--lines', default=20, action='store',
- help="lines of profile output or 'all' (default: 20)")
- parser.add_argument('-v', '--verbose', action='store_true',
- help="print additional output during builds")
- parser.add_argument('-s', '--stacktrace', action='store_true',
- help="add stacktraces to all printed statements")
- parser.add_argument('-V', '--version', action='store_true',
- help='show version number and exit')
+ parser.add_argument(
+ '-h', '--help', action='store_true',
+ help="show this help message and exit")
+ parser.add_argument(
+ '--color', action='store', default='auto',
+ choices=('always', 'never', 'auto'),
+ help="when to colorize output (default: auto)")
+ parser.add_argument(
+ '-d', '--debug', action='store_true',
+ help="write out debug logs during compile")
+ parser.add_argument(
+ '-D', '--pdb', action='store_true',
+ help="run spack under the pdb debugger")
+ parser.add_argument(
+ '-k', '--insecure', action='store_true',
+ help="do not check ssl certificates when downloading")
+ parser.add_argument(
+ '-l', '--enable-locks', action='store_true', dest='locks',
+ default=None, help="use filesystem locking (default)")
+ parser.add_argument(
+ '-L', '--disable-locks', action='store_false', dest='locks',
+ help="do not use filesystem locking (unsafe)")
+ parser.add_argument(
+ '-m', '--mock', action='store_true',
+ help="use mock packages instead of real ones")
+ parser.add_argument(
+ '-p', '--profile', action='store_true', dest='spack_profile',
+ help="profile execution using cProfile")
+ parser.add_argument(
+ '-P', '--sorted-profile', default=None, metavar="STAT",
+ help="profile and sort by one or more of:\n[%s]" %
+ ',\n '.join([', '.join(line) for line in stat_lines]))
+ parser.add_argument(
+ '--lines', default=20, action='store',
+ help="lines of profile output or 'all' (default: 20)")
+ parser.add_argument(
+ '-v', '--verbose', action='store_true',
+ help="print additional output during builds")
+ parser.add_argument(
+ '-s', '--stacktrace', action='store_true',
+ help="add stacktraces to all printed statements")
+ parser.add_argument(
+ '-V', '--version', action='store_true',
+ help='show version number and exit')
return parser
@@ -348,6 +365,11 @@ def setup_main_options(args):
tty.set_debug(args.debug)
tty.set_stacktrace(args.stacktrace)
+ # override lock configuration if passed on command line
+ if args.locks is not None:
+ spack.util.lock.check_lock_safety(spack.paths.prefix)
+ spack.config.set('config:locks', False, scope='command_line')
+
if args.debug:
spack.util.debug.register_interrupt_handler()
spack.config.set('config:debug', True, scope='command_line')
diff --git a/lib/spack/spack/schema/config.py b/lib/spack/spack/schema/config.py
index d38e31cfd2..4a5b25ed63 100644
--- a/lib/spack/spack/schema/config.py
+++ b/lib/spack/spack/schema/config.py
@@ -66,6 +66,7 @@
'verify_ssl': {'type': 'boolean'},
'debug': {'type': 'boolean'},
'checksum': {'type': 'boolean'},
+ 'locks': {'type': 'boolean'},
'dirty': {'type': 'boolean'},
'build_jobs': {'type': 'integer', 'minimum': 1},
}
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 0c460ddf5f..0c441b3269 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -34,7 +34,6 @@
from six.moves.urllib.parse import urljoin
import llnl.util.tty as tty
-import llnl.util.lock
from llnl.util.filesystem import mkdirp, can_access
from llnl.util.filesystem import remove_if_dead_link, remove_linked_tree
@@ -42,6 +41,7 @@
import spack.caches
import spack.config
import spack.error
+import spack.util.lock
import spack.fetch_strategy as fs
import spack.util.pattern as pattern
from spack.util.path import canonicalize_path
@@ -231,7 +231,7 @@ def __init__(
lock_id = prefix_bits(sha1, bit_length(sys.maxsize))
stage_lock_path = os.path.join(spack.paths.stage_path, '.lock')
- Stage.stage_locks[self.name] = llnl.util.lock.Lock(
+ Stage.stage_locks[self.name] = spack.util.lock.Lock(
stage_lock_path, lock_id, 1)
self._lock = Stage.stage_locks[self.name]
diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py
index c84573e75e..c279d7e918 100644
--- a/lib/spack/spack/test/llnl/util/lock.py
+++ b/lib/spack/spack/test/llnl/util/lock.py
@@ -73,8 +73,11 @@
import pytest
from llnl.util.filesystem import touch
+
+import spack.util.lock
+from spack.util.executable import which
from spack.util.multiproc import Barrier
-from llnl.util.lock import Lock, WriteTransaction, ReadTransaction, LockError
+from spack.util.lock import Lock, WriteTransaction, ReadTransaction, LockError
#
@@ -183,15 +186,23 @@ def private_lock_path(lock_dir):
lock_file = os.path.join(lock_dir, 'lockfile')
if mpi:
lock_file += '.%s' % comm.rank
+
yield lock_file
+ if os.path.exists(lock_file):
+ os.unlink(lock_file)
+
@pytest.fixture
def lock_path(lock_dir):
"""This lock is shared among all processes in a multiproc test."""
lock_file = os.path.join(lock_dir, 'lockfile')
+
yield lock_file
+ if os.path.exists(lock_file):
+ os.unlink(lock_file)
+
def local_multiproc_test(*functions):
"""Order some processes using simple barrier synchronization."""
@@ -900,3 +911,95 @@ def do_write_with_exception(exit_fn):
assert vals['exception']
assert not vals['exited_fn']
assert not vals['exception_fn']
+
+
+def test_disable_locking(private_lock_path):
+ """Ensure that locks do no real locking when disabled."""
+ old_value = spack.config.get('config:locks')
+
+ with spack.config.override('config:locks', False):
+ lock = Lock(private_lock_path)
+
+ lock.acquire_read()
+ assert not os.path.exists(private_lock_path)
+
+ lock.acquire_write()
+ assert not os.path.exists(private_lock_path)
+
+ lock.release_write()
+ assert not os.path.exists(private_lock_path)
+
+ lock.release_read()
+ assert not os.path.exists(private_lock_path)
+
+ assert old_value == spack.config.get('config:locks')
+
+
+def test_lock_checks_user(tmpdir):
+ """Ensure lock checks work."""
+ path = str(tmpdir)
+ uid = os.getuid()
+
+ # self-owned, own group
+ tmpdir.chown(uid, uid)
+
+ # safe
+ tmpdir.chmod(0o744)
+ spack.util.lock.check_lock_safety(path)
+
+ # safe
+ tmpdir.chmod(0o774)
+ spack.util.lock.check_lock_safety(path)
+
+ # unsafe
+ tmpdir.chmod(0o777)
+ with pytest.raises(spack.error.SpackError):
+ spack.util.lock.check_lock_safety(path)
+
+ # safe
+ tmpdir.chmod(0o474)
+ spack.util.lock.check_lock_safety(path)
+
+ # safe
+ tmpdir.chmod(0o477)
+ spack.util.lock.check_lock_safety(path)
+
+
+def test_lock_checks_group(tmpdir):
+ path = str(tmpdir)
+ uid = os.getuid()
+
+ id_cmd = which('id')
+ if not id_cmd:
+ pytest.skip("Can't determine user's groups.")
+
+ # find a legal gid to user that is NOT the user's uid
+ gids = [int(gid) for gid in id_cmd('-G', output=str).split(' ')]
+ gid = next((g for g in gids if g != uid), None)
+ if gid is None:
+ pytest.skip("Can't determine user's groups.")
+
+ # self-owned, another group
+ tmpdir.chown(uid, gid)
+
+ # safe
+ tmpdir.chmod(0o744)
+ spack.util.lock.check_lock_safety(path)
+
+ # unsafe
+ tmpdir.chmod(0o774)
+ with pytest.raises(spack.error.SpackError):
+ spack.util.lock.check_lock_safety(path)
+
+ # unsafe
+ tmpdir.chmod(0o777)
+ with pytest.raises(spack.error.SpackError):
+ spack.util.lock.check_lock_safety(path)
+
+ # safe
+ tmpdir.chmod(0o474)
+ spack.util.lock.check_lock_safety(path)
+
+ # safe
+ tmpdir.chmod(0o477)
+ spack.util.lock.check_lock_safety(path)
diff --git a/lib/spack/spack/util/file_cache.py b/lib/spack/spack/util/file_cache.py
index d0551faff9..56217e3a7c 100644
--- a/lib/spack/spack/util/file_cache.py
+++ b/lib/spack/spack/util/file_cache.py
@@ -26,9 +26,9 @@
import shutil
from llnl.util.filesystem import mkdirp
-from llnl.util.lock import Lock, ReadTransaction, WriteTransaction
from spack.error import SpackError
+from spack.util.lock import Lock, ReadTransaction, WriteTransaction
class FileCache(object):
diff --git a/lib/spack/spack/util/lock.py b/lib/spack/spack/util/lock.py
new file mode 100644
index 0000000000..a36cf5876d
--- /dev/null
+++ b/lib/spack/spack/util/lock.py
@@ -0,0 +1,91 @@
+##############################################################################
+# Copyright (c) 2013-2018, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/spack/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+"""Wrapper for ``llnl.util.lock`` allows locking to be enabled/disabled."""
+import os
+import stat
+
+import llnl.util.lock
+from llnl.util.lock import * # noqa
+
+import spack.config
+import spack.error
+import spack.paths
+
+
+class Lock(llnl.util.lock.Lock):
+ """Lock that can be disabled.
+
+ This overrides the ``_lock()`` and ``_unlock()`` methods from
+ ``llnl.util.lock`` so that all the lock API calls will succeed, but
+ the actual locking mechanism can be disabled via ``_enable_locks``.
+ """
+ def __init__(self, *args, **kwargs):
+ super(Lock, self).__init__(*args, **kwargs)
+ self._enable = spack.config.get('config:locks', True)
+
+ def _lock(self, op, timeout=0):
+ if self._enable:
+ super(Lock, self)._lock(op, timeout)
+
+ def _unlock(self):
+ """Unlock call that always succeeds."""
+ if self._enable:
+ super(Lock, self)._unlock()
+
+ def _debug(self, *args):
+ if self._enable:
+ super(Lock, self)._debug(*args)
+
+
+def check_lock_safety(path):
+ """Do some extra checks to ensure disabling locks is safe.
+
+ This will raise an error if ``path`` can is group- or world-writable
+ AND the current user can write to the directory (i.e., if this user
+ AND others could write to the path).
+
+ This is intended to run on the Spack prefix, but can be run on any
+ path for testing.
+ """
+ if os.access(path, os.W_OK):
+ stat_result = os.stat(path)
+ uid, gid = stat_result.st_uid, stat_result.st_gid
+ mode = stat_result[stat.ST_MODE]
+
+ writable = None
+ if (mode & stat.S_IWGRP) and (uid != gid):
+ # spack is group-writeable and the group is not the owner
+ writable = 'group'
+ elif (mode & stat.S_IWOTH):
+ # spack is world-writeable
+ writable = 'world'
+
+ if writable:
+ msg = "Refusing to disable locks: spack is {0}-writable.".format(
+ writable)
+ long_msg = (
+ "Running a shared spack without locks is unsafe. You must "
+ "restrict permissions on {0} or enable locks.").format(path)
+ raise spack.error.SpackError(msg, long_msg)
--
GitLab