features: Add install failure tracking removal through `spack clean` (#15314)

* Add ability to force removal of install failure tracking data through spack clean

* Add clean failures option to packaging guide

9 files changed, 141 insertions, 37 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index 1f246c0faa..533036852b 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -4252,23 +4252,29 @@ Does this in one of two ways:
 ``spack clean``
 ^^^^^^^^^^^^^^^
 
-Cleans up all of Spack's temporary and cached files.  This can be used to
+Cleans up Spack's temporary and cached files.  This command can be used to
 recover disk space if temporary files from interrupted or failed installs
-accumulate in the staging area.
+accumulate.
 
 When called with ``--stage`` or without arguments this removes all staged
 files.
 
-When called with ``--downloads`` this will clear all resources
-:ref:`cached <caching>` during installs.
+The ``--downloads`` option removes cached :ref:`cached <caching>` downloads.
 
-When called with ``--user-cache`` this will remove caches in the user home
-directory, including cached virtual indices.
+You can force the removal of all install failure tracking markers using the
+``--failures`` option.  Note that ``spack install`` will automatically clear
+relevant failure markings prior to performing the requested installation(s).
+
+Long-lived caches, like the virtual package index, are removed using the
+``--misc-cache`` option.
+
+The ``--python-cache`` option removes `.pyc`, `.pyo`, and `__pycache__`
+folders.
 
 To remove all of the above, the command can be called with ``--all``.
 
-When called with positional arguments, cleans up temporary files only
-for a particular package. If ``fetch``, ``stage``, or ``install``
+When called with positional arguments, this command cleans up temporary files
+only for a particular package. If ``fetch``, ``stage``, or ``install``
 are run again after this, Spack's build process will start from scratch.
 
 
diff --git a/lib/spack/spack/cmd/clean.py b/lib/spack/spack/cmd/clean.py
index 791a1b7dc3..d847e7a7c0 100644
--- a/lib/spack/spack/cmd/clean.py
+++ b/lib/spack/spack/cmd/clean.py
@@ -23,9 +23,9 @@ level = "long"
 
 
 class AllClean(argparse.Action):
-    """Activates flags -s -d -m and -p simultaneously"""
+    """Activates flags -s -d -f -m and -p simultaneously"""
     def __call__(self, parser, namespace, values, option_string=None):
-        parser.parse_args(['-sdmp'], namespace=namespace)
+        parser.parse_args(['-sdfmp'], namespace=namespace)
 
 
 def setup_parser(subparser):
@@ -36,21 +36,24 @@ def setup_parser(subparser):
         '-d', '--downloads', action='store_true',
         help="remove cached downloads")
     subparser.add_argument(
+        '-f', '--failures', action='store_true',
+        help="force removal of all install failure tracking markers")
+    subparser.add_argument(
         '-m', '--misc-cache', action='store_true',
         help="remove long-lived caches, like the virtual package index")
     subparser.add_argument(
         '-p', '--python-cache', action='store_true',
         help="remove .pyc, .pyo files and __pycache__ folders")
     subparser.add_argument(
-        '-a', '--all', action=AllClean, help="equivalent to -sdmp", nargs=0
+        '-a', '--all', action=AllClean, help="equivalent to -sdfmp", nargs=0
     )
     arguments.add_common_arguments(subparser, ['specs'])
 
 
 def clean(parser, args):
     # If nothing was set, activate the default
-    if not any([args.specs, args.stage, args.downloads, args.misc_cache,
-                args.python_cache]):
+    if not any([args.specs, args.stage, args.downloads, args.failures,
+                args.misc_cache, args.python_cache]):
         args.stage = True
 
     # Then do the cleaning falling through the cases
@@ -70,6 +73,10 @@ def clean(parser, args):
         tty.msg('Removing cached downloads')
         spack.caches.fetch_cache.destroy()
 
+    if args.failures:
+        tty.msg('Removing install failure marks')
+        spack.installer.clear_failures()
+
     if args.misc_cache:
         tty.msg('Removing cached information on repositories')
         spack.caches.misc_cache.destroy()
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index 001f96d1ff..2acc8c2db6 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -23,6 +23,7 @@ filesystem.
 import contextlib
 import datetime
 import os
+import six
 import socket
 import sys
 import time
@@ -33,14 +34,14 @@ except ImportError:
     _use_uuid = False
     pass
 
+import llnl.util.filesystem as fs
 import llnl.util.tty as tty
-import six
+
 import spack.repo
 import spack.spec
 import spack.store
 import spack.util.lock as lk
 import spack.util.spack_json as sjson
-from llnl.util.filesystem import mkdirp
 from spack.directory_layout import DirectoryLayoutError
 from spack.error import SpackError
 from spack.filesystem_view import YamlFilesystemView
@@ -316,10 +317,10 @@ class Database(object):
 
         # Create needed directories and files
         if not os.path.exists(self._db_dir):
