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

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

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:
 

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

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.

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

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

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},
             }

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]

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)

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

lib/spack/spack/util/lock.py

+##############################################################################
+# 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)