From a573e606a540c577b09649530811535b941d9862 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sat, 12 Apr 2025 15:02:47 -0700 Subject: [PATCH] Add a "states" option to command hooks, so you can optionally skip an "after" hook if borgmatic encounters an error (#1066). --- NEWS | 2 + borgmatic/commands/borgmatic.py | 13 ++- borgmatic/config/schema.yaml | 20 +++++ borgmatic/hooks/command.py | 25 +++--- ...reparation-and-cleanup-steps-to-backups.md | 36 +++++--- tests/unit/commands/test_borgmatic.py | 81 ++++++++++++++--- tests/unit/hooks/test_command.py | 90 +++++++++++++++---- 7 files changed, 215 insertions(+), 52 deletions(-) diff --git a/NEWS b/NEWS index ddf12237..5531d48d 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ 2.0.3.dev0 * #1065: Fix a regression in monitoring hooks in which an error pinged the finish state instead of the fail state. + * #1066: Add a "states" option to command hooks, so you can optionally skip an "after" hook if + borgmatic encounters an error. 2.0.2 * #1035: Document potential performance issues and workarounds with the ZFS, Btrfs, and LVM hooks: diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index d5415f8b..593a8873 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -301,7 +301,10 @@ def run_configuration(config_filename, config, config_paths, arguments): try: command.execute_hooks( command.filter_hooks( - config.get('commands'), after='error', action_names=arguments.keys() + config.get('commands'), + after='error', + action_names=arguments.keys(), + state_names=['fail'], ), config.get('umask'), borgmatic.config.paths.get_working_directory(config), @@ -907,6 +910,7 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log # Execute the actions corresponding to each configuration file. json_results = [] + encountered_error = False for config_filename, config in configs.items(): with Log_prefix(config_filename): @@ -917,6 +921,7 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log ) if error_logs: + encountered_error = True yield from log_error_records('An error occurred') yield from error_logs else: @@ -939,6 +944,7 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log local_path=get_local_path(configs), ) except (CalledProcessError, OSError) as error: + encountered_error = True yield from log_error_records('Error unmounting mount point', error) if json_results: @@ -948,7 +954,10 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log for config_filename, config in configs.items(): command.execute_hooks( command.filter_hooks( - config.get('commands'), after='everything', action_names=arguments.keys() + config.get('commands'), + after='everything', + action_names=arguments.keys(), + state_names=['fail' if encountered_error else 'finish'], ), config.get('umask'), borgmatic.config.paths.get_working_directory(config), diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index f99a8ee2..0216833b 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -1225,6 +1225,26 @@ properties: particular actions listed here. Defaults to running for all actions. example: [create, prune, compact, check] + states: + type: array + items: + type: string + enum: + - finish + - fail + description: | + Only trigger the hook if borgmatic encounters one + of the states (execution results) listed here, + where: + * "finish": No errors occurred. + * "fail": An error occurred. + This state is evaluated only for the scope of the + configured "action", "repository", etc., rather + than for the entire borgmatic run. Only available + for "after" hooks. Defaults to running the hook + for all states. + example: + - finish run: type: array items: diff --git a/borgmatic/hooks/command.py b/borgmatic/hooks/command.py index 83eb9241..e0843da6 100644 --- a/borgmatic/hooks/command.py +++ b/borgmatic/hooks/command.py @@ -47,21 +47,25 @@ def make_environment(current_environment, sys_module=sys): return environment -def filter_hooks(command_hooks, before=None, after=None, hook_name=None, action_names=None): +def filter_hooks(command_hooks, before=None, after=None, action_names=None, state_names=None): ''' Given a sequence of command hook dicts from configuration and one or more filters (before name, - after name, calling hook name, or a sequence of action names), filter down the command hooks to - just the ones that match the given filters. + after name, a sequence of action names, and/or a sequence of execution result state names), + filter down the command hooks to just the ones that match the given filters. ''' return tuple( hook_config for hook_config in command_hooks or () for config_action_names in (hook_config.get('when'),) + for config_state_names in (hook_config.get('states'),) if before is None or hook_config.get('before') == before if after is None or hook_config.get('after') == after if action_names is None or config_action_names is None or set(config_action_names or ()).intersection(set(action_names)) + if state_names is None + or config_state_names is None + or set(config_state_names or ()).intersection(set(state_names)) ) @@ -143,7 +147,7 @@ class Before_after_hooks: before_after='do_stuff', umask=config.get('umask'), dry_run=dry_run, - hook_name='myhook', + action_names=['create'], ): do() some() @@ -160,22 +164,20 @@ class Before_after_hooks: umask, working_directory, dry_run, - hook_name=None, action_names=None, **context, ): ''' Given a sequence of command hook configuration dicts, the before/after name, a umask to run - commands with, a working directory to run commands with, a dry run flag, the name of the - calling hook, a sequence of action names, and any context for the executed commands, save - those data points for use below. + commands with, a working directory to run commands with, a dry run flag, a sequence of + action names, and any context for the executed commands, save those data points for use + below. ''' self.command_hooks = command_hooks self.before_after = before_after self.umask = umask self.working_directory = working_directory self.dry_run = dry_run - self.hook_name = hook_name self.action_names = action_names self.context = context @@ -188,7 +190,6 @@ class Before_after_hooks: borgmatic.hooks.command.filter_hooks( self.command_hooks, before=self.before_after, - hook_name=self.hook_name, action_names=self.action_names, ), self.umask, @@ -202,7 +203,7 @@ class Before_after_hooks: # Trigger the after hook manually, since raising here will prevent it from being run # otherwise. - self.__exit__(None, None, None) + self.__exit__(exception_type=type(error), exception=error, traceback=None) raise ValueError(f'Error running before {self.before_after} hook: {error}') @@ -215,8 +216,8 @@ class Before_after_hooks: borgmatic.hooks.command.filter_hooks( self.command_hooks, after=self.before_after, - hook_name=self.hook_name, action_names=self.action_names, + state_names=['fail' if exception_type else 'finish'], ), self.umask, self.working_directory, diff --git a/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md b/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md index 5eceba2d..505ab0cd 100644 --- a/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md +++ b/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md @@ -55,6 +55,29 @@ commands: - "echo Backup: start" ``` +By default, an `after` command hook runs even if an error occurs in the +corresponding `before` hook or between those two hooks. This allows you to +perform cleanup steps that correspond to `before` preparation commands—even when +something goes wrong. You may notice that this is a departure from the way that +the deprecated `after_*` hooks worked in borgmatic prior to version 2.0.0. + +New in version 2.0.3 You can +customize this behavior with the `states` option. For instance, here's an +example of an `after` hook that only triggers on success and not on error: + +```yaml +commands: + - after: action + when: [create] + states: [finish] + run: + - echo "After successful create!" +``` + +Additionally, when command hooks run, they respect the `working_directory` +option if it is configured, meaning that the hook commands are run in that +directory. + Each command in the `commands:` list has the following options: * `before` or `after`: Name for the point in borgmatic's execution that the commands should be run before or after, one of: @@ -64,18 +87,11 @@ Each command in the `commands:` list has the following options: * `everything` runs before or after all configuration files. Errors here do not trigger `error` hooks or the `fail` state in monitoring hooks. This replaces the deprecated `before_everything` and `after_everything`. * `error` runs after an error occurs—and it's only available for `after`. This replaces the deprecated `on_error` hook. * `when`: Only trigger the hook when borgmatic is run with particular actions (`create`, `prune`, etc.) listed here. Defaults to running for all actions. + * `states`: New in version 2.0.3 Only trigger the hook if borgmatic encounters one of the states (execution results) listed here. This state is evaluated only for the scope of the configured `action`, `repository`, etc., rather than for the entire borgmatic run. Only available for `after` hooks. Defaults to running the hook for all states. One or more of: + * `finish`: No errors occurred. + * `fail`: An error occurred. * `run`: List of one or more shell commands or scripts to run when this command hook is triggered. -An `after` command hook runs even if an error occurs in the corresponding -`before` hook or between those two hooks. This allows you to perform cleanup -steps that correspond to `before` preparation commands—even when something goes -wrong. This is a departure from the way that the deprecated `after_*` hooks -worked in borgmatic prior to version 2.0.0. - -Additionally, when command hooks run, they respect the `working_directory` -option if it is configured, meaning that the hook commands are run in that -directory. - ### Order of execution diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index 85a5ceea..b2525076 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -1932,7 +1932,12 @@ def test_collect_highlander_action_summary_logs_error_on_run_validate_failure(): def test_collect_configuration_run_summary_logs_info_for_success(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -1952,7 +1957,12 @@ def test_collect_configuration_run_summary_logs_info_for_success(): def test_collect_configuration_run_summary_executes_hooks_for_create(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -1975,7 +1985,12 @@ def test_collect_configuration_run_summary_executes_hooks_for_create(): def test_collect_configuration_run_summary_logs_info_for_success_with_extract(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2018,7 +2033,12 @@ def test_collect_configuration_run_summary_logs_extract_with_repository_error(): def test_collect_configuration_run_summary_logs_info_for_success_with_mount(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2064,7 +2084,12 @@ def test_collect_configuration_run_summary_logs_mount_with_repository_error(): def test_collect_configuration_run_summary_logs_missing_configs_error(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['fail'] + ) flexmock(module.command).should_receive('execute_hooks') arguments = {'global': flexmock(config_paths=[])} expected_logs = (flexmock(),) @@ -2079,9 +2104,14 @@ def test_collect_configuration_run_summary_logs_missing_configs_error(): assert logs == expected_logs -def test_collect_configuration_run_summary_logs_pre_hook_error(): +def test_collect_configuration_run_summary_logs_before_hook_error(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['fail'] + ) flexmock(module.command).should_receive('execute_hooks').and_raise(ValueError) expected_logs = (flexmock(),) flexmock(module).should_receive('log_error_records').and_return(expected_logs) @@ -2102,9 +2132,14 @@ def test_collect_configuration_run_summary_logs_pre_hook_error(): assert logs == expected_logs -def test_collect_configuration_run_summary_logs_post_hook_error(): +def test_collect_configuration_run_summary_logs_after_hook_error(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks').and_return(None).and_raise(ValueError) flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2152,7 +2187,12 @@ def test_collect_configuration_run_summary_logs_for_list_with_archive_and_reposi def test_collect_configuration_run_summary_logs_info_for_success_with_list(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2175,7 +2215,12 @@ def test_collect_configuration_run_summary_logs_info_for_success_with_list(): def test_collect_configuration_run_summary_logs_run_configuration_error_logs(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['fail'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return( @@ -2198,7 +2243,12 @@ def test_collect_configuration_run_summary_logs_run_configuration_error_logs(): def test_collect_configuration_run_summary_logs_run_umount_error(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['fail'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2225,7 +2275,12 @@ def test_collect_configuration_run_summary_logs_run_umount_error(): def test_collect_configuration_run_summary_logs_outputs_merged_json_results(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') - flexmock(module.command).should_receive('filter_hooks') + flexmock(module.command).should_receive('filter_hooks').with_args( + object, before='everything', action_names=object + ) + flexmock(module.command).should_receive('filter_hooks').with_args( + object, after='everything', action_names=object, state_names=['finish'] + ) flexmock(module.command).should_receive('execute_hooks') flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return(['foo', 'bar']).and_return( diff --git a/tests/unit/hooks/test_command.py b/tests/unit/hooks/test_command.py index acfcd7d1..667e6ab3 100644 --- a/tests/unit/hooks/test_command.py +++ b/tests/unit/hooks/test_command.py @@ -167,6 +167,44 @@ def test_make_environment_with_pyinstaller_and_LD_LIBRARY_PATH_ORIG_copies_it_in }, ), ), + ( + ( + { + 'before': 'action', + 'states': ['finish'], # Not actually valid; only valid for "after". + 'run': ['foo'], + }, + { + 'after': 'action', + 'run': ['bar'], + }, + { + 'after': 'action', + 'states': ['finish'], + 'run': ['baz'], + }, + { + 'after': 'action', + 'states': ['fail'], + 'run': ['quux'], + }, + ), + { + 'after': 'action', + 'state_names': ['finish'], + }, + ( + { + 'after': 'action', + 'run': ['bar'], + }, + { + 'after': 'action', + 'states': ['finish'], + 'run': ['baz'], + }, + ), + ), ), ) def test_filter_hooks(hooks, filters, expected_hooks): @@ -336,14 +374,13 @@ def test_before_after_hooks_calls_command_hooks(): flexmock(module).should_receive('filter_hooks').with_args( commands, before='action', - hook_name='myhook', action_names=['create'], ).and_return(flexmock()).once() flexmock(module).should_receive('filter_hooks').with_args( commands, after='action', - hook_name='myhook', action_names=['create'], + state_names=['finish'], ).and_return(flexmock()).once() flexmock(module).should_receive('execute_hooks').twice() @@ -353,7 +390,6 @@ def test_before_after_hooks_calls_command_hooks(): umask=1234, working_directory='/working', dry_run=False, - hook_name='myhook', action_names=['create'], context1='stuff', context2='such', @@ -369,14 +405,13 @@ def test_before_after_hooks_with_before_error_runs_after_hook_and_raises(): flexmock(module).should_receive('filter_hooks').with_args( commands, before='action', - hook_name='myhook', action_names=['create'], ).and_return(flexmock()).once() flexmock(module).should_receive('filter_hooks').with_args( commands, after='action', - hook_name='myhook', action_names=['create'], + state_names=['fail'], ).and_return(flexmock()).once() flexmock(module).should_receive('execute_hooks').and_raise(OSError).and_return(None) flexmock(module).should_receive('considered_soft_failure').and_return(False) @@ -388,7 +423,6 @@ def test_before_after_hooks_with_before_error_runs_after_hook_and_raises(): umask=1234, working_directory='/working', dry_run=False, - hook_name='myhook', action_names=['create'], context1='stuff', context2='such', @@ -404,14 +438,13 @@ def test_before_after_hooks_with_before_soft_failure_raises(): flexmock(module).should_receive('filter_hooks').with_args( commands, before='action', - hook_name='myhook', action_names=['create'], ).and_return(flexmock()).once() flexmock(module).should_receive('filter_hooks').with_args( commands, after='action', - hook_name='myhook', action_names=['create'], + state_names=['finish'], ).never() flexmock(module).should_receive('execute_hooks').and_raise(OSError) flexmock(module).should_receive('considered_soft_failure').and_return(True) @@ -423,7 +456,6 @@ def test_before_after_hooks_with_before_soft_failure_raises(): umask=1234, working_directory='/working', dry_run=False, - hook_name='myhook', action_names=['create'], context1='stuff', context2='such', @@ -431,6 +463,38 @@ def test_before_after_hooks_with_before_soft_failure_raises(): pass +def test_before_after_hooks_with_wrapped_code_error_runs_after_hook_and_raises(): + commands = [ + {'before': 'repository', 'run': ['foo', 'bar']}, + {'after': 'repository', 'run': ['baz']}, + ] + flexmock(module).should_receive('filter_hooks').with_args( + commands, + before='action', + action_names=['create'], + ).and_return(flexmock()).once() + flexmock(module).should_receive('filter_hooks').with_args( + commands, + after='action', + action_names=['create'], + state_names=['fail'], + ).and_return(flexmock()).once() + flexmock(module).should_receive('execute_hooks').twice() + + with pytest.raises(ValueError): + with module.Before_after_hooks( + command_hooks=commands, + before_after='action', + umask=1234, + working_directory='/working', + dry_run=False, + action_names=['create'], + context1='stuff', + context2='such', + ): + raise ValueError() + + def test_before_after_hooks_with_after_error_raises(): commands = [ {'before': 'repository', 'run': ['foo', 'bar']}, @@ -439,14 +503,13 @@ def test_before_after_hooks_with_after_error_raises(): flexmock(module).should_receive('filter_hooks').with_args( commands, before='action', - hook_name='myhook', action_names=['create'], ).and_return(flexmock()).once() flexmock(module).should_receive('filter_hooks').with_args( commands, after='action', - hook_name='myhook', action_names=['create'], + state_names=['finish'], ).and_return(flexmock()).once() flexmock(module).should_receive('execute_hooks').and_return(None).and_raise(OSError) flexmock(module).should_receive('considered_soft_failure').and_return(False) @@ -458,7 +521,6 @@ def test_before_after_hooks_with_after_error_raises(): umask=1234, working_directory='/working', dry_run=False, - hook_name='myhook', action_names=['create'], context1='stuff', context2='such', @@ -474,14 +536,13 @@ def test_before_after_hooks_with_after_soft_failure_raises(): flexmock(module).should_receive('filter_hooks').with_args( commands, before='action', - hook_name='myhook', action_names=['create'], ).and_return(flexmock()).once() flexmock(module).should_receive('filter_hooks').with_args( commands, after='action', - hook_name='myhook', action_names=['create'], + state_names=['finish'], ).and_return(flexmock()).once() flexmock(module).should_receive('execute_hooks').and_return(None).and_raise(OSError) flexmock(module).should_receive('considered_soft_failure').and_return(True) @@ -493,7 +554,6 @@ def test_before_after_hooks_with_after_soft_failure_raises(): umask=1234, working_directory='/working', dry_run=False, - hook_name='myhook', action_names=['create'], context1='stuff', context2='such',