From 2c70ad81ecb21b2a2be7cb49a4aa32e5849c3934 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Wed, 1 Jan 2025 15:00:36 -0800 Subject: [PATCH] Fix the "spot" check to support relative source directory paths (#960). Fix the "spot" check to no longer consider pipe files within an archive for file comparisons. Fix auto-excluding of special files (when databases are configured) to support relative source directory paths. --- NEWS | 8 ++- borgmatic/actions/check.py | 25 +++++--- borgmatic/actions/config/bootstrap.py | 2 +- borgmatic/actions/create.py | 12 +++- borgmatic/borg/create.py | 11 ++-- tests/unit/actions/test_check.py | 84 ++++++++++++++++++++------- tests/unit/actions/test_create.py | 19 ++++++ tests/unit/borg/test_create.py | 11 ++++ 8 files changed, 132 insertions(+), 40 deletions(-) diff --git a/NEWS b/NEWS index 7274ebde..702c7639 100644 --- a/NEWS +++ b/NEWS @@ -1,8 +1,12 @@ 1.9.6.dev0 - * #959: Fix an error in the Btrfs hook when a "/" subvolume is configured in borgmatic's source - directories. + * #959: Fix an error in the Btrfs hook when a subvolume mounted at "/" is configured in borgmatic's + source directories. * #960: Fix for archives storing relative source directory paths such that they contain the working directory. + * #960: Fix the "spot" check to support relative source directory paths. + * Fix the "spot" check to no longer consider pipe files within an archive for file comparisons. + * Fix auto-excluding of special files (when databases are configured) to support relative source + directory paths. * Drop support for Python 3.8, which has been end-of-lifed. 1.9.5 diff --git a/borgmatic/actions/check.py b/borgmatic/actions/check.py index 3a14affe..57c15269 100644 --- a/borgmatic/actions/check.py +++ b/borgmatic/actions/check.py @@ -409,6 +409,7 @@ def collect_spot_check_source_paths( BORG_DIRECTORY_FILE_TYPE = 'd' +BORG_PIPE_FILE_TYPE = 'p' def collect_spot_check_archive_paths( @@ -426,6 +427,9 @@ def collect_spot_check_archive_paths( local Borg version, global arguments as an argparse.Namespace instance, the local Borg path, the remote Borg path, and the borgmatic runtime directory, collect the paths from the given archive (but only include files and symlinks and exclude borgmatic runtime directories). + + These paths do not have a leading slash, as that's how Borg stores them. As a result, we don't + know whether they came from absolute or relative source directories. ''' borgmatic_source_directory = borgmatic.config.paths.get_borgmatic_source_directory(config) @@ -437,15 +441,17 @@ def collect_spot_check_archive_paths( config, local_borg_version, global_arguments, - path_format='{type} /{path}{NL}', # noqa: FS003 + path_format='{type} {path}{NL}', # noqa: FS003 local_path=local_path, remote_path=remote_path, ) for (file_type, path) in (line.split(' ', 1),) - if file_type != BORG_DIRECTORY_FILE_TYPE - if pathlib.Path('/borgmatic') not in pathlib.Path(path).parents - if pathlib.Path(borgmatic_source_directory) not in pathlib.Path(path).parents - if pathlib.Path(borgmatic_runtime_directory) not in pathlib.Path(path).parents + if file_type not in (BORG_DIRECTORY_FILE_TYPE, BORG_PIPE_FILE_TYPE) + if pathlib.Path('borgmatic') not in pathlib.Path(path).parents + if pathlib.Path(borgmatic_source_directory.lstrip(os.path.sep)) + not in pathlib.Path(path).parents + if pathlib.Path(borgmatic_runtime_directory.lstrip(os.path.sep)) + not in pathlib.Path(path).parents ) @@ -532,7 +538,7 @@ def compare_spot_check_hashes( local_borg_version, global_arguments, list_paths=source_sample_paths_subset, - path_format='{xxh64} /{path}{NL}', # noqa: FS003 + path_format='{xxh64} {path}{NL}', # noqa: FS003 local_path=local_path, remote_path=remote_path, ) @@ -544,7 +550,7 @@ def compare_spot_check_hashes( failing_paths = [] for path, source_hash in source_hashes.items(): - archive_hash = archive_hashes.get(path) + archive_hash = archive_hashes.get(path.lstrip(os.path.sep)) if archive_hash is not None and archive_hash == source_hash: continue @@ -626,11 +632,12 @@ def spot_check( count_delta_percentage = abs(len(source_paths) - len(archive_paths)) / len(source_paths) * 100 if count_delta_percentage > spot_check_config['count_tolerance_percentage']: + rootless_source_paths = set(path.lstrip(os.path.sep) for path in source_paths) logger.debug( - f'{log_prefix}: Paths in source paths but not latest archive: {", ".join(set(source_paths) - set(archive_paths)) or "none"}' + f'{log_prefix}: Paths in source paths but not latest archive: {", ".join(rootless_source_paths - set(archive_paths)) or "none"}' ) logger.debug( - f'{log_prefix}: Paths in latest archive but not source paths: {", ".join(set(archive_paths) - set(source_paths)) or "none"}' + f'{log_prefix}: Paths in latest archive but not source paths: {", ".join(set(archive_paths) - rootless_source_paths) or "none"}' ) raise ValueError( f'Spot check failed: {count_delta_percentage:.2f}% file count delta between source paths and latest archive (tolerance is {spot_check_config["count_tolerance_percentage"]}%)' diff --git a/borgmatic/actions/config/bootstrap.py b/borgmatic/actions/config/bootstrap.py index 17cf60f5..cf6b56d6 100644 --- a/borgmatic/actions/config/bootstrap.py +++ b/borgmatic/actions/config/bootstrap.py @@ -41,7 +41,7 @@ def get_config_paths(archive_name, bootstrap_arguments, global_arguments, local_ config = make_bootstrap_config(bootstrap_arguments) # Probe for the manifest file in multiple locations, as the default location has moved to the - # borgmatic runtime directory (which get stored as just "/borgmatic" with Borg 1.4+). But we + # borgmatic runtime directory (which gets stored as just "/borgmatic" with Borg 1.4+). But we # still want to support reading the manifest from previously created archives as well. with borgmatic.config.paths.Runtime_directory( {'user_runtime_directory': bootstrap_arguments.user_runtime_directory}, diff --git a/borgmatic/actions/create.py b/borgmatic/actions/create.py index 9e1d6f68..bceb25da 100644 --- a/borgmatic/actions/create.py +++ b/borgmatic/actions/create.py @@ -133,12 +133,15 @@ def pattern_root_directories(patterns=None): ] -def process_source_directories(config, source_directories=None): +def process_source_directories(config, source_directories=None, skip_expand_paths=None): ''' Given a sequence of source directories (either in the source_directories argument or, lacking that, from config), expand and deduplicate the source directories, returning the result. + + If any paths are given to skip, don't expand them. ''' working_directory = borgmatic.config.paths.get_working_directory(config) + skip_paths = set(skip_expand_paths or ()) if source_directories is None: source_directories = tuple(config.get('source_directories', ())) @@ -146,9 +149,10 @@ def process_source_directories(config, source_directories=None): return deduplicate_directories( map_directories_to_devices( expand_directories( - tuple(source_directories), + tuple(source for source in source_directories if source not in skip_paths), working_directory=working_directory, ) + + tuple(skip_paths) ), additional_directory_devices=map_directories_to_devices( expand_directories( @@ -220,7 +224,9 @@ def run_create( # Process source directories again in case any data source hooks updated them. Without this # step, we could end up with duplicate paths that cause Borg to hang when it tries to read # from the same named pipe twice. - source_directories = process_source_directories(config, source_directories) + source_directories = process_source_directories( + config, source_directories, skip_expand_paths=config_paths + ) stream_processes = [process for processes in active_dumps.values() for process in processes] json_output = borgmatic.borg.create.create_archive( diff --git a/borgmatic/borg/create.py b/borgmatic/borg/create.py index 5ed0cfaf..819a46b7 100644 --- a/borgmatic/borg/create.py +++ b/borgmatic/borg/create.py @@ -134,13 +134,14 @@ def make_list_filter_flags(local_borg_version, dry_run): return f'{base_flags}-' -def special_file(path): +def special_file(path, working_directory=None): ''' Return whether the given path is a special file (character device, block device, or named pipe - / FIFO). + / FIFO). If a working directory is given, take it into account when making the full path to + check. ''' try: - mode = os.stat(path).st_mode + mode = os.stat(os.path.join(working_directory or '', path)).st_mode except (FileNotFoundError, OSError): return False @@ -208,7 +209,9 @@ def collect_special_file_paths( f'The runtime directory {os.path.normpath(borgmatic_runtime_directory)} overlaps with the configured excludes. Please remove it from excludes or change the runtime directory.' ) - return tuple(path for path in paths if special_file(path) if path not in skip_paths) + return tuple( + path for path in paths if special_file(path, working_directory) if path not in skip_paths + ) def check_all_source_directories_exist(source_directories): diff --git a/tests/unit/actions/test_check.py b/tests/unit/actions/test_check.py index d41237c1..2bdb9689 100644 --- a/tests/unit/actions/test_check.py +++ b/tests/unit/actions/test_check.py @@ -725,15 +725,16 @@ def test_collect_spot_check_source_paths_skips_directories(): ) -def test_collect_spot_check_archive_paths_excludes_directories(): +def test_collect_spot_check_archive_paths_excludes_directories_and_pipes(): flexmock(module.borgmatic.config.paths).should_receive( 'get_borgmatic_source_directory' ).and_return('/home/user/.borgmatic') flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( ( - 'f /etc/path', - 'f /etc/other', - 'd /etc/dir', + 'f etc/path', + 'p var/pipe', + 'f etc/other', + 'd etc/dir', ) ) @@ -746,7 +747,7 @@ def test_collect_spot_check_archive_paths_excludes_directories(): local_path=flexmock(), remote_path=flexmock(), borgmatic_runtime_directory='/run/user/1001/borgmatic', - ) == ('/etc/path', '/etc/other') + ) == ('etc/path', 'etc/other') def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_directory_as_stored_with_prefix_truncation(): @@ -755,8 +756,8 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_dir ).and_return('/root/.borgmatic') flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( ( - 'f /etc/path', - 'f /borgmatic/some/thing', + 'f etc/path', + 'f borgmatic/some/thing', ) ) @@ -769,7 +770,7 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_dir local_path=flexmock(), remote_path=flexmock(), borgmatic_runtime_directory='/run/user/0/borgmatic', - ) == ('/etc/path',) + ) == ('etc/path',) def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_source_directory(): @@ -778,8 +779,8 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_source_dire ).and_return('/root/.borgmatic') flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( ( - 'f /etc/path', - 'f /root/.borgmatic/some/thing', + 'f etc/path', + 'f root/.borgmatic/some/thing', ) ) @@ -792,7 +793,7 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_source_dire local_path=flexmock(), remote_path=flexmock(), borgmatic_runtime_directory='/run/user/0/borgmatic', - ) == ('/etc/path',) + ) == ('etc/path',) def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_directory(): @@ -801,8 +802,8 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_dir ).and_return('/root.borgmatic') flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( ( - 'f /etc/path', - 'f /run/user/0/borgmatic/some/thing', + 'f etc/path', + 'f run/user/0/borgmatic/some/thing', ) ) @@ -815,7 +816,7 @@ def test_collect_spot_check_archive_paths_excludes_file_in_borgmatic_runtime_dir local_path=flexmock(), remote_path=flexmock(), borgmatic_runtime_directory='/run/user/0/borgmatic', - ) == ('/etc/path',) + ) == ('etc/path',) def test_collect_spot_check_source_paths_uses_working_directory(): @@ -877,7 +878,7 @@ def test_compare_spot_check_hashes_returns_paths_having_failing_hashes(): 'hash1 /foo\nhash2 /bar' ) flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['hash1 /foo', 'nothash2 /bar'] + ['hash1 foo', 'nothash2 bar'] ) assert module.compare_spot_check_hashes( @@ -904,6 +905,47 @@ def test_compare_spot_check_hashes_returns_paths_having_failing_hashes(): ) == ('/bar',) +def test_compare_spot_check_hashes_returns_relative_paths_having_failing_hashes(): + flexmock(module.random).should_receive('sample').replace_with( + lambda population, count: population[:count] + ) + flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return( + None, + ) + flexmock(module.os.path).should_receive('exists').and_return(True) + flexmock(module.borgmatic.execute).should_receive( + 'execute_command_and_capture_output' + ).with_args(('xxh64sum', 'foo', 'bar'), working_directory=None).and_return( + 'hash1 foo\nhash2 bar' + ) + flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( + ['hash1 foo', 'nothash2 bar'] + ) + + assert module.compare_spot_check_hashes( + repository={'path': 'repo'}, + archive='archive', + config={ + 'checks': [ + { + 'name': 'archives', + 'frequency': '2 weeks', + }, + { + 'name': 'spot', + 'data_sample_percentage': 50, + }, + ] + }, + local_borg_version=flexmock(), + global_arguments=flexmock(), + local_path=flexmock(), + remote_path=flexmock(), + log_prefix='repo', + source_paths=('foo', 'bar', 'baz', 'quux'), + ) == ('bar',) + + def test_compare_spot_check_hashes_handles_data_sample_percentage_above_100(): flexmock(module.random).should_receive('sample').replace_with( lambda population, count: population[:count] @@ -918,7 +960,7 @@ def test_compare_spot_check_hashes_handles_data_sample_percentage_above_100(): 'hash1 /foo\nhash2 /bar' ) flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['nothash1 /foo', 'nothash2 /bar'] + ['nothash1 foo', 'nothash2 bar'] ) assert module.compare_spot_check_hashes( @@ -959,7 +1001,7 @@ def test_compare_spot_check_hashes_uses_xxh64sum_command_option(): 'hash1 /foo\nhash2 /bar' ) flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['hash1 /foo', 'nothash2 /bar'] + ['hash1 foo', 'nothash2 bar'] ) assert module.compare_spot_check_hashes( @@ -997,7 +1039,7 @@ def test_compare_spot_check_hashes_considers_path_missing_from_archive_as_not_ma 'hash1 /foo\nhash2 /bar' ) flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['hash1 /foo'] + ['hash1 foo'] ) assert module.compare_spot_check_hashes( @@ -1033,7 +1075,7 @@ def test_compare_spot_check_hashes_considers_non_existent_path_as_not_matching() 'execute_command_and_capture_output' ).with_args(('xxh64sum', '/foo'), working_directory=None).and_return('hash1 /foo') flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['hash1 /foo', 'hash2 /bar'] + ['hash1 foo', 'hash2 bar'] ) assert module.compare_spot_check_hashes( @@ -1076,8 +1118,8 @@ def test_compare_spot_check_hashes_with_too_many_paths_feeds_them_to_commands_in 'hash3 /baz\nhash4 /quux' ) flexmock(module.borgmatic.borg.list).should_receive('capture_archive_listing').and_return( - ['hash1 /foo', 'hash2 /bar'] - ).and_return(['hash3 /baz', 'nothash4 /quux']) + ['hash1 foo', 'hash2 bar'] + ).and_return(['hash3 baz', 'nothash4 quux']) assert module.compare_spot_check_hashes( repository={'path': 'repo'}, diff --git a/tests/unit/actions/test_create.py b/tests/unit/actions/test_create.py index 47a09e9c..664296b3 100644 --- a/tests/unit/actions/test_create.py +++ b/tests/unit/actions/test_create.py @@ -205,6 +205,25 @@ def test_process_source_directories_prefers_source_directory_argument_to_config( ) == ('foo', 'bar') +def test_process_source_directories_skips_expand_for_requested_paths(): + flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return( + '/working' + ) + flexmock(module).should_receive('deduplicate_directories').and_return(('foo', 'bar')) + flexmock(module).should_receive('map_directories_to_devices').and_return({}) + flexmock(module).should_receive('expand_directories').with_args( + ('bar',), working_directory='/working' + ).and_return(()).once() + flexmock(module).should_receive('pattern_root_directories').and_return(()) + flexmock(module).should_receive('expand_directories').with_args( + (), working_directory='/working' + ).and_return(()) + + assert module.process_source_directories( + config={'source_directories': ['foo', 'bar']}, skip_expand_paths=('foo',) + ) == ('foo', 'bar') + + def test_run_create_executes_and_calls_hooks_for_configured_repository(): flexmock(module.logger).answer = lambda message: None flexmock(module.borgmatic.config.validate).should_receive('repositories_match').never() diff --git a/tests/unit/borg/test_create.py b/tests/unit/borg/test_create.py index b6c088fc..2929d866 100644 --- a/tests/unit/borg/test_create.py +++ b/tests/unit/borg/test_create.py @@ -258,6 +258,17 @@ def test_special_file_treats_broken_symlink_as_non_special(): assert module.special_file('/broken/symlink') is False +def test_special_file_prepends_relative_path_with_working_directory(): + flexmock(module.os).should_receive('stat').with_args('/working/dir/relative').and_return( + flexmock(st_mode=flexmock()) + ) + flexmock(module.stat).should_receive('S_ISCHR').and_return(False) + flexmock(module.stat).should_receive('S_ISBLK').and_return(False) + flexmock(module.stat).should_receive('S_ISFIFO').and_return(False) + + assert module.special_file('relative', '/working/dir') is False + + def test_any_parent_directories_treats_parents_as_match(): module.any_parent_directories('/foo/bar.txt', ('/foo', '/etc'))