-            mkdirp(self._db_dir)
+            fs.mkdirp(self._db_dir)
 
         if not os.path.exists(self._failure_dir) and not is_upstream:
-            mkdirp(self._failure_dir)
+            fs.mkdirp(self._failure_dir)
 
         self.is_upstream = is_upstream
         self.last_seen_verifier = ''
@@ -373,6 +374,23 @@ class Database(object):
         return os.path.join(self._failure_dir,
                             '{0}-{1}'.format(spec.name, spec.full_hash()))
 
+    def clear_all_failures(self):
+        """Force remove install failure tracking files."""
+        tty.debug('Releasing prefix failure locks')
+        for pkg_id in list(self._prefix_failures.keys()):
+            lock = self._prefix_failures.pop(pkg_id, None)
+            if lock:
+                lock.release_write()
+
+        # Remove all failure markings (aka files)
+        tty.debug('Removing prefix failure tracking files')
+        for fail_mark in os.listdir(self._failure_dir):
+            try:
+                os.remove(os.path.join(self._failure_dir, fail_mark))
+            except OSError as exc:
+                tty.warn('Unable to remove failure marking file {0}: {1}'
+                         .format(fail_mark, str(exc)))
+
     def clear_failure(self, spec, force=False):
         """
         Remove any persistent and cached failure tracking for the spec.
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index cd2aa00bd0..a2b0220a8b 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -373,6 +373,13 @@ def _update_explicit_entry_in_db(pkg, rec, explicit):
             rec.explicit = True
 
 
+def clear_failures():
+    """
+    Remove all failure tracking markers for the Spack instance.
+    """
+    spack.store.db.clear_all_failures()
+
+
 def dump_packages(spec, path):
     """
     Dump all package information for a spec and its dependencies.
