diff --git a/NEWS b/NEWS index a9eb7f7ec..46f1cd0e0 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ 1.5.6.dev0 + * #316: Fix hang when a stale database dump named pipe from an aborted borgmatic run remains on + disk. * Tweak comment indentation in generated configuration file for clarity. 1.5.5 diff --git a/borgmatic/hooks/dump.py b/borgmatic/hooks/dump.py index e31db2374..8bc9fcb89 100644 --- a/borgmatic/hooks/dump.py +++ b/borgmatic/hooks/dump.py @@ -48,46 +48,24 @@ def create_named_pipe_for_dump(dump_path): os.mkfifo(dump_path, mode=0o600) -def remove_database_dumps(dump_path, databases, database_type_name, log_prefix, dry_run): +def remove_database_dumps(dump_path, database_type_name, log_prefix, dry_run): ''' - Remove the database dumps for the given databases in the dump directory path. The databases are - supplied as a sequence of dicts, one dict describing each database as per the configuration - schema. Use the name of the database type and the log prefix in any log entries. If this is a - dry run, then don't actually remove anything. + Remove all database dumps in the given dump directory path (including the directory itself). If + this is a dry run, then don't actually remove anything. ''' - if not databases: - logger.debug('{}: No {} databases configured'.format(log_prefix, database_type_name)) - return - dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' logger.info( '{}: Removing {} database dumps{}'.format(log_prefix, database_type_name, dry_run_label) ) - for database in databases: - dump_filename = make_database_dump_filename( - dump_path, database['name'], database.get('hostname') - ) + expanded_path = os.path.expanduser(dump_path) - logger.debug( - '{}: Removing {} database dump {} from {}{}'.format( - log_prefix, database_type_name, database['name'], dump_filename, dry_run_label - ) - ) - if dry_run: - continue + if dry_run: + return - if os.path.exists(dump_filename): - if os.path.isdir(dump_filename): - shutil.rmtree(dump_filename) - else: - os.remove(dump_filename) - - dump_file_dir = os.path.dirname(dump_filename) - - if os.path.exists(dump_file_dir) and len(os.listdir(dump_file_dir)) == 0: - os.rmdir(dump_file_dir) + if os.path.exists(expanded_path): + shutil.rmtree(expanded_path) def convert_glob_patterns_to_borg_patterns(patterns): diff --git a/borgmatic/hooks/mysql.py b/borgmatic/hooks/mysql.py index d3599e647..99cd73e83 100644 --- a/borgmatic/hooks/mysql.py +++ b/borgmatic/hooks/mysql.py @@ -118,14 +118,11 @@ def dump_databases(databases, log_prefix, location_config, dry_run): def remove_database_dumps(databases, log_prefix, location_config, dry_run): # pragma: no cover ''' - Remove the database dumps for the given databases. The databases are supplied as a sequence of - dicts, one dict describing each database as per the configuration schema. Use the log prefix in - any log entries. Use the given location configuration dict to construct the destination path. If - this is a dry run, then don't actually remove anything. + Remove all database dump files for this hook regardless of the given databases. Use the log + prefix in any log entries. Use the given location configuration dict to construct the + destination path. If this is a dry run, then don't actually remove anything. ''' - dump.remove_database_dumps( - make_dump_path(location_config), databases, 'MySQL', log_prefix, dry_run - ) + dump.remove_database_dumps(make_dump_path(location_config), 'MySQL', log_prefix, dry_run) def make_database_dump_pattern( diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index a40189f13..db90025e2 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -82,14 +82,11 @@ def dump_databases(databases, log_prefix, location_config, dry_run): def remove_database_dumps(databases, log_prefix, location_config, dry_run): # pragma: no cover ''' - Remove the database dumps for the given databases. The databases are supplied as a sequence of - dicts, one dict describing each database as per the configuration schema. Use the log prefix in - any log entries. Use the given location configuration dict to construct the destination path. If - this is a dry run, then don't actually remove anything. + Remove all database dump files for this hook regardless of the given databases. Use the log + prefix in any log entries. Use the given location configuration dict to construct the + destination path. If this is a dry run, then don't actually remove anything. ''' - dump.remove_database_dumps( - make_dump_path(location_config), databases, 'PostgreSQL', log_prefix, dry_run - ) + dump.remove_database_dumps(make_dump_path(location_config), 'PostgreSQL', log_prefix, dry_run) def make_database_dump_pattern( diff --git a/tests/unit/hooks/test_dump.py b/tests/unit/hooks/test_dump.py index 1b2f28c05..1b13b952c 100644 --- a/tests/unit/hooks/test_dump.py +++ b/tests/unit/hooks/test_dump.py @@ -47,69 +47,28 @@ def test_create_named_pipe_for_dump_does_not_raise(): module.create_named_pipe_for_dump('/path/to/pipe') -def test_remove_database_dumps_removes_dump_for_each_database(): - databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').with_args( - 'databases', 'foo', None - ).and_return('databases/localhost/foo') - flexmock(module).should_receive('make_database_dump_filename').with_args( - 'databases', 'bar', None - ).and_return('databases/localhost/bar') - +def test_remove_database_dumps_removes_dump_path(): + flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost') flexmock(module.os.path).should_receive('exists').and_return(True) - flexmock(module.os.path).should_receive('isdir').and_return(False) - flexmock(module.os).should_receive('remove').with_args('databases/localhost/foo').once() - flexmock(module.os).should_receive('remove').with_args('databases/localhost/bar').once() - flexmock(module.os).should_receive('listdir').with_args('databases/localhost').and_return( - ['bar'] - ).and_return([]) + flexmock(module.shutil).should_receive('rmtree').with_args('databases/localhost').once() - flexmock(module.os).should_receive('rmdir').with_args('databases/localhost').once() - - module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False) - - -def test_remove_database_dumps_removes_dump_in_directory_format(): - databases = [{'name': 'foo'}] - flexmock(module).should_receive('make_database_dump_filename').with_args( - 'databases', 'foo', None - ).and_return('databases/localhost/foo') - - flexmock(module.os.path).should_receive('exists').and_return(True) - flexmock(module.os.path).should_receive('isdir').and_return(True) - flexmock(module.os).should_receive('remove').never() - flexmock(module.shutil).should_receive('rmtree').with_args('databases/localhost/foo').once() - flexmock(module.os).should_receive('listdir').with_args('databases/localhost').and_return([]) - flexmock(module.os).should_receive('rmdir').with_args('databases/localhost').once() - - module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False) + module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=False) def test_remove_database_dumps_with_dry_run_skips_removal(): - databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module.os).should_receive('rmdir').never() - flexmock(module.os).should_receive('remove').never() - - module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=True) - - -def test_remove_database_dumps_without_dump_present_skips_removal(): - databases = [{'name': 'foo'}] - flexmock(module).should_receive('make_database_dump_filename').with_args( - 'databases', 'foo', None - ).and_return('databases/localhost/foo') - - flexmock(module.os.path).should_receive('exists').and_return(False) - flexmock(module.os.path).should_receive('isdir').never() - flexmock(module.os).should_receive('remove').never() + flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost') + flexmock(module.os.path).should_receive('exists').never() flexmock(module.shutil).should_receive('rmtree').never() - flexmock(module.os).should_receive('rmdir').never() - module.remove_database_dumps('databases', databases, 'SuperDB', 'test.yaml', dry_run=False) + module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=True) -def test_remove_database_dumps_without_databases_does_not_raise(): - module.remove_database_dumps('databases', [], 'SuperDB', 'test.yaml', dry_run=False) +def test_remove_database_dumps_without_dump_path_present_skips_removal(): + flexmock(module.os.path).should_receive('expanduser').and_return('databases/localhost') + flexmock(module.os.path).should_receive('exists').and_return(False) + flexmock(module.shutil).should_receive('rmtree').never() + + module.remove_database_dumps('databases', 'SuperDB', 'test.yaml', dry_run=False) def test_convert_glob_patterns_to_borg_patterns_removes_leading_slash():