env views: make view updates atomic (#23476)

Currently, environment views blink out of existence during the view regeneration, and are slowly built back up to their new and improved state. This is not good if other processes attempt to access the view -- they can see it in an inconsistent state.

This PR fixes makes environment view updates atomic. This requires a level of indirection (via symlink, similar to nix or guix) from the view root to the underlying implementation on the filesystem. 

Now, an environment view at `/path/to/foo` is a symlink to `/path/to/._foo/<hash>`, where `<hash>` is a hash of the contents of the view.  We construct the view in its content-keyed hash directory, create a new symlink to this directory, and atomically replace the symlink with one to the new view.

This PR has a couple of other benefits:
* It future-proofs environment views so that we can implement rollback.
* It ensures that we don't leave users in an inconsistent state if building a new view fails for some reason.

For background:
* there is no atomic operation in posix that allows for a non-empty directory to be replaced.
* There is an atomic `renameat2` in the linux kernel starting in version 3.15, but many filesystems don't support the system call, including NFS3 and NFS4, which makes it a poor implementation choice for an HPC tool, so we use the symlink approach that others tools like nix and guix have used successfully.

6 files changed, 172 insertions, 72 deletions

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index 14e349ff11..fbdcb67761 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -2,7 +2,6 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 import collections
 import os
 import re
@@ -34,6 +33,7 @@ import spack.util.environment
 from spack.spec import Spec
 from spack.spec_list import SpecList, InvalidSpecConstraintError
 from spack.variant import UnknownVariantError
+import spack.util.hash
 import spack.util.lock as lk
 from spack.util.path import substitute_path_variables
 import spack.util.path
@@ -472,9 +472,11 @@ class ViewDescriptor(object):
                     self.link == other.link])
 
     def to_dict(self):
-        ret = {'root': self.root}
+        ret = syaml.syaml_dict([('root', self.root)])
         if self.projections:
-            ret['projections'] = self.projections
+            projections_dict = syaml.syaml_dict(
+                sorted(self.projections.items()))
+            ret['projections'] = projections_dict
         if self.select:
             ret['select'] = self.select
         if self.exclude:
@@ -492,10 +494,66 @@ class ViewDescriptor(object):
                               d.get('exclude', []),
                               d.get('link', default_view_link))
 
-    def view(self):
-        root = self.root
-        if not os.path.isabs(root):
-            root = os.path.normpath(os.path.join(self.base, self.root))
+    @property
+    def _current_root(self):
+        if not os.path.exists(self.root):
+            return None
+
+        root = os.readlink(self.root)
+        if os.path.isabs(root):
+            return root
+
+        root_dir = os.path.dirname(self.root)
+        return os.path.join(root_dir, root)
+
+    def _next_root(self, specs):
+        content_hash = self.content_hash(specs)
+        root_dir = os.path.dirname(self.root)
+        root_name = os.path.basename(self.root)
+        return os.path.join(root_dir, '._%s' % root_name, content_hash)
+
+    def content_hash(self, specs):
+        d = syaml.syaml_dict([
+            ('descriptor', self.to_dict()),
+            ('specs', [(spec.full_hash(), spec.prefix) for spec in sorted(specs)])
+        ])
+        contents = sjson.dump(d)
+        return spack.util.hash.b32_hash(contents)
+
+    def get_projection_for_spec(self, spec):
+        """Get projection for spec relative to view root
+
+        Getting the projection from the underlying root will get the temporary
+        projection. This gives the permanent projection relative to the root
+        symlink.
+        """
+        view = self.view()
+        view_path = view.get_projection_for_spec(spec)
+        rel_path = os.path.relpath(view_path, self._current_root)
+        return os.path.join(self.root, rel_path)
+
+    def view(self, new=None):
+        """
+        Generate the FilesystemView object for this ViewDescriptor
+
+        By default, this method returns a FilesystemView object rooted at the
+        current underlying root of this ViewDescriptor (self._current_root)
+
+        Raise if new is None and there is no current view
+
+        Arguments:
+            new (string or None): If a string, create a FilesystemView
+                rooted at that path. Default None. This should only be used to
+                regenerate the view, and cannot be used to access specs.
+        """
+        root = self._current_root
+        if new:
+            root = new
+        if not root:
+            # This can only be hit if we write a future bug
+            msg = ("Attempting to get nonexistent view from environment. "
+                   "View root is at %s" % self.root)
+            raise SpackEnvironmentViewError(msg)
         return YamlFilesystemView(root, spack.store.layout,
                                   ignore_conflicts=True,
                                   projections=self.projections)
