From 9d29ecf3049b7be5fb89080fdff6cea94397d18f Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Fri, 8 Nov 2019 11:53:27 -0800 Subject: [PATCH] Remove MySQL/MariaDB database dumps after backing them up (#228). --- borgmatic/commands/borgmatic.py | 3 ++ borgmatic/hooks/database.py | 14 ------- borgmatic/hooks/dump.py | 54 +++++++++++++++++++++++++++ borgmatic/hooks/mysql.py | 13 ++++++- borgmatic/hooks/postgresql.py | 37 ++++-------------- tests/unit/commands/test_borgmatic.py | 7 ++++ tests/unit/hooks/test_database.py | 26 ------------- tests/unit/hooks/test_dump.py | 54 +++++++++++++++++++++++++++ tests/unit/hooks/test_mysql.py | 12 +++--- tests/unit/hooks/test_postgresql.py | 52 ++++++-------------------- 10 files changed, 154 insertions(+), 118 deletions(-) delete mode 100644 borgmatic/hooks/database.py create mode 100644 borgmatic/hooks/dump.py delete mode 100644 tests/unit/hooks/test_database.py create mode 100644 tests/unit/hooks/test_dump.py diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index e1ad3e4fc..b0884ab88 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -107,6 +107,9 @@ def run_configuration(config_filename, config, arguments): postgresql.remove_database_dumps( hooks.get('postgresql_databases'), config_filename, global_arguments.dry_run ) + mysql.remove_database_dumps( + hooks.get('mysql_databases'), config_filename, global_arguments.dry_run + ) command.execute_hook( hooks.get('after_backup'), hooks.get('umask'), diff --git a/borgmatic/hooks/database.py b/borgmatic/hooks/database.py deleted file mode 100644 index da7737dca..000000000 --- a/borgmatic/hooks/database.py +++ /dev/null @@ -1,14 +0,0 @@ -import os - - -def make_database_dump_filename(dump_path, name, hostname=None): - ''' - Based on the given dump directory path, database name, and hostname, return a filename to use - for the database dump. The hostname defaults to localhost. - - Raise ValueError if the database name is invalid. - ''' - if os.path.sep in name: - raise ValueError('Invalid database name {}'.format(name)) - - return os.path.join(os.path.expanduser(dump_path), hostname or 'localhost', name) diff --git a/borgmatic/hooks/dump.py b/borgmatic/hooks/dump.py new file mode 100644 index 000000000..a5f8d3d45 --- /dev/null +++ b/borgmatic/hooks/dump.py @@ -0,0 +1,54 @@ +import logging +import os + +logger = logging.getLogger(__name__) + + +def make_database_dump_filename(dump_path, name, hostname=None): + ''' + Based on the given dump directory path, database name, and hostname, return a filename to use + for the database dump. The hostname defaults to localhost. + + Raise ValueError if the database name is invalid. + ''' + if os.path.sep in name: + raise ValueError('Invalid database name {}'.format(name)) + + return os.path.join(os.path.expanduser(dump_path), hostname or 'localhost', name) + + +def remove_database_dumps(dump_path, databases, 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. + ''' + 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') + ) + + logger.debug( + '{}: Removing {} database dump {} from {}{}'.format( + log_prefix, database_type_name, database['name'], dump_filename, dry_run_label + ) + ) + if dry_run: + continue + + os.remove(dump_filename) + dump_path = os.path.dirname(dump_filename) + + if len(os.listdir(dump_path)) == 0: + os.rmdir(dump_path) diff --git a/borgmatic/hooks/mysql.py b/borgmatic/hooks/mysql.py index 53a44ebcc..bebee3bf1 100644 --- a/borgmatic/hooks/mysql.py +++ b/borgmatic/hooks/mysql.py @@ -2,7 +2,7 @@ import logging import os from borgmatic.execute import execute_command -from borgmatic.hooks.database import make_database_dump_filename +from borgmatic.hooks import dump DUMP_PATH = '~/.borgmatic/mysql_databases' logger = logging.getLogger(__name__) @@ -24,7 +24,7 @@ def dump_databases(databases, log_prefix, dry_run): for database in databases: name = database['name'] - dump_filename = make_database_dump_filename(DUMP_PATH, name, database.get('hostname')) + dump_filename = dump.make_database_dump_filename(DUMP_PATH, name, database.get('hostname')) command = ( ('mysqldump', '--add-drop-database') + (('--host', database['hostname']) if 'hostname' in database else ()) @@ -46,3 +46,12 @@ def dump_databases(databases, log_prefix, dry_run): execute_command( command, output_file=open(dump_filename, 'w'), extra_environment=extra_environment ) + + +def remove_database_dumps(databases, log_prefix, dry_run): + ''' + 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. If this is a dry run, then don't actually remove anything. + ''' + dump.remove_database_dumps(DUMP_PATH, databases, 'MySQL', log_prefix, dry_run) diff --git a/borgmatic/hooks/postgresql.py b/borgmatic/hooks/postgresql.py index 4ebb3abde..0ad0ad6fa 100644 --- a/borgmatic/hooks/postgresql.py +++ b/borgmatic/hooks/postgresql.py @@ -3,7 +3,7 @@ import logging import os from borgmatic.execute import execute_command -from borgmatic.hooks.database import make_database_dump_filename +from borgmatic.hooks import dump DUMP_PATH = '~/.borgmatic/postgresql_databases' logger = logging.getLogger(__name__) @@ -25,7 +25,7 @@ def dump_databases(databases, log_prefix, dry_run): for database in databases: name = database['name'] - dump_filename = make_database_dump_filename(DUMP_PATH, name, database.get('hostname')) + dump_filename = dump.make_database_dump_filename(DUMP_PATH, name, database.get('hostname')) all_databases = bool(name == 'all') command = ( ('pg_dumpall' if all_databases else 'pg_dump', '--no-password', '--clean') @@ -55,32 +55,7 @@ def remove_database_dumps(databases, log_prefix, dry_run): dicts, one dict describing each database as per the configuration schema. Use the log prefix in any log entries. If this is a dry run, then don't actually remove anything. ''' - if not databases: - logger.debug('{}: No PostgreSQL databases configured'.format(log_prefix)) - return - - dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' - - logger.info('{}: Removing PostgreSQL database dumps{}'.format(log_prefix, dry_run_label)) - - for database in databases: - dump_filename = make_database_dump_filename( - DUMP_PATH, database['name'], database.get('hostname') - ) - - logger.debug( - '{}: Removing PostgreSQL database dump {} from {}{}'.format( - log_prefix, database['name'], dump_filename, dry_run_label - ) - ) - if dry_run: - continue - - os.remove(dump_filename) - dump_path = os.path.dirname(dump_filename) - - if len(os.listdir(dump_path)) == 0: - os.rmdir(dump_path) + dump.remove_database_dumps(DUMP_PATH, databases, 'PostgreSQL', log_prefix, dry_run) def make_database_dump_patterns(names): @@ -89,7 +64,9 @@ def make_database_dump_patterns(names): dumps in an archive. An empty sequence of names indicates that the patterns should match all dumps. ''' - return [make_database_dump_filename(DUMP_PATH, name, hostname='*') for name in (names or ['*'])] + return [ + dump.make_database_dump_filename(DUMP_PATH, name, hostname='*') for name in (names or ['*']) + ] def convert_glob_patterns_to_borg_patterns(patterns): @@ -150,7 +127,7 @@ def restore_database_dumps(databases, log_prefix, dry_run): dry_run_label = ' (dry run; not actually restoring anything)' if dry_run else '' for database in databases: - dump_filename = make_database_dump_filename( + dump_filename = dump.make_database_dump_filename( DUMP_PATH, database['name'], database.get('hostname') ) restore_command = ( diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index 471240713..2b76b56df 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -24,8 +24,12 @@ def test_run_configuration_executes_hooks_for_create_action(): flexmock(module.borg_environment).should_receive('initialize') flexmock(module.command).should_receive('execute_hook').twice() flexmock(module.postgresql).should_receive('dump_databases').once() + flexmock(module.mysql).should_receive('dump_databases').once() flexmock(module.healthchecks).should_receive('ping_healthchecks').twice() + flexmock(module.cronitor).should_receive('ping_cronitor').twice() + flexmock(module.cronhub).should_receive('ping_cronhub').twice() flexmock(module.postgresql).should_receive('remove_database_dumps').once() + flexmock(module.mysql).should_receive('remove_database_dumps').once() flexmock(module).should_receive('run_actions').and_return([]) config = {'location': {'repositories': ['foo']}} arguments = {'global': flexmock(dry_run=False), 'create': flexmock()} @@ -37,7 +41,10 @@ def test_run_configuration_logs_actions_error(): flexmock(module.borg_environment).should_receive('initialize') flexmock(module.command).should_receive('execute_hook') flexmock(module.postgresql).should_receive('dump_databases') + flexmock(module.mysql).should_receive('dump_databases') flexmock(module.healthchecks).should_receive('ping_healthchecks') + flexmock(module.cronitor).should_receive('ping_cronitor') + flexmock(module.cronhub).should_receive('ping_cronhub') expected_results = [flexmock()] flexmock(module).should_receive('make_error_log_records').and_return(expected_results) flexmock(module).should_receive('run_actions').and_raise(OSError) diff --git a/tests/unit/hooks/test_database.py b/tests/unit/hooks/test_database.py deleted file mode 100644 index dbbda76ad..000000000 --- a/tests/unit/hooks/test_database.py +++ /dev/null @@ -1,26 +0,0 @@ -import pytest -from flexmock import flexmock - -from borgmatic.hooks import database as module - - -def test_make_database_dump_filename_uses_name_and_hostname(): - flexmock(module.os.path).should_receive('expanduser').and_return('databases') - - assert ( - module.make_database_dump_filename('databases', 'test', 'hostname') - == 'databases/hostname/test' - ) - - -def test_make_database_dump_filename_without_hostname_defaults_to_localhost(): - flexmock(module.os.path).should_receive('expanduser').and_return('databases') - - assert module.make_database_dump_filename('databases', 'test') == 'databases/localhost/test' - - -def test_make_database_dump_filename_with_invalid_name_raises(): - flexmock(module.os.path).should_receive('expanduser').and_return('databases') - - with pytest.raises(ValueError): - module.make_database_dump_filename('databases', 'invalid/name') diff --git a/tests/unit/hooks/test_dump.py b/tests/unit/hooks/test_dump.py new file mode 100644 index 000000000..3546e84e7 --- /dev/null +++ b/tests/unit/hooks/test_dump.py @@ -0,0 +1,54 @@ +import pytest +from flexmock import flexmock + +from borgmatic.hooks import dump as module + + +def test_make_database_dump_filename_uses_name_and_hostname(): + flexmock(module.os.path).should_receive('expanduser').and_return('databases') + + assert ( + module.make_database_dump_filename('databases', 'test', 'hostname') + == 'databases/hostname/test' + ) + + +def test_make_database_dump_filename_without_hostname_defaults_to_localhost(): + flexmock(module.os.path).should_receive('expanduser').and_return('databases') + + assert module.make_database_dump_filename('databases', 'test') == 'databases/localhost/test' + + +def test_make_database_dump_filename_with_invalid_name_raises(): + flexmock(module.os.path).should_receive('expanduser').and_return('databases') + + with pytest.raises(ValueError): + module.make_database_dump_filename('databases', 'invalid/name') + + +def test_remove_database_dumps_removes_dump_for_each_database(): + databases = [{'name': 'foo'}, {'name': 'bar'}] + flexmock(module).should_receive('make_database_dump_filename').and_return( + 'databases/localhost/foo' + ).and_return('databases/localhost/bar') + flexmock(module.os).should_receive('listdir').and_return([]) + flexmock(module.os).should_receive('rmdir') + + for name in ('foo', 'bar'): + flexmock(module.os).should_receive('remove').with_args( + 'databases/localhost/{}'.format(name) + ).once() + + module.remove_database_dumps('databases', 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_databases_does_not_raise(): + module.remove_database_dumps('databases', [], 'SuperDB', 'test.yaml', dry_run=False) diff --git a/tests/unit/hooks/test_mysql.py b/tests/unit/hooks/test_mysql.py index 078d2f750..8e35f8995 100644 --- a/tests/unit/hooks/test_mysql.py +++ b/tests/unit/hooks/test_mysql.py @@ -8,7 +8,7 @@ from borgmatic.hooks import mysql as module def test_dump_databases_runs_mysqldump_for_each_database(): databases = [{'name': 'foo'}, {'name': 'bar'}] output_file = flexmock() - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') flexmock(module.os).should_receive('makedirs') @@ -26,7 +26,7 @@ def test_dump_databases_runs_mysqldump_for_each_database(): def test_dump_databases_with_dry_run_skips_mysqldump(): databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') flexmock(module.os).should_receive('makedirs').never() @@ -42,7 +42,7 @@ def test_dump_databases_without_databases_does_not_raise(): def test_dump_databases_runs_mysqldump_with_hostname_and_port(): databases = [{'name': 'foo', 'hostname': 'database.example.org', 'port': 5433}] output_file = flexmock() - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/database.example.org/foo' ) flexmock(module.os).should_receive('makedirs') @@ -71,7 +71,7 @@ def test_dump_databases_runs_mysqldump_with_hostname_and_port(): def test_dump_databases_runs_mysqldump_with_username_and_password(): databases = [{'name': 'foo', 'username': 'root', 'password': 'trustsome1'}] output_file = flexmock() - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.os).should_receive('makedirs') @@ -89,7 +89,7 @@ def test_dump_databases_runs_mysqldump_with_username_and_password(): def test_dump_databases_runs_mysqldump_with_options(): databases = [{'name': 'foo', 'options': '--stuff=such'}] output_file = flexmock() - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.os).should_receive('makedirs') @@ -107,7 +107,7 @@ def test_dump_databases_runs_mysqldump_with_options(): def test_dump_databases_runs_mysqldump_for_all_databases(): databases = [{'name': 'all'}] output_file = flexmock() - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/all' ) flexmock(module.os).should_receive('makedirs') diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index 7d0ba87df..21062823d 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -6,7 +6,7 @@ from borgmatic.hooks import postgresql as module def test_dump_databases_runs_pg_dump_for_each_database(): databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') flexmock(module.os).should_receive('makedirs') @@ -31,7 +31,7 @@ def test_dump_databases_runs_pg_dump_for_each_database(): def test_dump_databases_with_dry_run_skips_pg_dump(): databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') flexmock(module.os).should_receive('makedirs').never() @@ -46,7 +46,7 @@ def test_dump_databases_without_databases_does_not_raise(): def test_dump_databases_runs_pg_dump_with_hostname_and_port(): databases = [{'name': 'foo', 'hostname': 'database.example.org', 'port': 5433}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/database.example.org/foo' ) flexmock(module.os).should_receive('makedirs') @@ -74,7 +74,7 @@ def test_dump_databases_runs_pg_dump_with_hostname_and_port(): def test_dump_databases_runs_pg_dump_with_username_and_password(): databases = [{'name': 'foo', 'username': 'postgres', 'password': 'trustsome1'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.os).should_receive('makedirs') @@ -100,7 +100,7 @@ def test_dump_databases_runs_pg_dump_with_username_and_password(): def test_dump_databases_runs_pg_dump_with_format(): databases = [{'name': 'foo', 'format': 'tar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.os).should_receive('makedirs') @@ -124,7 +124,7 @@ def test_dump_databases_runs_pg_dump_with_format(): def test_dump_databases_runs_pg_dump_with_options(): databases = [{'name': 'foo', 'options': '--stuff=such'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ) flexmock(module.os).should_receive('makedirs') @@ -149,7 +149,7 @@ def test_dump_databases_runs_pg_dump_with_options(): def test_dump_databases_runs_pg_dumpall_for_all_databases(): databases = [{'name': 'all'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/all' ) flexmock(module.os).should_receive('makedirs') @@ -162,36 +162,8 @@ def test_dump_databases_runs_pg_dumpall_for_all_databases(): module.dump_databases(databases, 'test.yaml', dry_run=False) -def test_remove_database_dumps_removes_dump_for_each_database(): - databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( - 'databases/localhost/foo' - ).and_return('databases/localhost/bar') - flexmock(module.os).should_receive('listdir').and_return([]) - flexmock(module.os).should_receive('rmdir') - - for name in ('foo', 'bar'): - flexmock(module.os).should_receive('remove').with_args( - 'databases/localhost/{}'.format(name) - ).once() - - module.remove_database_dumps(databases, '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, 'test.yaml', dry_run=True) - - -def test_remove_database_dumps_without_databases_does_not_raise(): - module.remove_database_dumps([], 'test.yaml', dry_run=False) - - def test_make_database_dump_patterns_converts_names_to_glob_paths(): - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/*/foo' ).and_return('databases/*/bar') @@ -202,7 +174,7 @@ def test_make_database_dump_patterns_converts_names_to_glob_paths(): def test_make_database_dump_patterns_treats_empty_names_as_matching_all_databases(): - flexmock(module).should_receive('make_database_dump_filename').with_args( + flexmock(module.dump).should_receive('make_database_dump_filename').with_args( module.DUMP_PATH, '*', '*' ).and_return('databases/*/*') @@ -258,7 +230,7 @@ def test_get_database_configurations_with_unknown_database_name_raises(): def test_restore_database_dumps_restores_each_database(): databases = [{'name': 'foo'}, {'name': 'bar'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') @@ -290,7 +262,7 @@ def test_restore_database_dumps_without_databases_does_not_raise(): def test_restore_database_dumps_runs_pg_restore_with_hostname_and_port(): databases = [{'name': 'foo', 'hostname': 'database.example.org', 'port': 5433}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar') @@ -333,7 +305,7 @@ def test_restore_database_dumps_runs_pg_restore_with_hostname_and_port(): def test_restore_database_dumps_runs_pg_restore_with_username_and_password(): databases = [{'name': 'foo', 'username': 'postgres', 'password': 'trustsome1'}] - flexmock(module).should_receive('make_database_dump_filename').and_return( + flexmock(module.dump).should_receive('make_database_dump_filename').and_return( 'databases/localhost/foo' ).and_return('databases/localhost/bar')