zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

env views: make view updates atomic (#23476)

Browse Source 
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.
This commit is contained in:
Greg Becker 2021-05-12 23:56:20 -07:00 committed by GitHub
parent ee73f75239
commit f8740c8c75
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 168 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

151
lib/spack/spack/environment.py
View File

@ -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 @@
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 @@ def __eq__(self, other):
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 @@ def from_dict(base_path, d):
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 @@ def __contains__(self, spec):
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 @@ def regenerate(self, all_specs, roots):
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 @@ def regenerate(self, all_specs, roots):
# 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)
# 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
# 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()
# 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
view.clean()
specs_in_view = set(view.get_all_specs())
tty.msg("Updating view at {0}".format(self.root))
if new_root == old_root:
tty.debug("View at %s does not need regeneration." % self.root)
return
rm_specs = specs_in_view - installed_specs_for_view
add_specs = installed_specs_for_view - specs_in_view
# construct view at new_root
tty.msg("Updating view at {0}".format(self.root))
# 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)
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."""

23
lib/spack/spack/spec.py
View File

@ -76,10 +76,8 @@
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.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 @@ def _spec_hash(self, hash):
# 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 @@ def full_hash(self, length=None):
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):

22
lib/spack/spack/test/cmd/env.py
View File

@ -9,6 +9,7 @@
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 noop(*args):
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 @@ def test_env_config_view_default(
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')

7
lib/spack/spack/test/spec_dag.py
View File

@ -13,6 +13,7 @@
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 @@ def test_hash_bits(self):
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."""

2
lib/spack/spack/user_environment.py
View File

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

31
lib/spack/spack/util/hash.py Normal file
View File

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