@@ -517,9 +575,10 @@ class ViewDescriptor(object):
 
         return True
 
-    def regenerate(self, all_specs, roots):
+    def specs_for_view(self, all_specs, roots):
         specs_for_view = []
         specs = all_specs if self.link == 'all' else roots
+
         for spec in specs:
             # The view does not store build deps, so if we want it to
             # recognize environment specs (which do store build deps),
@@ -531,6 +590,10 @@ class ViewDescriptor(object):
                 spec_copy._hash = spec._hash
                 spec_copy._normal = spec._normal
                 specs_for_view.append(spec_copy)
+        return specs_for_view
+
+    def regenerate(self, all_specs, roots):
+        specs_for_view = self.specs_for_view(all_specs, roots)
 
         # regeneration queries the database quite a bit; this read
         # transaction ensures that we don't repeatedly lock/unlock.
@@ -540,36 +603,52 @@ class ViewDescriptor(object):
 
             # To ensure there are no conflicts with packages being installed
             # that cannot be resolved or have repos that have been removed
-            # we always regenerate the view from scratch. We must first make
-            # sure the root directory exists for the very first time though.
-            root = os.path.normpath(
-                self.root if os.path.isabs(self.root) else os.path.join(
-                    self.base, self.root)
-            )
-            fs.mkdirp(root)
-
-            # The tempdir for the directory transaction must be in the same
-            # filesystem mount as the view for symlinks to work. Provide
-            # dirname(root) as the tempdir for the
-            # replace_directory_transaction because it must be on the same
-            # filesystem mount as the view itself. Otherwise it may be
-            # impossible to construct the view in the tempdir even when it can
-            # be constructed in-place.
-            with fs.replace_directory_transaction(root, os.path.dirname(root)):
-                view = self.view()
-
-                view.clean()
-                specs_in_view = set(view.get_all_specs())
-                tty.msg("Updating view at {0}".format(self.root))
-
-                rm_specs = specs_in_view - installed_specs_for_view
-                add_specs = installed_specs_for_view - specs_in_view
-
-                # pass all_specs in, as it's expensive to read all the
-                # spec.yaml files twice.
-                view.remove_specs(*rm_specs, with_dependents=False,
-                                  all_specs=specs_in_view)
-                view.add_specs(*add_specs, with_dependencies=False)
+            # we always regenerate the view from scratch.
+            # We will do this by hashing the view contents and putting the view
+            # in a directory by hash, and then having a symlink to the real
+            # view in the root. The real root for a view at /dirname/basename
+            # will be /dirname/._basename_<hash>.
+            # This allows for atomic swaps when we update the view
+
+            # cache the roots because the way we determine which is which does
+            # not work while we are updating
+            new_root = self._next_root(installed_specs_for_view)
+            old_root = self._current_root
+
+            if new_root == old_root:
+                tty.debug("View at %s does not need regeneration." % self.root)
+                return
+
+            # construct view at new_root
+            tty.msg("Updating view at {0}".format(self.root))
+
+            view = self.view(new=new_root)
+            fs.mkdirp(new_root)
+            view.add_specs(*installed_specs_for_view,
+                           with_dependencies=False)
+
+            # create symlink from tmpname to new_root
+            root_dirname = os.path.dirname(self.root)
+            tmp_symlink_name = os.path.join(root_dirname, '._view_link')
+            if os.path.exists(tmp_symlink_name):
+                os.unlink(tmp_symlink_name)
+            os.symlink(new_root, tmp_symlink_name)
+
+            # mv symlink atomically over root symlink to old_root
+            if os.path.exists(self.root) and not os.path.islink(self.root):
+                msg = "Cannot create view: "
+                msg += "file already exists and is not a link: %s" % self.root
+                raise SpackEnvironmentViewError(msg)
+            os.rename(tmp_symlink_name, self.root)
+
+            # remove old_root
+            if old_root and os.path.exists(old_root):
+                try:
+                    shutil.rmtree(old_root)
+                except (IOError, OSError) as e:
+                    msg = "Failed to remove old view at %s\n" % old_root
+                    msg += str(e)
+                    tty.warn(msg)
 
 
 class Environment(object):