@@ -835,7 +842,7 @@ class PackageInstaller(object):
         """
         lock = self.failed.get(pkg_id, None)
         if lock is not None:
-            err = "{0} exception when removing failure mark for {1}: {2}"
+            err = "{0} exception when removing failure tracking for {1}: {2}"
             msg = 'Removing failure mark on {0}'
             try:
                 tty.verbose(msg.format(pkg_id))
diff --git a/lib/spack/spack/test/cmd/clean.py b/lib/spack/spack/test/cmd/clean.py
index c3e9d79f23..4df708cf50 100644
--- a/lib/spack/spack/test/cmd/clean.py
+++ b/lib/spack/spack/test/cmd/clean.py
@@ -28,18 +28,21 @@ def mock_calls_for_clean(monkeypatch):
         spack.caches.fetch_cache, 'destroy', Counter(), raising=False)
     monkeypatch.setattr(
         spack.caches.misc_cache, 'destroy', Counter())
+    monkeypatch.setattr(
+        spack.installer, 'clear_failures', Counter())
 
 
 @pytest.mark.usefixtures(
     'mock_packages', 'config', 'mock_calls_for_clean'
 )
 @pytest.mark.parametrize('command_line,counters', [
-    ('mpileaks', [1, 0, 0, 0]),
-    ('-s',       [0, 1, 0, 0]),
-    ('-sd',      [0, 1, 1, 0]),
-    ('-m',       [0, 0, 0, 1]),
-    ('-a',       [0, 1, 1, 1]),
-    ('',         [0, 0, 0, 0]),
+    ('mpileaks', [1, 0, 0, 0, 0]),
+    ('-s',       [0, 1, 0, 0, 0]),
+    ('-sd',      [0, 1, 1, 0, 0]),
+    ('-m',       [0, 0, 0, 1, 0]),
+    ('-f',       [0, 0, 0, 0, 1]),
+    ('-a',       [0, 1, 1, 1, 1]),
+    ('',         [0, 0, 0, 0, 0]),
 ])
 def test_function_calls(command_line, counters):
 
@@ -52,3 +55,4 @@ def test_function_calls(command_line, counters):
     assert spack.stage.purge.call_count == counters[1]
     assert spack.caches.fetch_cache.destroy.call_count == counters[2]
     assert spack.caches.misc_cache.destroy.call_count == counters[3]
+    assert spack.installer.clear_failures.call_count == counters[4]
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 4a5db55c01..e4df22a6a5 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -610,17 +610,17 @@ def test_build_warning_output(tmpdir, mock_fetch, install_mockery, capfd):
         assert 'foo.c:89: warning: some weird warning!' in msg
 
 
-def test_cache_only_fails(tmpdir, mock_fetch, install_mockery, capfd):
-    msg = ''
-    with capfd.disabled():
-        try:
-            install('--cache-only', 'libdwarf')
-        except spack.installer.InstallError as e:
-            msg = str(e)
-
-    # libelf from cache failed to install, which automatically removed the
-    # the libdwarf build task and flagged the package as failed to install.
-    assert 'Installation of libdwarf failed' in msg
+def test_cache_only_fails(tmpdir, mock_fetch, install_mockery):
+    # libelf from cache fails to install, which automatically removes the
+    # the libdwarf build task and flags the package as failed to install.
+    err_msg = 'Installation of libdwarf failed'
+    with pytest.raises(spack.installer.InstallError, match=err_msg):
+        install('--cache-only', 'libdwarf')
+
+    # Check that failure prefix locks are still cached
+    failure_lock_prefixes = ','.join(spack.store.db._prefix_failures.keys())
+    assert 'libelf' in failure_lock_prefixes
+    assert 'libdwarf' in failure_lock_prefixes
 
 
 def test_install_only_dependencies(tmpdir, mock_fetch, install_mockery):
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index bac5bcebf6..8bd8deadce 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -601,6 +601,16 @@ def install_mockery(tmpdir, config, mock_packages, monkeypatch):
     tmpdir.join('opt').remove()
     spack.store.store = real_store
 
+    # Also wipe out any cached prefix failure locks (associated with
+    # the session-scoped mock archive).
+    for pkg_id in list(spack.store.db._prefix_failures.keys()):
+        lock = spack.store.db._prefix_failures.pop(pkg_id, None)
+        if lock:
+            try:
+                lock.release_write()
+            except Exception:
+                pass
+
 
 @pytest.fixture(scope='function')
 def install_mockery_mutable_config(
diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py
index 89efbe4fbe..cc4b168e6c 100644
--- a/lib/spack/spack/test/installer.py
+++ b/lib/spack/spack/test/installer.py
@@ -462,6 +462,58 @@ def test_dump_packages_deps_errs(install_mockery, tmpdir, monkeypatch, capsys):
     assert "Couldn't copy in provenance for cmake" in out
 
 
+def test_clear_failures_success(install_mockery):
+    """Test the clear_failures happy path."""
+
+    # Set up a test prefix failure lock
+    lock = lk.Lock(spack.store.db.prefix_fail_path, start=1, length=1,
+                   default_timeout=1e-9, desc='test')
+    try:
+        lock.acquire_write()
+    except lk.LockTimeoutError:
+        tty.warn('Failed to write lock the test install failure')
+    spack.store.db._prefix_failures['test'] = lock
+
+    # Set up a fake failure mark (or file)
+    fs.touch(os.path.join(spack.store.db._failure_dir, 'test'))
+
+    # Now clear failure tracking
+    inst.clear_failures()
+
+    # Ensure there are no cached failure locks or failure marks
+    assert len(spack.store.db._prefix_failures) == 0
+    assert len(os.listdir(spack.store.db._failure_dir)) == 0
+
+    # Ensure the core directory and failure lock file still exist
+    assert os.path.isdir(spack.store.db._failure_dir)
+    assert os.path.isfile(spack.store.db.prefix_fail_path)
+
+
+def test_clear_failures_errs(install_mockery, monkeypatch, capsys):
+    """Test the clear_failures exception paths."""
+    orig_fn = os.remove
+    err_msg = 'Mock os remove'
+
+    def _raise_except(path):
+        raise OSError(err_msg)
+
+    # Set up a fake failure mark (or file)
+    fs.touch(os.path.join(spack.store.db._failure_dir, 'test'))
+
+    monkeypatch.setattr(os, 'remove', _raise_except)
+
+    # Clear failure tracking
+    inst.clear_failures()
+
+    # Ensure expected warning generated
+    out = str(capsys.readouterr()[1])
+    assert 'Unable to remove failure' in out
+    assert err_msg in out
+
+    # Restore remove for teardown
+    monkeypatch.setattr(os, 'remove', orig_fn)
+
+
 def test_check_deps_status_install_failure(install_mockery, monkeypatch):
     spec, installer = create_installer('a')
 
@@ -669,7 +721,7 @@ def test_cleanup_failed_err(install_mockery, tmpdir, monkeypatch, capsys):
 
         installer._cleanup_failed(pkg_id)
         out = str(capsys.readouterr()[1])
-        assert 'exception when removing failure mark' in out
+        assert 'exception when removing failure tracking' in out
         assert msg in out
 
 
diff --git a/share/spack/spack-completion.bash b/share/spack/spack-completion.bash
index 620dd9bef2..e2db47ba30 100755
--- a/share/spack/spack-completion.bash
+++ b/share/spack/spack-completion.bash
@@ -484,7 +484,7 @@ _spack_ci_rebuild() {
 _spack_clean() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -s --stage -d --downloads -m --misc-cache -p --python-cache -a --all"
+        SPACK_COMPREPLY="-h --help -s --stage -d --downloads -f --failures -m --misc-cache -p --python-cache -a --all"
     else
         _all_packages
     fi