From 5015f5fe12d93024ed0d7594d860f1f2cd977548 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Thu, 14 Dec 2023 17:29:11 +0100 Subject: [PATCH] `verdi profile delete`: Make it storage plugin agnostic (#6224) The `verdi profile delete` implementation was hardcoded to work only for the `core.psql_dos` storage plugin. However, it is possible for profiles to use different storage plugins, causing the command to except. The command now forwards to `Config.delete_profile`. This method gets the storage plugin defined for the profile and constructs an instance of it for the given profile. It then calls `delete` on the storage instance if the `delete_storage` argument is set to `True`. The new `delete` method is defined on the `StorageBackend` abstract base class. It is intentionally not made abstract, but rather explicitly raises `NotImplementedError`. Otherwise existing storage plugins outside of `aiida-core` would no longer be instantiable before they implement the method. The various options of `verdi profile delete` to control what parts of the storage are deleted, are simplified to just a single switch that controls whether the storage is deleted or not. The switch is not defined at all by default. The user will have to explicitly select either `--delete-data` or `--keep-data`, or they will be prompted what value to choose. If the `--force` flag is specified and no explicit flag to delete or keep the data, the command raises. The command now also properly detects if the delete profile was the default, in which case another profile is marked as the new default if any profiles remain at all. --- aiida/cmdline/commands/cmd_profile.py | 78 +++++++-------------- aiida/manage/configuration/config.py | 49 ++++++------- aiida/orm/implementation/storage_backend.py | 4 ++ aiida/storage/psql_dos/backend.py | 29 ++++++++ aiida/storage/sqlite_dos/backend.py | 11 +++ aiida/storage/sqlite_temp/backend.py | 4 ++ aiida/storage/sqlite_zip/backend.py | 7 ++ tests/cmdline/commands/test_profile.py | 63 +++++++++++++---- tests/manage/configuration/test_config.py | 2 +- 9 files changed, 149 insertions(+), 98 deletions(-) diff --git a/aiida/cmdline/commands/cmd_profile.py b/aiida/cmdline/commands/cmd_profile.py index 1b52c07da2..e004d9daad 100644 --- a/aiida/cmdline/commands/cmd_profile.py +++ b/aiida/cmdline/commands/cmd_profile.py @@ -8,6 +8,8 @@ # For further information please visit http://www.aiida.net # ########################################################################### """`verdi profile` command.""" +from __future__ import annotations + import click from aiida.cmdline.commands.cmd_verdi import verdi @@ -133,69 +135,39 @@ def profile_setdefault(profile): @verdi_profile.command('delete') -@options.FORCE(help='to skip questions and warnings about loss of data') -@click.option( - '--include-config/--skip-config', - default=True, - show_default=True, - help='Include deletion of entry in configuration file.' -) -@click.option( - '--include-db/--skip-db', - 'include_database', - default=True, - show_default=True, - help='Include deletion of associated database.' -) -@click.option( - '--include-db-user/--skip-db-user', - 'include_database_user', - default=False, - show_default=True, - help='Include deletion of associated database user.' -) +@options.FORCE(help='Skip any prompts for confirmation.') @click.option( - '--include-repository/--skip-repository', - default=True, - show_default=True, - help='Include deletion of associated file repository.' + '--delete-data/--keep-data', + default=None, + help='Whether to delete the storage with all its data or not. This flag has to be explicitly specified' ) @arguments.PROFILES(required=True) -def profile_delete(force, include_config, include_database, include_database_user, include_repository, profiles): +def profile_delete(force, delete_data, profiles): """Delete one or more profiles. The PROFILES argument takes one or multiple profile names that will be deleted. Deletion here means that the profile - will be removed including its file repository and database. The various options can be used to control which parts - of the profile are deleted. + will be removed from the config file. If ``--delete-storage`` is specified, the storage containing all data is also + deleted. """ - if not include_config: - echo.echo_deprecated('the `--skip-config` option is deprecated and is no longer respected.') - - for profile in profiles: + if force and delete_data is None: + raise click.BadParameter( + 'When the `-f/--force` flag is used either `--delete-data` or `--keep-data` has to be explicitly specified.' + ) - includes = { - 'database': include_database, - 'database user': include_database_user, - 'file repository': include_repository - } + if not force and delete_data is None: + echo.echo_warning('Do you also want to permanently delete all data?', nl=False) + delete_data = click.confirm('', default=False) - if not all(includes.values()): - excludes = [label for label, value in includes.items() if not value] - message_suffix = f' excluding: {", ".join(excludes)}.' - else: - message_suffix = '.' + for profile in profiles: + suffix = click.style('including' if delete_data else 'without', fg='red', bold=True) + echo.echo_warning(f'Deleting profile `{profile.name}`, {suffix} all data.') - echo.echo_warning(f'deleting profile `{profile.name}`{message_suffix}') - echo.echo_warning('this operation cannot be undone, ', nl=False) + if not force: + echo.echo_warning('This operation cannot be undone, are you sure you want to continue?', nl=False) - if not force and not click.confirm('are you sure you want to continue?'): - echo.echo_report(f'deleting of `{profile.name} cancelled.') + if not force and not click.confirm(''): + echo.echo_report(f'Deleting of `{profile.name}` cancelled.') continue - get_config().delete_profile( - profile.name, - include_database=include_database, - include_database_user=include_database_user, - include_repository=include_repository - ) - echo.echo_success(f'profile `{profile.name}` was deleted{message_suffix}.') + get_config().delete_profile(profile.name, delete_storage=delete_data) + echo.echo_success(f'Profile `{profile.name}` was deleted.') diff --git a/aiida/manage/configuration/config.py b/aiida/manage/configuration/config.py index cc7a2db47b..ff6c9cbcdf 100644 --- a/aiida/manage/configuration/config.py +++ b/aiida/manage/configuration/config.py @@ -545,44 +545,35 @@ def remove_profile(self, name): self._profiles.pop(name) return self - def delete_profile( - self, - name: str, - include_database: bool = True, - include_database_user: bool = False, - include_repository: bool = True - ): + def delete_profile(self, name: str, delete_storage: bool = True) -> None: """Delete a profile including its storage. - :param include_database: also delete the database configured for the profile. - :param include_database_user: also delete the database user configured for the profile. - :param include_repository: also delete the repository configured for the profile. + :param delete_storage: Whether to delete the storage with all its data or not. """ - import shutil - - from aiida.manage.external.postgres import Postgres + from aiida.plugins import StorageFactory profile = self.get_profile(name) + is_default_profile: bool = profile.name == self.default_profile_name - if include_repository: - # Note, this is currently being hardcoded, but really this `delete_profile` should get the storage backend - # for the given profile and call `StorageBackend.erase` method. Currently this is leaking details - # of the implementation of the PsqlDosBackend into a generic function which would fail for profiles that - # use a different storage backend implementation. - from aiida.storage.psql_dos.backend import get_filepath_container - folder = get_filepath_container(profile).parent - if folder.exists(): - shutil.rmtree(folder) + if delete_storage: + storage_cls = StorageFactory(profile.storage_backend) + storage = storage_cls(profile) + storage.delete() + LOGGER.report(f'Data storage deleted, configuration was: {profile.storage_config}') + else: + LOGGER.report(f'Data storage not deleted, configuration is: {profile.storage_config}') - if include_database: - postgres = Postgres.from_profile(profile) - if postgres.db_exists(profile.storage_config['database_name']): - postgres.drop_db(profile.storage_config['database_name']) + self.remove_profile(name) - if include_database_user and postgres.dbuser_exists(profile.storage_config['database_username']): - postgres.drop_dbuser(profile.storage_config['database_username']) + if is_default_profile and not self.profile_names: + LOGGER.warning(f'`{name}` was the default profile, no profiles remain to set as default.') + self.store() + return + + if is_default_profile: + LOGGER.warning(f'`{name}` was the default profile, setting `{self.profile_names[0]}` as the new default.') + self.set_default_profile(self.profile_names[0], overwrite=True) - self.remove_profile(name) self.store() def set_default_profile(self, name, overwrite=False): diff --git a/aiida/orm/implementation/storage_backend.py b/aiida/orm/implementation/storage_backend.py index b62440f3f7..248f6392d9 100644 --- a/aiida/orm/implementation/storage_backend.py +++ b/aiida/orm/implementation/storage_backend.py @@ -247,6 +247,10 @@ def bulk_update(self, entity_type: 'EntityTypes', rows: List[dict]) -> None: :raises: ``IntegrityError`` if the keys in a row are not a subset of the columns in the table """ + def delete(self) -> None: + """Delete the storage and all the data.""" + raise NotImplementedError() + @abc.abstractmethod def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]): """Delete all nodes corresponding to pks in the input and any links to/from them. diff --git a/aiida/storage/psql_dos/backend.py b/aiida/storage/psql_dos/backend.py index afb1a6936c..6b83d9bbc4 100644 --- a/aiida/storage/psql_dos/backend.py +++ b/aiida/storage/psql_dos/backend.py @@ -20,6 +20,7 @@ from sqlalchemy.orm import Session, scoped_session, sessionmaker from aiida.common.exceptions import ClosedStorage, ConfigurationError, IntegrityError +from aiida.common.log import AIIDA_LOGGER from aiida.manage.configuration.profile import Profile from aiida.orm.entities import EntityTypes from aiida.orm.implementation import BackendEntity, StorageBackend @@ -36,6 +37,7 @@ __all__ = ('PsqlDosBackend',) +LOGGER = AIIDA_LOGGER.getChild(__file__) CONTAINER_DEFAULTS: dict = { 'pack_size_target': 4 * 1024 * 1024 * 1024, 'loose_prefix_len': 2, @@ -347,6 +349,33 @@ def bulk_update(self, entity_type: EntityTypes, rows: List[dict]) -> None: with (nullcontext() if self.in_transaction else self.transaction()): session.execute(update(mapper), rows) + def delete(self, delete_database_user: bool = False) -> None: + """Delete the storage and all the data. + + :param delete_database_user: Also delete the database user. This is ``False`` by default because the user may be + used by other databases. + """ + import shutil + + from aiida.manage.external.postgres import Postgres + + profile = self.profile + config = profile.storage_config + postgres = Postgres.from_profile(self.profile) + repository = get_filepath_container(profile).parent + + if repository.exists(): + shutil.rmtree(repository) + LOGGER.report(f'Deleted repository at `{repository}`.') + + if postgres.db_exists(config['database_name']): + postgres.drop_db(config['database_name']) + LOGGER.report(f'Deleted database `{config["database_name"]}`.') + + if delete_database_user and postgres.dbuser_exists(config['database_username']): + postgres.drop_dbuser(config['database_username']) + LOGGER.report(f'Deleted database user `{config["database_username"]}`.') + def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]) -> None: # pylint: disable=no-value-for-parameter from aiida.storage.psql_dos.models.group import DbGroupNode diff --git a/aiida/storage/sqlite_dos/backend.py b/aiida/storage/sqlite_dos/backend.py index 8d928eb0b2..aebbe3d265 100644 --- a/aiida/storage/sqlite_dos/backend.py +++ b/aiida/storage/sqlite_dos/backend.py @@ -12,6 +12,7 @@ from functools import cached_property from pathlib import Path +from shutil import rmtree from typing import TYPE_CHECKING from uuid import uuid4 @@ -20,6 +21,7 @@ from sqlalchemy import insert from sqlalchemy.orm import scoped_session, sessionmaker +from aiida.common.log import AIIDA_LOGGER from aiida.manage import Profile from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER from aiida.orm.implementation import BackendEntity @@ -36,6 +38,8 @@ __all__ = ('SqliteDosStorage',) +LOGGER = AIIDA_LOGGER.getChild(__file__) + class SqliteDosMigrator(PsqlDosMigrator): """Storage implementation using Sqlite database and disk-objectstore container. @@ -143,6 +147,13 @@ def _initialise_session(self): engine = create_sqla_engine(Path(self._profile.storage_config['filepath']) / 'database.sqlite') self._session_factory = scoped_session(sessionmaker(bind=engine, future=True, expire_on_commit=True)) + def delete(self) -> None: # type: ignore[override] # pylint: disable=arguments-differ + """Delete the storage and all the data.""" + filepath = Path(self.profile.storage_config['filepath']) + if filepath.exists(): + rmtree(filepath) + LOGGER.report(f'Deleted storage directory at `{filepath}`.') + def get_repository(self) -> 'DiskObjectStoreRepositoryBackend': from aiida.repository.backend import DiskObjectStoreRepositoryBackend container = Container(str(Path(self.profile.storage_config['filepath']) / 'container')) diff --git a/aiida/storage/sqlite_temp/backend.py b/aiida/storage/sqlite_temp/backend.py index 82b4709cab..ffac23d5ee 100644 --- a/aiida/storage/sqlite_temp/backend.py +++ b/aiida/storage/sqlite_temp/backend.py @@ -284,6 +284,10 @@ def bulk_update(self, entity_type: EntityTypes, rows: list[dict]) -> None: with (nullcontext() if self.in_transaction else self.transaction()): session.execute(update(mapper), rows) + def delete(self) -> None: + """Delete the storage and all the data.""" + self._repo.erase() + def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]): raise NotImplementedError diff --git a/aiida/storage/sqlite_zip/backend.py b/aiida/storage/sqlite_zip/backend.py index 9bc4896d67..7814c83e20 100644 --- a/aiida/storage/sqlite_zip/backend.py +++ b/aiida/storage/sqlite_zip/backend.py @@ -293,6 +293,13 @@ def bulk_insert(self, entity_type: EntityTypes, rows: list[dict], allow_defaults def bulk_update(self, entity_type: EntityTypes, rows: list[dict]) -> None: raise ReadOnlyError() + def delete(self) -> None: + """Delete the storage and all the data.""" + filepath = Path(self.profile.storage_config['filepath']) + if filepath.exists(): + filepath.unlink() + LOGGER.report(f'Deleted archive at `{filepath}`.') + def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]): raise ReadOnlyError() diff --git a/tests/cmdline/commands/test_profile.py b/tests/cmdline/commands/test_profile.py index 43daec8a10..659eb757a0 100644 --- a/tests/cmdline/commands/test_profile.py +++ b/tests/cmdline/commands/test_profile.py @@ -106,35 +106,68 @@ def test_show_with_profile_option(run_cli_command, mock_profiles): assert profile_name_non_default not in result.output -def test_delete_partial(run_cli_command, mock_profiles): - """Test the ``verdi profile delete`` command. - - .. note:: we skip deleting the database as this might require sudo rights and this is tested in the CI tests - defined in the file ``.github/system_tests/test_profile.py`` - """ - profile_list = mock_profiles() - run_cli_command(cmd_profile.profile_delete, ['--force', '--skip-db', profile_list[1]], use_subprocess=False) - result = run_cli_command(cmd_profile.profile_list, use_subprocess=False) - assert profile_list[1] not in result.output - - def test_delete(run_cli_command, mock_profiles, pg_test_cluster): """Test for verdi profile delete command.""" kwargs = {'database_port': pg_test_cluster.dsn['port']} profile_list = mock_profiles(**kwargs) + config = configuration.get_config() # Delete single profile - run_cli_command(cmd_profile.profile_delete, ['--force', profile_list[1]], use_subprocess=False) + result = run_cli_command( + cmd_profile.profile_delete, ['--force', '--keep-data', profile_list[0]], use_subprocess=False + ) + output = result.output + assert f'`{profile_list[0]}` was the default profile, setting `{profile_list[1]}` as the new default.' in output result = run_cli_command(cmd_profile.profile_list, use_subprocess=False) - assert profile_list[1] not in result.output + assert profile_list[0] not in result.output + assert config.default_profile_name == profile_list[1] # Delete multiple profiles - run_cli_command(cmd_profile.profile_delete, ['--force', profile_list[2], profile_list[3]], use_subprocess=False) + result = run_cli_command( + cmd_profile.profile_delete, ['--force', '--keep-data', profile_list[1], profile_list[2], profile_list[3]], + use_subprocess=False + ) + assert 'was the default profile, no profiles remain to set as default.' in result.output result = run_cli_command(cmd_profile.profile_list, use_subprocess=False) + assert profile_list[1] not in result.output assert profile_list[2] not in result.output assert profile_list[3] not in result.output +def test_delete_force(run_cli_command, mock_profiles, pg_test_cluster): + """Test that if force is specified the ``--delete-data`` or ``--keep-data`` has to be explicitly specified.""" + kwargs = {'database_port': pg_test_cluster.dsn['port']} + profile_list = mock_profiles(**kwargs) + + result = run_cli_command( + cmd_profile.profile_delete, ['--force', profile_list[0]], use_subprocess=False, raises=True + ) + assert 'When the `-f/--force` flag is used either `--delete-data` or `--keep-data`' in result.output + + +@pytest.mark.parametrize('entry_point', ('core.sqlite_dos', 'core.sqlite_zip')) +def test_delete_storage(run_cli_command, isolated_config, tmp_path, entry_point): + """Test the ``verdi profile delete`` command with the ``--delete-storage`` option.""" + profile_name = 'temp-profile' + + if entry_point == 'core.sqlite_zip': + filepath = tmp_path / 'archive.aiida' + create_archive([], filename=filepath) + else: + filepath = tmp_path / 'storage' + + options = [entry_point, '-n', '--filepath', str(filepath), '--profile', profile_name, '--email', 'email@host'] + result = run_cli_command(cmd_profile.profile_setup, options, use_subprocess=False) + assert filepath.exists() + assert profile_name in isolated_config.profile_names + + run_cli_command(cmd_profile.profile_delete, ['--force', '--delete-data', profile_name], use_subprocess=False) + result = run_cli_command(cmd_profile.profile_list, use_subprocess=False) + assert profile_name not in result.output + assert not filepath.exists() + assert profile_name not in isolated_config.profile_names + + @pytest.mark.parametrize('entry_point', ('core.sqlite_temp', 'core.sqlite_dos', 'core.sqlite_zip')) def test_setup(run_cli_command, isolated_config, tmp_path, entry_point): """Test the ``verdi profile setup`` command. diff --git a/tests/manage/configuration/test_config.py b/tests/manage/configuration/test_config.py index 13f9bd54b7..5e5d684d6b 100644 --- a/tests/manage/configuration/test_config.py +++ b/tests/manage/configuration/test_config.py @@ -427,7 +427,7 @@ def test_delete_profile(config_with_profile, profile_factory): # Write the contents to disk so that the to-be-deleted profile is in the config file on disk config.store() - config.delete_profile(profile_name) + config.delete_profile(profile_name, delete_storage=False) assert profile_name not in config.profile_names # Now reload the config from disk to make sure the changes after deletion were persisted to disk