diff --git a/NEWS b/NEWS index 65032279..4d25f9dc 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,9 @@ * #987: Fix a "list" action error when the "encryption_passcommand" option is set. * #987: When both "encryption_passcommand" and "encryption_passphrase" are configured, prefer "encryption_passphrase" even if it's an empty value. + * #988: With the "max_duration" option or the "--max-duration" flag, run the archives and + repository checks separately so they don't interfere with one another. Previously, borgmatic + refused to run checks in this situation. * #989: Fix the log message code to avoid using Python 3.10+ logging features. Now borgmatic will work with Python 3.9 again. diff --git a/borgmatic/borg/check.py b/borgmatic/borg/check.py index ee26c923..4350b8e4 100644 --- a/borgmatic/borg/check.py +++ b/borgmatic/borg/check.py @@ -64,15 +64,11 @@ def make_check_name_flags(checks, archive_filter_flags): ('--repository-only',) - However, if both "repository" and "archives" are in checks, then omit them from the returned - flags because Borg does both checks by default. If "data" is in checks, that implies "archives". + However, if both "repository" and "archives" are in checks, then omit the "only" flags from the + returned flags because Borg does both checks by default. Note that a "data" check only works + along with an "archives" check. ''' - if 'data' in checks: - data_flags = ('--verify-data',) - checks.update({'archives'}) - else: - data_flags = () - + data_flags = ('--verify-data',) if 'data' in checks else () common_flags = (archive_filter_flags if 'archives' in checks else ()) + data_flags if {'repository', 'archives'}.issubset(checks): @@ -142,49 +138,50 @@ def check_archives( except StopIteration: repository_check_config = {} - if check_arguments.max_duration and 'archives' in checks: - raise ValueError('The archives check cannot run when the --max-duration flag is used') - if repository_check_config.get('max_duration') and 'archives' in checks: - raise ValueError( - 'The archives check cannot run when the repository check has the max_duration option set' - ) - max_duration = check_arguments.max_duration or repository_check_config.get('max_duration') + umask = config.get('umask') - borg_exit_codes = config.get('borg_exit_codes') - - full_command = ( - (local_path, 'check') - + (('--repair',) if check_arguments.repair else ()) - + (('--max-duration', str(max_duration)) if max_duration else ()) - + make_check_name_flags(checks, archive_filter_flags) - + (('--remote-path', remote_path) if remote_path else ()) - + (('--umask', str(umask)) if umask else ()) - + (('--log-json',) if global_arguments.log_json else ()) - + (('--lock-wait', str(lock_wait)) if lock_wait else ()) - + verbosity_flags - + (('--progress',) if check_arguments.progress else ()) - + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ()) - + flags.make_repository_flags(repository_path, local_borg_version) - ) - working_directory = borgmatic.config.paths.get_working_directory(config) - # The Borg repair option triggers an interactive prompt, which won't work when output is - # captured. And progress messes with the terminal directly. - if check_arguments.repair or check_arguments.progress: - execute_command( - full_command, - output_file=DO_NOT_CAPTURE, - extra_environment=environment.make_environment(config), - working_directory=working_directory, - borg_local_path=local_path, - borg_exit_codes=borg_exit_codes, + if 'data' in checks: + checks.add('archives') + + grouped_checks = (checks,) + + # If max_duration is set, then archives and repository checks need to be run separately, as Borg + # doesn't support --max-duration along with an archives checks. + if max_duration and 'archives' in checks and 'repository' in checks: + checks.remove('repository') + grouped_checks = (checks, {'repository'}) + + for checks_subset in grouped_checks: + full_command = ( + (local_path, 'check') + + (('--repair',) if check_arguments.repair else ()) + + ( + ('--max-duration', str(max_duration)) + if max_duration and 'repository' in checks_subset + else () + ) + + make_check_name_flags(checks_subset, archive_filter_flags) + + (('--remote-path', remote_path) if remote_path else ()) + + (('--umask', str(umask)) if umask else ()) + + (('--log-json',) if global_arguments.log_json else ()) + + (('--lock-wait', str(lock_wait)) if lock_wait else ()) + + verbosity_flags + + (('--progress',) if check_arguments.progress else ()) + + (tuple(extra_borg_options.split(' ')) if extra_borg_options else ()) + + flags.make_repository_flags(repository_path, local_borg_version) ) - else: + execute_command( full_command, + # The Borg repair option triggers an interactive prompt, which won't work when output is + # captured. And progress messes with the terminal directly. + output_file=( + DO_NOT_CAPTURE if check_arguments.repair or check_arguments.progress else None + ), extra_environment=environment.make_environment(config), working_directory=working_directory, borg_local_path=local_path, diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml index c7bce20e..33a6bbe2 100644 --- a/borgmatic/config/schema.yaml +++ b/borgmatic/config/schema.yaml @@ -632,8 +632,8 @@ properties: long-running repository check into multiple partial checks. Defaults to no interruption. Only applies to the "repository" check, does not check - the repository index, and is not compatible with a - simultaneous "archives" check or "--repair" flag. + the repository index and is not compatible with + the "--repair" flag. example: 3600 - required: - name diff --git a/tests/unit/borg/test_check.py b/tests/unit/borg/test_check.py index 1ff43313..3fadc2fd 100644 --- a/tests/unit/borg/test_check.py +++ b/tests/unit/borg/test_check.py @@ -8,13 +8,16 @@ from borgmatic.borg import check as module from ..test_verbosity import insert_logging_mock -def insert_execute_command_mock(command, working_directory=None, borg_exit_codes=None): +def insert_execute_command_mock( + command, output_file=None, working_directory=None, borg_exit_codes=None +): flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return( working_directory, ) flexmock(module).should_receive('execute_command').with_args( command, + output_file=output_file, extra_environment=None, working_directory=working_directory, borg_local_path=command[0], @@ -250,11 +253,11 @@ def test_make_check_name_flags_without_archives_check_and_with_archive_filter_fl assert flags == ('--repository-only',) -def test_make_check_name_flags_with_data_check_returns_flag_and_implies_archives(): +def test_make_check_name_flags_with_archives_and_data_check_returns_verify_data_flag(): flexmock(module.feature).should_receive('available').and_return(True) flexmock(module.flags).should_receive('make_match_archives_flags').and_return(()) - flags = module.make_check_name_flags({'data'}, ()) + flags = module.make_check_name_flags({'archives', 'data'}, ()) assert flags == ( '--archives-only', @@ -262,6 +265,15 @@ def test_make_check_name_flags_with_data_check_returns_flag_and_implies_archives ) +def test_make_check_name_flags_with_repository_and_data_check_returns_verify_data_flag(): + flexmock(module.feature).should_receive('available').and_return(True) + flexmock(module.flags).should_receive('make_match_archives_flags').and_return(()) + + flags = module.make_check_name_flags({'archives', 'data', 'repository'}, ()) + + assert flags == ('--verify-data',) + + def test_make_check_name_flags_with_extract_omits_extract_flag(): flexmock(module.feature).should_receive('available').and_return(True) flexmock(module.flags).should_receive('make_match_archives_flags').and_return(()) @@ -271,21 +283,6 @@ def test_make_check_name_flags_with_extract_omits_extract_flag(): assert flags == () -def test_make_check_name_flags_with_repository_and_data_checks_does_not_return_repository_only(): - flexmock(module.feature).should_receive('available').and_return(True) - flexmock(module.flags).should_receive('make_match_archives_flags').and_return(()) - - flags = module.make_check_name_flags( - { - 'repository', - 'data', - }, - (), - ) - - assert flags == ('--verify-data',) - - def test_get_repository_id_with_valid_json_does_not_raise(): config = {} flexmock(module.repo_info).should_receive('display_repository_info').and_return( @@ -336,7 +333,9 @@ def test_get_repository_id_with_missing_json_keys_raises(): def test_check_archives_with_progress_passes_through_to_borg(): config = {} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) @@ -369,7 +368,9 @@ def test_check_archives_with_progress_passes_through_to_borg(): def test_check_archives_with_repair_passes_through_to_borg(): config = {} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) @@ -402,12 +403,15 @@ def test_check_archives_with_repair_passes_through_to_borg(): def test_check_archives_with_max_duration_flag_passes_through_to_borg(): config = {} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) flexmock(module).should_receive('execute_command').with_args( ('borg', 'check', '--max-duration', '33', 'repo'), + output_file=None, extra_environment=None, working_directory=None, borg_local_path='borg', @@ -432,37 +436,17 @@ def test_check_archives_with_max_duration_flag_passes_through_to_borg(): ) -def test_check_archives_with_max_duration_flag_and_archives_check_errors(): - config = {} - flexmock(module).should_receive('execute_command').never() - - with pytest.raises(ValueError): - module.check_archives( - repository_path='repo', - config=config, - local_borg_version='1.2.3', - check_arguments=flexmock( - progress=None, - repair=None, - only_checks=None, - force=None, - match_archives=None, - max_duration=33, - ), - global_arguments=flexmock(log_json=False), - checks={'repository', 'archives'}, - archive_filter_flags=(), - ) - - def test_check_archives_with_max_duration_option_passes_through_to_borg(): config = {'checks': [{'name': 'repository', 'max_duration': 33}]} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) flexmock(module).should_receive('execute_command').with_args( ('borg', 'check', '--max-duration', '33', 'repo'), + output_file=None, extra_environment=None, working_directory=None, borg_local_path='borg', @@ -487,37 +471,145 @@ def test_check_archives_with_max_duration_option_passes_through_to_borg(): ) -def test_check_archives_with_max_duration_option_and_archives_check_errors(): - config = {'checks': [{'name': 'repository', 'max_duration': 33}]} - flexmock(module).should_receive('execute_command').never() +def test_check_archives_with_max_duration_option_and_archives_check_runs_repository_check_separately(): + config = {'checks': [{'name': 'repository', 'max_duration': 33}, {'name': 'archives'}]} + flexmock(module).should_receive('make_check_name_flags').with_args({'archives'}, ()).and_return( + ('--archives-only',) + ) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(('--repository-only',)) + flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) + insert_execute_command_mock(('borg', 'check', '--archives-only', 'repo')) + insert_execute_command_mock( + ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo') + ) - with pytest.raises(ValueError): - module.check_archives( - repository_path='repo', - config=config, - local_borg_version='1.2.3', - check_arguments=flexmock( - progress=None, - repair=None, - only_checks=None, - force=None, - match_archives=None, - max_duration=None, - ), - global_arguments=flexmock(log_json=False), - checks={'repository', 'archives'}, - archive_filter_flags=(), - ) + module.check_archives( + repository_path='repo', + config=config, + local_borg_version='1.2.3', + check_arguments=flexmock( + progress=None, + repair=None, + only_checks=None, + force=None, + match_archives=None, + max_duration=None, + ), + global_arguments=flexmock(log_json=False), + checks={'repository', 'archives'}, + archive_filter_flags=(), + ) + + +def test_check_archives_with_max_duration_flag_and_archives_check_runs_repository_check_separately(): + config = {'checks': [{'name': 'repository'}, {'name': 'archives'}]} + flexmock(module).should_receive('make_check_name_flags').with_args({'archives'}, ()).and_return( + ('--archives-only',) + ) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(('--repository-only',)) + flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) + insert_execute_command_mock(('borg', 'check', '--archives-only', 'repo')) + insert_execute_command_mock( + ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo') + ) + + module.check_archives( + repository_path='repo', + config=config, + local_borg_version='1.2.3', + check_arguments=flexmock( + progress=None, + repair=None, + only_checks=None, + force=None, + match_archives=None, + max_duration=33, + ), + global_arguments=flexmock(log_json=False), + checks={'repository', 'archives'}, + archive_filter_flags=(), + ) + + +def test_check_archives_with_max_duration_option_and_data_check_runs_repository_check_separately(): + config = {'checks': [{'name': 'repository', 'max_duration': 33}, {'name': 'data'}]} + flexmock(module).should_receive('make_check_name_flags').with_args( + {'data', 'archives'}, () + ).and_return(('--archives-only', '--verify-data')) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(('--repository-only',)) + flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) + insert_execute_command_mock(('borg', 'check', '--archives-only', '--verify-data', 'repo')) + insert_execute_command_mock( + ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo') + ) + + module.check_archives( + repository_path='repo', + config=config, + local_borg_version='1.2.3', + check_arguments=flexmock( + progress=None, + repair=None, + only_checks=None, + force=None, + match_archives=None, + max_duration=None, + ), + global_arguments=flexmock(log_json=False), + checks={'repository', 'data'}, + archive_filter_flags=(), + ) + + +def test_check_archives_with_max_duration_flag_and_data_check_runs_repository_check_separately(): + config = {'checks': [{'name': 'repository'}, {'name': 'data'}]} + flexmock(module).should_receive('make_check_name_flags').with_args( + {'data', 'archives'}, () + ).and_return(('--archives-only', '--verify-data')) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(('--repository-only',)) + flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) + insert_execute_command_mock(('borg', 'check', '--archives-only', '--verify-data', 'repo')) + insert_execute_command_mock( + ('borg', 'check', '--max-duration', '33', '--repository-only', 'repo') + ) + + module.check_archives( + repository_path='repo', + config=config, + local_borg_version='1.2.3', + check_arguments=flexmock( + progress=None, + repair=None, + only_checks=None, + force=None, + match_archives=None, + max_duration=33, + ), + global_arguments=flexmock(log_json=False), + checks={'repository', 'data'}, + archive_filter_flags=(), + ) def test_check_archives_with_max_duration_flag_overrides_max_duration_option(): config = {'checks': [{'name': 'repository', 'max_duration': 33}]} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) flexmock(module).should_receive('execute_command').with_args( ('borg', 'check', '--max-duration', '44', 'repo'), + output_file=None, extra_environment=None, working_directory=None, borg_local_path='borg', @@ -575,9 +667,37 @@ def test_check_archives_calls_borg_with_parameters(checks): ) +def test_check_archives_with_data_check_implies_archives_check_calls_borg_with_parameters(): + config = {} + flexmock(module).should_receive('make_check_name_flags').with_args( + {'data', 'archives'}, () + ).and_return(()) + flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) + insert_execute_command_mock(('borg', 'check', 'repo')) + + module.check_archives( + repository_path='repo', + config=config, + local_borg_version='1.2.3', + check_arguments=flexmock( + progress=None, + repair=None, + only_checks=None, + force=None, + match_archives=None, + max_duration=None, + ), + global_arguments=flexmock(log_json=False), + checks={'data'}, + archive_filter_flags=(), + ) + + def test_check_archives_with_log_info_passes_through_to_borg(): config = {} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) insert_logging_mock(logging.INFO) insert_execute_command_mock(('borg', 'check', '--info', 'repo')) @@ -602,7 +722,9 @@ def test_check_archives_with_log_info_passes_through_to_borg(): def test_check_archives_with_log_debug_passes_through_to_borg(): config = {} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) insert_logging_mock(logging.DEBUG) insert_execute_command_mock(('borg', 'check', '--debug', '--show-rc', 'repo')) @@ -806,7 +928,9 @@ def test_check_archives_with_retention_prefix(): def test_check_archives_with_extra_borg_options_passes_through_to_borg(): config = {'extra_borg_options': {'check': '--extra --options'}} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) insert_execute_command_mock(('borg', 'check', '--extra', '--options', 'repo')) @@ -829,15 +953,16 @@ def test_check_archives_with_extra_borg_options_passes_through_to_borg(): def test_check_archives_with_match_archives_passes_through_to_borg(): - config = {} - flexmock(module).should_receive('make_check_name_flags').and_return( - ('--match-archives', 'foo-*') - ) + config = {'checks': [{'name': 'archives'}]} + flexmock(module).should_receive('make_check_name_flags').with_args( + {'archives'}, object + ).and_return(('--match-archives', 'foo-*')) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None) flexmock(module).should_receive('execute_command').with_args( ('borg', 'check', '--match-archives', 'foo-*', 'repo'), + output_file=None, extra_environment=None, working_directory=None, borg_local_path='borg', @@ -864,7 +989,9 @@ def test_check_archives_with_match_archives_passes_through_to_borg(): def test_check_archives_calls_borg_with_working_directory(): config = {'working_directory': '/working/dir'} - flexmock(module).should_receive('make_check_name_flags').and_return(()) + flexmock(module).should_receive('make_check_name_flags').with_args( + {'repository'}, () + ).and_return(()) flexmock(module.flags).should_receive('make_repository_flags').and_return(('repo',)) flexmock(module.environment).should_receive('make_environment') flexmock(module.borgmatic.config.paths).should_receive('get_working_directory').and_return(None)