@@ -2106,3 +2185,7 @@ def is_latest_format(manifest):
 
 class SpackEnvironmentError(spack.error.SpackError):
     """Superclass for all errors to do with Spack environments."""
+
+
+class SpackEnvironmentViewError(SpackEnvironmentError):
+    """Class for errors regarding view generation."""
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 71971453f2..54ae03092e 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -76,10 +76,8 @@ thing.  Spack uses ~variant in directory names and in the canonical form of
 specs to avoid ambiguity.  Both are provided because ~ can cause shell
 expansion when it is the first character in an id typed on the command line.
 """
-import base64
 import sys
 import collections
-import hashlib
 import itertools
 import operator
 import os
@@ -108,6 +106,7 @@ import spack.solver
 import spack.store
 import spack.util.crypto
 import spack.util.executable
+import spack.util.hash
 import spack.util.module_cmd as md
 import spack.util.prefix
 import spack.util.spack_json as sjson
@@ -1505,13 +1504,7 @@ class Spec(object):
         # this when we move to using package hashing on all specs.
         node_dict = self.to_node_dict(hash=hash)
         yaml_text = syaml.dump(node_dict, default_flow_style=True)
-        sha = hashlib.sha1(yaml_text.encode('utf-8'))
-        b32_hash = base64.b32encode(sha.digest()).lower()
-
-        if sys.version_info[0] >= 3:
-            b32_hash = b32_hash.decode('utf-8')
-
-        return b32_hash
+        return spack.util.hash.b32_hash(yaml_text)
 
     def _cached_hash(self, hash, length=None):
         """Helper function for storing a cached hash on the spec.
@@ -1567,7 +1560,7 @@ class Spec(object):
 
     def dag_hash_bit_prefix(self, bits):
         """Get the first <bits> bits of the DAG hash as an integer type."""
-        return base32_prefix_bits(self.dag_hash(), bits)
+        return spack.util.hash.base32_prefix_bits(self.dag_hash(), bits)
 
     def to_node_dict(self, hash=ht.dag_hash):
         """Create a dictionary representing the state of this Spec.
@@ -4764,16 +4757,6 @@ def save_dependency_spec_yamls(
             fd.write(dep_spec.to_yaml(hash=ht.build_hash))
 
 
-def base32_prefix_bits(hash_string, bits):
-    """Return the first <bits> bits of a base32 string as an integer."""
-    if bits > len(hash_string) * 5:
-        raise ValueError("Too many bits! Requested %d bit prefix of '%s'."
-                         % (bits, hash_string))
-
-    hash_bytes = base64.b32decode(hash_string, casefold=True)
-    return spack.util.crypto.prefix_bits(hash_bytes, bits)
-
-
 class SpecParseError(spack.error.SpecError):
     """Wrapper for ParseError for when we're parsing specs."""
     def __init__(self, parse_error):
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 9358d0a3a9..f6933dc349 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -9,6 +9,7 @@ from six import StringIO
 import pytest
 
 import llnl.util.filesystem as fs
+import llnl.util.link_tree
 
 import spack.hash_types as ht
 import spack.modules
