From 48d3e8d3503e8c64048f158e1c2c5206cdefd9c6 Mon Sep 17 00:00:00 2001 From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com> Date: Wed, 24 Jun 2020 18:28:53 -0700 Subject: 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 --- lib/spack/docs/packaging_guide.rst | 22 +++++++++------ lib/spack/spack/cmd/clean.py | 17 ++++++++---- lib/spack/spack/database.py | 26 +++++++++++++++--- lib/spack/spack/installer.py | 9 ++++++- lib/spack/spack/test/cmd/clean.py | 16 ++++++----- lib/spack/spack/test/cmd/install.py | 22 +++++++-------- lib/spack/spack/test/conftest.py | 10 +++++++ lib/spack/spack/test/installer.py | 54 ++++++++++++++++++++++++++++++++++++- 8 files changed, 140 insertions(+), 36 deletions(-) (limited to 'lib') 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 ` during installs. +The ``--downloads`` option removes cached :ref:`cached ` 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): @@ -35,6 +35,9 @@ def setup_parser(subparser): subparser.add_argument( '-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") @@ -42,15 +45,15 @@ def setup_parser(subparser): '-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 -- cgit v1.2.3-70-g09d2