From 48dca28c74918c28695dcdfeefed93b7958adc01 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Tue, 17 Dec 2024 11:00:19 -0800 Subject: [PATCH] When the ZFS, Btrfs, or LVM hooks aren't configured, don't try to cleanup snapshots for them. --- NEWS | 1 + borgmatic/hooks/data_source/bootstrap.py | 2 +- borgmatic/hooks/data_source/btrfs.py | 6 +++++- borgmatic/hooks/data_source/lvm.py | 7 +++++-- borgmatic/hooks/data_source/zfs.py | 7 +++++-- borgmatic/hooks/dispatch.py | 6 +++++- tests/unit/hooks/data_source/test_bootstrap.py | 12 ++++++------ tests/unit/hooks/data_source/test_btrfs.py | 18 ++++++++++++++++++ tests/unit/hooks/data_source/test_lvm.py | 17 +++++++++++++++++ tests/unit/hooks/data_source/test_zfs.py | 15 +++++++++++++++ tests/unit/hooks/test_dispatch.py | 2 +- 11 files changed, 79 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 61f9ac2e..925c8b28 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,6 @@ 1.9.5.dev0 * #954: Fix findmnt command error in the Btrfs hook by switching to parsing JSON output. + * When the ZFS, Btrfs, or LVM hooks aren't configured, don't try to cleanup snapshots for them. 1.9.4 * #80 (beta): Add an LVM hook for snapshotting and backing up LVM logical volumes. See the diff --git a/borgmatic/hooks/data_source/bootstrap.py b/borgmatic/hooks/data_source/bootstrap.py index 7ef209ab..3afb5987 100644 --- a/borgmatic/hooks/data_source/bootstrap.py +++ b/borgmatic/hooks/data_source/bootstrap.py @@ -34,7 +34,7 @@ def dump_data_sources( Return an empty sequence, since there are no ongoing dump processes from this hook. ''' - if hook_config.get('store_config_files') is False: + if hook_config and hook_config.get('store_config_files') is False: return [] borgmatic_manifest_path = os.path.join( diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py index 8c7d24a9..25295460 100644 --- a/borgmatic/hooks/data_source/btrfs.py +++ b/borgmatic/hooks/data_source/btrfs.py @@ -266,8 +266,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_ ''' Given a Btrfs configuration dict, a configuration dict, a log prefix, the borgmatic runtime directory, and whether this is a dry run, delete any Btrfs snapshots created by borgmatic. Use - the log prefix in any log entries. If this is a dry run, then don't actually remove anything. + the log prefix in any log entries. If this is a dry run or Btrfs isn't configured in borgmatic's + configuration, then don't actually remove anything. ''' + if hook_config is None: + return + dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' btrfs_command = hook_config.get('btrfs_command', 'btrfs') diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py index f0533f56..ece03a97 100644 --- a/borgmatic/hooks/data_source/lvm.py +++ b/borgmatic/hooks/data_source/lvm.py @@ -288,9 +288,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_ ''' Given an LVM configuration dict, a configuration dict, a log prefix, the borgmatic runtime directory, and whether this is a dry run, unmount and delete any LVM snapshots created by - borgmatic. Use the log prefix in any log entries. If this is a dry run, then don't actually - remove anything. + borgmatic. Use the log prefix in any log entries. If this is a dry run or LVM isn't configured + in borgmatic's configuration, then don't actually remove anything. ''' + if hook_config is None: + return + dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' # Unmount snapshots. diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py index da35a256..0c8a24b3 100644 --- a/borgmatic/hooks/data_source/zfs.py +++ b/borgmatic/hooks/data_source/zfs.py @@ -283,9 +283,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_ ''' Given a ZFS configuration dict, a configuration dict, a log prefix, the borgmatic runtime directory, and whether this is a dry run, unmount and destroy any ZFS snapshots created by - borgmatic. Use the log prefix in any log entries. If this is a dry run, then don't actually - remove anything. + borgmatic. Use the log prefix in any log entries. If this is a dry run or ZFS isn't configured + in borgmatic's configuration, then don't actually remove anything. ''' + if hook_config is None: + return + dry_run_label = ' (dry run; not actually removing anything)' if dry_run else '' # Unmount snapshots. diff --git a/borgmatic/hooks/dispatch.py b/borgmatic/hooks/dispatch.py index b716846c..d3c828c4 100644 --- a/borgmatic/hooks/dispatch.py +++ b/borgmatic/hooks/dispatch.py @@ -32,7 +32,11 @@ def call_hook(function_name, config, log_prefix, hook_name, *args, **kwargs): Raise AttributeError if the function name is not found in the module. Raise anything else that the called function raises. ''' - hook_config = config.get(hook_name) or config.get(f'{hook_name}_databases') or {} + if hook_name in config or f'{hook_name}_databases' in config: + hook_config = config.get(hook_name) or config.get(f'{hook_name}_databases') or {} + else: + hook_config = None + module_name = hook_name.split('_databases')[0] # Probe for a data source or monitoring hook module corresponding to the hook name. diff --git a/tests/unit/hooks/data_source/test_bootstrap.py b/tests/unit/hooks/data_source/test_bootstrap.py index 62730883..86267cb7 100644 --- a/tests/unit/hooks/data_source/test_bootstrap.py +++ b/tests/unit/hooks/data_source/test_bootstrap.py @@ -22,7 +22,7 @@ def test_dump_data_sources_creates_manifest_file(): ).once() module.dump_data_sources( - hook_config={}, + hook_config=None, config={}, log_prefix='test', config_paths=('test.yaml',), @@ -53,7 +53,7 @@ def test_dump_data_sources_with_dry_run_does_not_create_manifest_file(): flexmock(module.json).should_receive('dump').never() module.dump_data_sources( - hook_config={}, + hook_config=None, config={}, log_prefix='test', config_paths=('test.yaml',), @@ -74,7 +74,7 @@ def test_remove_data_source_dumps_deletes_manifest_and_parent_directory(): flexmock(module.os).should_receive('rmdir').with_args('/run/borgmatic/bootstrap').once() module.remove_data_source_dumps( - hook_config={}, + hook_config=None, config={}, log_prefix='test', borgmatic_runtime_directory='/run/borgmatic', @@ -91,7 +91,7 @@ def test_remove_data_source_dumps_with_dry_run_bails(): flexmock(module.os).should_receive('rmdir').never() module.remove_data_source_dumps( - hook_config={}, + hook_config=None, config={}, log_prefix='test', borgmatic_runtime_directory='/run/borgmatic', @@ -110,7 +110,7 @@ def test_remove_data_source_dumps_swallows_manifest_file_not_found_error(): flexmock(module.os).should_receive('rmdir').with_args('/run/borgmatic/bootstrap').once() module.remove_data_source_dumps( - hook_config={}, + hook_config=None, config={}, log_prefix='test', borgmatic_runtime_directory='/run/borgmatic', @@ -131,7 +131,7 @@ def test_remove_data_source_dumps_swallows_manifest_parent_directory_not_found_e ).once() module.remove_data_source_dumps( - hook_config={}, + hook_config=None, config={}, log_prefix='test', borgmatic_runtime_directory='/run/borgmatic', diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py index 02909c6a..35d78b2c 100644 --- a/tests/unit/hooks/data_source/test_btrfs.py +++ b/tests/unit/hooks/data_source/test_btrfs.py @@ -485,6 +485,24 @@ def test_remove_data_source_dumps_deletes_snapshots(): ) +def test_remove_data_source_dumps_without_hook_configuration_bails(): + flexmock(module).should_receive('get_subvolumes').never() + flexmock(module).should_receive('make_snapshot_path').never() + flexmock(module.borgmatic.config.paths).should_receive( + 'replace_temporary_subdirectory_with_glob' + ).never() + flexmock(module).should_receive('delete_snapshot').never() + flexmock(module.shutil).should_receive('rmtree').never() + + module.remove_data_source_dumps( + hook_config=None, + config={'source_directories': '/mnt/subvolume'}, + log_prefix='test', + borgmatic_runtime_directory='/run/borgmatic', + dry_run=False, + ) + + def test_remove_data_source_dumps_with_get_subvolumes_file_not_found_error_bails(): config = {'btrfs': {}} flexmock(module).should_receive('get_subvolumes').and_raise(FileNotFoundError) diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py index b11bb1b9..37fc13db 100644 --- a/tests/unit/hooks/data_source/test_lvm.py +++ b/tests/unit/hooks/data_source/test_lvm.py @@ -673,6 +673,23 @@ def test_remove_data_source_dumps_unmounts_and_remove_snapshots(): ) +def test_remove_data_source_dumps_bails_for_missing_lvm_configuration(): + flexmock(module).should_receive('get_logical_volumes').never() + flexmock(module.borgmatic.config.paths).should_receive( + 'replace_temporary_subdirectory_with_glob' + ).never() + flexmock(module).should_receive('unmount_snapshot').never() + flexmock(module).should_receive('remove_snapshot').never() + + module.remove_data_source_dumps( + hook_config=None, + config={'source_directories': '/mnt/lvolume'}, + log_prefix='test', + borgmatic_runtime_directory='/run/borgmatic', + dry_run=False, + ) + + def test_remove_data_source_dumps_bails_for_missing_lsblk_command(): config = {'lvm': {}} flexmock(module).should_receive('get_logical_volumes').and_raise(FileNotFoundError) diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py index afd0270e..f90df123 100644 --- a/tests/unit/hooks/data_source/test_zfs.py +++ b/tests/unit/hooks/data_source/test_zfs.py @@ -321,6 +321,21 @@ def test_remove_data_source_dumps_use_custom_commands(): ) +def test_remove_data_source_dumps_bails_for_missing_hook_configuration(): + flexmock(module).should_receive('get_all_dataset_mount_points').never() + flexmock(module.borgmatic.config.paths).should_receive( + 'replace_temporary_subdirectory_with_glob' + ).never() + + module.remove_data_source_dumps( + hook_config=None, + config={'source_directories': '/mnt/dataset'}, + log_prefix='test', + borgmatic_runtime_directory='/run/borgmatic', + dry_run=False, + ) + + def test_remove_data_source_dumps_bails_for_missing_zfs_command(): flexmock(module).should_receive('get_all_dataset_mount_points').and_raise(FileNotFoundError) flexmock(module.borgmatic.config.paths).should_receive( diff --git a/tests/unit/hooks/test_dispatch.py b/tests/unit/hooks/test_dispatch.py index 08bec40f..619b4366 100644 --- a/tests/unit/hooks/test_dispatch.py +++ b/tests/unit/hooks/test_dispatch.py @@ -95,7 +95,7 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an 'borgmatic.hooks.monitoring.super_hook' ).and_return(test_module) flexmock(test_module).should_receive('hook_function').with_args( - {}, config, 'prefix', 55, value=66 + None, config, 'prefix', 55, value=66 ).and_return(expected_return_value).once() return_value = module.call_hook('hook_function', config, 'prefix', 'super_hook', 55, value=66)