From d7f1c10c8c79f8a41a67bb0f8e46016064d9d2ec Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Wed, 12 Oct 2022 10:26:09 -0700 Subject: [PATCH] To prevent Borg hangs, unconditionally delete stale named pipes before dumping databases (#360). --- NEWS | 1 + borgmatic/commands/borgmatic.py | 8 +++--- borgmatic/hooks/dispatch.py | 29 +++++++++++++------- borgmatic/hooks/dump.py | 2 +- tests/unit/commands/test_borgmatic.py | 3 ++- tests/unit/hooks/test_dispatch.py | 39 ++++++++++++++++++++++++--- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/NEWS b/NEWS index 90f48cd..c6676dd 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,7 @@ 1.7.3.dev0 * #357: Add "break-lock" action for removing any repository and cache locks leftover from Borg aborting. + * #360: To prevent Borg hangs, unconditionally delete stale named pipes before dumping databases. * #587: When database hooks are enabled, auto-exclude special files from a "create" action to prevent Borg from hanging. You can override/prevent this behavior by explicitly setting the "read_special" option to true. diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index aef683c..80b7655 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -360,7 +360,7 @@ def run_actions( **hook_context, ) logger.info('{}: Creating archive{}'.format(repository, dry_run_label)) - dispatch.call_hooks( + dispatch.call_hooks_even_if_unconfigured( 'remove_database_dumps', hooks, repository, @@ -395,7 +395,7 @@ def run_actions( if json_output: # pragma: nocover yield json.loads(json_output) - dispatch.call_hooks( + dispatch.call_hooks_even_if_unconfigured( 'remove_database_dumps', hooks, config_filename, @@ -556,7 +556,7 @@ def run_actions( repository, arguments['restore'].archive ) ) - dispatch.call_hooks( + dispatch.call_hooks_even_if_unconfigured( 'remove_database_dumps', hooks, repository, @@ -626,7 +626,7 @@ def run_actions( extract_process, ) - dispatch.call_hooks( + dispatch.call_hooks_even_if_unconfigured( 'remove_database_dumps', hooks, repository, diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index ebe52c4..41dcee0 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -29,19 +29,14 @@ def call_hook(function_name, hooks, log_prefix, hook_name, *args, **kwargs): ''' Given the hooks configuration dict and a prefix to use in log entries, call the requested function of the Python module corresponding to the given hook name. Supply that call with the - configuration for this hook, the log prefix, and any given args and kwargs. Return any return - value. - - If the hook name is not present in the hooks configuration, then bail without calling anything. + configuration for this hook (if any), the log prefix, and any given args and kwargs. Return any + return value. Raise ValueError if the hook name is unknown. Raise AttributeError if the function name is not found in the module. Raise anything else that the called function raises. ''' - config = hooks.get(hook_name) - if not config: - logger.debug('{}: No {} hook configured.'.format(log_prefix, hook_name)) - return + config = hooks.get(hook_name, {}) try: module = HOOK_NAME_TO_MODULE[hook_name] @@ -59,7 +54,7 @@ def call_hooks(function_name, hooks, log_prefix, hook_names, *args, **kwargs): configuration for that hook, the log prefix, and any given args and kwargs. Collect any return values into a dict from hook name to return value. - If the hook name is not present in the hooks configuration, then don't call the function for it, + If the hook name is not present in the hooks configuration, then don't call the function for it and omit it from the return values. Raise ValueError if the hook name is unknown. @@ -71,3 +66,19 @@ def call_hooks(function_name, hooks, log_prefix, hook_names, *args, **kwargs): for hook_name in hook_names if hooks.get(hook_name) } + + +def call_hooks_even_if_unconfigured(function_name, hooks, log_prefix, hook_names, *args, **kwargs): + ''' + Given the hooks configuration dict and a prefix to use in log entries, call the requested + function of the Python module corresponding to each given hook name. Supply each call with the + configuration for that hook, the log prefix, and any given args and kwargs. Collect any return + values into a dict from hook name to return value. + + Raise AttributeError if the function name is not found in the module. + Raise anything else that a called function raises. An error stops calls to subsequent functions. + ''' + return { + hook_name: call_hook(function_name, hooks, log_prefix, hook_name, *args, **kwargs) + for hook_name in hook_names + } diff --git a/borgmatic/hooks/dump.py b/borgmatic/hooks/dump.py index cbfe5fa..d4c8e3f 100644 --- a/borgmatic/hooks/dump.py +++ b/borgmatic/hooks/dump.py @@ -55,7 +55,7 @@ def remove_database_dumps(dump_path, database_type_name, log_prefix, dry_run): ''' dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' - logger.info( + logger.debug( '{}: Removing {} database dumps{}'.format(log_prefix, database_type_name, dry_run_label) ) diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index 64d75d3..d51e8d9 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -455,7 +455,8 @@ def test_run_actions_executes_and_calls_hooks_for_create_action(): flexmock(module.command).should_receive('execute_hook').times( 4 ) # Before/after extract and before/after actions. - flexmock(module.dispatch).should_receive('call_hooks').and_return({}).times(3) + flexmock(module.dispatch).should_receive('call_hooks').and_return({}) + flexmock(module.dispatch).should_receive('call_hooks_even_if_unconfigured').and_return({}) arguments = { 'global': flexmock(monitoring_verbosity=1, dry_run=False), 'create': flexmock( diff --git a/tests/unit/hooks/test_dispatch.py b/tests/unit/hooks/test_dispatch.py index 7224e19..a332109 100644 --- a/tests/unit/hooks/test_dispatch.py +++ b/tests/unit/hooks/test_dispatch.py @@ -27,13 +27,18 @@ def test_call_hook_invokes_module_function_with_arguments_and_returns_value(): assert return_value == expected_return_value -def test_call_hook_without_hook_config_skips_call(): +def test_call_hook_without_hook_config_invokes_module_function_with_arguments_and_returns_value(): hooks = {'other_hook': flexmock()} + expected_return_value = flexmock() test_module = sys.modules[__name__] flexmock(module).HOOK_NAME_TO_MODULE = {'super_hook': test_module} - flexmock(test_module).should_receive('hook_function').never() + flexmock(test_module).should_receive('hook_function').with_args( + {}, 'prefix', 55, value=66 + ).and_return(expected_return_value).once() - module.call_hook('hook_function', hooks, 'prefix', 'super_hook', 55, value=66) + return_value = module.call_hook('hook_function', hooks, 'prefix', 'super_hook', 55, value=66) + + assert return_value == expected_return_value def test_call_hook_without_corresponding_module_raises(): @@ -76,3 +81,31 @@ def test_call_hooks_calls_skips_return_values_for_null_hooks(): return_values = module.call_hooks('do_stuff', hooks, 'prefix', ('super_hook', 'other_hook'), 55) assert return_values == expected_return_values + + +def test_call_hooks_even_if_unconfigured_calls_each_hook_and_collects_return_values(): + hooks = {'super_hook': flexmock(), 'other_hook': flexmock()} + expected_return_values = {'super_hook': flexmock(), 'other_hook': flexmock()} + flexmock(module).should_receive('call_hook').and_return( + expected_return_values['super_hook'] + ).and_return(expected_return_values['other_hook']) + + return_values = module.call_hooks_even_if_unconfigured( + 'do_stuff', hooks, 'prefix', ('super_hook', 'other_hook'), 55 + ) + + assert return_values == expected_return_values + + +def test_call_hooks_even_if_unconfigured_calls_each_hook_configured_or_not_and_collects_return_values(): + hooks = {'other_hook': flexmock()} + expected_return_values = {'super_hook': flexmock(), 'other_hook': flexmock()} + flexmock(module).should_receive('call_hook').and_return( + expected_return_values['super_hook'] + ).and_return(expected_return_values['other_hook']) + + return_values = module.call_hooks_even_if_unconfigured( + 'do_stuff', hooks, 'prefix', ('super_hook', 'other_hook'), 55 + ) + + assert return_values == expected_return_values