@@ -1097,7 +1098,7 @@ def test_store_different_build_deps():
 
 def test_env_updates_view_install(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     with ev.read('test'):
         add('mpileaks')
@@ -1108,12 +1109,13 @@ def test_env_updates_view_install(
 
 def test_env_view_fails(
         tmpdir, mock_packages, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     with ev.read('test'):
         add('libelf')
         add('libelf cflags=-g')
-        with pytest.raises(RuntimeError, match='merge blocked by file'):
+        with pytest.raises(llnl.util.link_tree.MergeConflictError,
+                           match='merge blocked by file'):
             install('--fake')
 
 
@@ -1126,7 +1128,7 @@ def test_env_without_view_install(
     with pytest.raises(spack.environment.SpackEnvironmentError):
         test_env.default_view
 
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
 
     with ev.read('test'):
         add('mpileaks')
@@ -1161,7 +1163,7 @@ env:
 
 def test_env_updates_view_install_package(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     with ev.read('test'):
         install('--fake', 'mpileaks')
@@ -1171,7 +1173,7 @@ def test_env_updates_view_install_package(
 
 def test_env_updates_view_add_concretize(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     install('--fake', 'mpileaks')
     with ev.read('test'):
@@ -1183,7 +1185,7 @@ def test_env_updates_view_add_concretize(
 
 def test_env_updates_view_uninstall(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     with ev.read('test'):
         install('--fake', 'mpileaks')
@@ -1198,7 +1200,7 @@ def test_env_updates_view_uninstall(
 
 def test_env_updates_view_uninstall_referenced_elsewhere(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     install('--fake', 'mpileaks')
     with ev.read('test'):
@@ -1215,7 +1217,7 @@ def test_env_updates_view_uninstall_referenced_elsewhere(
 
 def test_env_updates_view_remove_concretize(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     install('--fake', 'mpileaks')
     with ev.read('test'):
@@ -1233,7 +1235,7 @@ def test_env_updates_view_remove_concretize(
 
 def test_env_updates_view_force_remove(
         tmpdir, mock_stage, mock_fetch, install_mockery):
-    view_dir = tmpdir.mkdir('view')
+    view_dir = tmpdir.join('view')
     env('create', '--with-view=%s' % view_dir, 'test')
     with ev.read('test'):
         install('--fake', 'mpileaks')
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index efee120ae5..ea16f655db 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -13,6 +13,7 @@ import spack.package
 from spack.spec import Spec
 from spack.dependency import all_deptypes, Dependency, canonical_deptype
 from spack.util.mock_package import MockPackageMultiRepo
+import spack.util.hash as hashutil
 
 
 def check_links(spec_to_check):
@@ -705,17 +706,17 @@ class TestSpecDag(object):
                                 for c in test_hash])
 
             for bits in (1, 2, 3, 4, 7, 8, 9, 16, 64, 117, 128, 160):
-                actual_int = spack.spec.base32_prefix_bits(test_hash, bits)
+                actual_int = hashutil.base32_prefix_bits(test_hash, bits)
                 fmt = "#0%sb" % (bits + 2)
                 actual = format(actual_int, fmt).replace('0b', '')
 
                 assert expected[:bits] == actual
 
             with pytest.raises(ValueError):
-                spack.spec.base32_prefix_bits(test_hash, 161)
+                hashutil.base32_prefix_bits(test_hash, 161)
 
             with pytest.raises(ValueError):
-                spack.spec.base32_prefix_bits(test_hash, 256)
+                hashutil.base32_prefix_bits(test_hash, 256)
 
     def test_traversal_directions(self):
         """Make sure child and parent traversals of specs work."""
diff --git a/lib/spack/spack/user_environment.py b/lib/spack/spack/user_environment.py
index f111aaedc3..7dd63dcdea 100644
--- a/lib/spack/spack/user_environment.py
+++ b/lib/spack/spack/user_environment.py
@@ -72,7 +72,7 @@ def environment_modifications_for_spec(spec, view=None):
     the view."""
     spec = spec.copy()
     if view and not spec.external:
-        spec.prefix = prefix.Prefix(view.view().get_projection_for_spec(spec))
+        spec.prefix = prefix.Prefix(view.get_projection_for_spec(spec))
 
     # generic environment modifications determined by inspecting the spec
     # prefix
diff --git a/lib/spack/spack/util/hash.py b/lib/spack/spack/util/hash.py
new file mode 100644
index 0000000000..da81284ea6
--- /dev/null
+++ b/lib/spack/spack/util/hash.py
@@ -0,0 +1,31 @@
+# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import base64
+import hashlib
+import sys
+
+import spack.util.crypto
+
+
+def b32_hash(content):
+    """Return the b32 encoded sha1 hash of the input string as a string."""
+    sha = hashlib.sha1(content.encode('utf-8'))
+    b32_hash = base64.b32encode(sha.digest()).lower()
+
+    if sys.version_info[0] >= 3:
+        b32_hash = b32_hash.decode('utf-8')
+
+    return b32_hash
+
+
+def base32_prefix_bits(hash_string, bits):
+    """Return the first <bits> bits of a base32 string as an integer."""
+    if bits > len(hash_string) * 5:
+        raise ValueError("Too many bits! Requested %d bit prefix of '%s'."
+                         % (bits, hash_string))
+
+    hash_bytes = base64.b32decode(hash_string, casefold=True)
+    return spack.util.crypto.prefix_bits(hash_bytes, bits)