diff options
| -rw-r--r-- | lib/spack/llnl/util/filesystem.py | 40 | ||||
| -rw-r--r-- | lib/spack/spack/installer.py | 100 | ||||
| -rw-r--r-- | lib/spack/spack/test/installer.py | 95 |
3 files changed, 200 insertions, 35 deletions
diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index e44f8c608f..f3c2ee5ab1 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -656,6 +656,12 @@ def working_dir(dirname, **kwargs): os.chdir(orig_dir) +class CouldNotRestoreDirectoryBackup(RuntimeError): + def __init__(self, inner_exception, outer_exception): + self.inner_exception = inner_exception + self.outer_exception = outer_exception + + @contextmanager def replace_directory_transaction(directory_name, tmp_root=None): """Moves a directory to a temporary space. If the operations executed @@ -683,29 +689,33 @@ def replace_directory_transaction(directory_name, tmp_root=None): assert os.path.isabs(tmp_root) tmp_dir = tempfile.mkdtemp(dir=tmp_root) - tty.debug('TEMPORARY DIRECTORY CREATED [{0}]'.format(tmp_dir)) + tty.debug('Temporary directory created [{0}]'.format(tmp_dir)) shutil.move(src=directory_name, dst=tmp_dir) - tty.debug('DIRECTORY MOVED [src={0}, dest={1}]'.format( - directory_name, tmp_dir - )) + tty.debug('Directory moved [src={0}, dest={1}]'.format(directory_name, tmp_dir)) try: yield tmp_dir - except (Exception, KeyboardInterrupt, SystemExit): - # Delete what was there, before copying back the original content - if os.path.exists(directory_name): - shutil.rmtree(directory_name) - shutil.move( - src=os.path.join(tmp_dir, directory_basename), - dst=os.path.dirname(directory_name) - ) - tty.debug('DIRECTORY RECOVERED [{0}]'.format(directory_name)) + except (Exception, KeyboardInterrupt, SystemExit) as inner_exception: + # Try to recover the original directory, if this fails, raise a + # composite exception. + try: + # Delete what was there, before copying back the original content + if os.path.exists(directory_name): + shutil.rmtree(directory_name) + shutil.move( + src=os.path.join(tmp_dir, directory_basename), + dst=os.path.dirname(directory_name) + ) + except Exception as outer_exception: + raise CouldNotRestoreDirectoryBackup(inner_exception, outer_exception) + + tty.debug('Directory recovered [{0}]'.format(directory_name)) raise else: # Otherwise delete the temporary directory - shutil.rmtree(tmp_dir) - tty.debug('TEMPORARY DIRECTORY DELETED [{0}]'.format(tmp_dir)) + shutil.rmtree(tmp_dir, ignore_errors=True) + tty.debug('Temporary directory deleted [{0}]'.format(tmp_dir)) def hash_directory(directory, ignore=[]): diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index d6a0bcabd7..2f1544ee04 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -85,6 +85,15 @@ STATUS_DEQUEUED = 'dequeued' STATUS_REMOVED = 'removed' +class InstallAction(object): + #: Don't perform an install + NONE = 0 + #: Do a standard install + INSTALL = 1 + #: Do an overwrite install + OVERWRITE = 2 + + def _check_last_phase(pkg): """ Ensures the specified package has a valid last phase before proceeding @@ -1418,6 +1427,42 @@ class PackageInstaller(object): for dependent_id in dependents.difference(task.dependents): task.add_dependent(dependent_id) + def _install_action(self, task): + """ + Determine whether the installation should be overwritten (if it already + exists) or skipped (if has been handled by another process). + + If the package has not been installed yet, this will indicate that the + installation should proceed as normal (i.e. no need to transactionally + preserve the old prefix). + """ + # If we don't have to overwrite, do a normal install + if task.pkg.spec.dag_hash() not in task.request.overwrite: + return InstallAction.INSTALL + + # If it's not installed, do a normal install as well + rec, installed = self._check_db(task.pkg.spec) + if not installed: + return InstallAction.INSTALL + + # Ensure install_tree projections have not changed. + assert task.pkg.prefix == rec.path + + # If another process has overwritten this, we shouldn't install at all + if rec.installation_time >= task.request.overwrite_time: + return InstallAction.NONE + + # If the install prefix is missing, warn about it, and proceed with + # normal install. + if not os.path.exists(task.pkg.prefix): + tty.debug("Missing installation to overwrite") + return InstallAction.INSTALL + + # Otherwise, do an actual overwrite install. We backup the original + # install directory, put the old prefix + # back on failure + return InstallAction.OVERWRITE + def install(self): """ Install the requested package(s) and or associated dependencies. @@ -1561,26 +1606,12 @@ class PackageInstaller(object): # Proceed with the installation since we have an exclusive write # lock on the package. try: - if pkg.spec.dag_hash() in task.request.overwrite: - rec, _ = self._check_db(pkg.spec) - if rec and rec.installed: - if rec.installation_time < task.request.overwrite_time: - # If it's actually overwriting, do a fs transaction - if os.path.exists(rec.path): - with fs.replace_directory_transaction( - rec.path): - # fs transaction will put the old prefix - # back on failure, so make sure to keep it. - keep_prefix = True - self._install_task(task) - else: - tty.debug("Missing installation to overwrite") - self._install_task(task) - else: - # overwriting nothing - self._install_task(task) - else: + action = self._install_action(task) + + if action == InstallAction.INSTALL: self._install_task(task) + elif action == InstallAction.OVERWRITE: + OverwriteInstall(self, spack.store.db, task).install() self._update_installed(task) @@ -1631,7 +1662,7 @@ class PackageInstaller(object): finally: # Remove the install prefix if anything went wrong during # install. - if not keep_prefix: + if not keep_prefix and not action == InstallAction.OVERWRITE: pkg.remove_prefix() # The subprocess *may* have removed the build stage. Mark it @@ -1883,6 +1914,35 @@ def build_process(pkg, install_args): return installer.run() +class OverwriteInstall(object): + def __init__(self, installer, database, task, tmp_root=None): + self.installer = installer + self.database = database + self.task = task + self.tmp_root = tmp_root + + def install(self): + """ + Try to run the install task overwriting the package prefix. + If this fails, try to recover the original install prefix. If that fails + too, mark the spec as uninstalled. This function always the original + install error if installation fails. + """ + try: + with fs.replace_directory_transaction(self.task.pkg.prefix, self.tmp_root): + self.installer._install_task(self.task) + except fs.CouldNotRestoreDirectoryBackup as e: + self.database.remove(self.task.pkg.spec) + tty.error('Recovery of install dir of {0} failed due to ' + '{1}: {2}. The spec is now uninstalled.'.format( + self.task.pkg.name, + e.outer_exception.__class__.__name__, + str(e.outer_exception))) + + # Unwrap the actual installation exception. + raise e.inner_exception + + class BuildTask(object): """Class for representing the build task for a package.""" diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py index 37ce6f5bdb..4f7f24dc24 100644 --- a/lib/spack/spack/test/installer.py +++ b/lib/spack/spack/test/installer.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import os +import shutil import py import pytest @@ -1177,3 +1178,97 @@ def test_install_skip_patch(install_mockery, mock_fetch): spec, install_args = const_arg[0] assert inst.package_id(spec.package) in installer.installed + + +def test_overwrite_install_backup_success(temporary_store, config, mock_packages, + tmpdir): + """ + When doing an overwrite install that fails, Spack should restore the backup + of the original prefix, and leave the original spec marked installed. + """ + # Where to store the backups + backup = str(tmpdir.mkdir("backup")) + + # Get a build task. TODO: refactor this to avoid calling internal methods + const_arg = installer_args(["b"]) + installer = create_installer(const_arg) + installer._init_queue() + task = installer._pop_task() + + # Make sure the install prefix exists with some trivial file + installed_file = os.path.join(task.pkg.prefix, 'some_file') + fs.touchp(installed_file) + + class InstallerThatWipesThePrefixDir: + def _install_task(self, task): + shutil.rmtree(task.pkg.prefix, ignore_errors=True) + fs.mkdirp(task.pkg.prefix) + raise Exception("Some fatal install error") + + class FakeDatabase: + called = False + + def remove(self, spec): + self.called = True + + fake_installer = InstallerThatWipesThePrefixDir() + fake_db = FakeDatabase() + overwrite_install = inst.OverwriteInstall( + fake_installer, fake_db, task, tmp_root=backup) + + # Installation should throw the installation exception, not the backup + # failure. + with pytest.raises(Exception, match='Some fatal install error'): + overwrite_install.install() + + # Make sure the package is not marked uninstalled and the original dir + # is back. + assert not fake_db.called + assert os.path.exists(installed_file) + + +def test_overwrite_install_backup_failure(temporary_store, config, mock_packages, + tmpdir): + """ + When doing an overwrite install that fails, Spack should try to recover the + original prefix. If that fails, the spec is lost, and it should be removed + from the database. + """ + # Where to store the backups + backup = str(tmpdir.mkdir("backup")) + + class InstallerThatAccidentallyDeletesTheBackupDir: + def _install_task(self, task): + # Remove the backup directory so that restoring goes terribly wrong + shutil.rmtree(backup) + raise Exception("Some fatal install error") + + class FakeDatabase: + called = False + + def remove(self, spec): + self.called = True + + # Get a build task. TODO: refactor this to avoid calling internal methods + const_arg = installer_args(["b"]) + installer = create_installer(const_arg) + installer._init_queue() + task = installer._pop_task() + + # Make sure the install prefix exists + installed_file = os.path.join(task.pkg.prefix, 'some_file') + fs.touchp(installed_file) + + fake_installer = InstallerThatAccidentallyDeletesTheBackupDir() + fake_db = FakeDatabase() + overwrite_install = inst.OverwriteInstall( + fake_installer, fake_db, task, tmp_root=backup) + + # Installation should throw the installation exception, not the backup + # failure. + with pytest.raises(Exception, match='Some fatal install error'): + overwrite_install.install() + + # Make sure that `remove` was called on the database after an unsuccessful + # attempt to restore the backup. + assert fake_db.called |