diff --git a/NEWS b/NEWS index 50887a40..47093737 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,9 @@ * #1151: Fix snapshotting in the ZFS, Btrfs, and LVM hooks to play nicely with the Borg 1.4+ "slashdot" hack within source directory paths. * #1152: Fix a regression in the Loki monitoring hook in which log messages weren't sending. + * #1156: Fix snapshotting in the ZFS, Btrfs, and LVM hooks to snapshot both parent and child + volumes/filesystems instead of just the parent. As part of this fix, borgmatic no longer + deduplicates patterns except for those containing the borgmatic runtime directory. * Fix a traceback (TypeError) regression in the "spot" check when the "local_path" option isn't set. diff --git a/borgmatic/actions/create.py b/borgmatic/actions/create.py index 6f3ce56c..dc31c606 100644 --- a/borgmatic/actions/create.py +++ b/borgmatic/actions/create.py @@ -54,6 +54,7 @@ def run_create( pattern.collect_patterns(config), config, working_directory, + borgmatic_runtime_directory, ) borgmatic.hooks.dispatch.call_hooks_even_if_unconfigured( 'remove_data_source_dumps', @@ -80,6 +81,7 @@ def run_create( patterns, config, working_directory, + borgmatic_runtime_directory, skip_expand_paths=config_paths, ) stream_processes = [process for processes in active_dumps.values() for process in processes] diff --git a/borgmatic/actions/pattern.py b/borgmatic/actions/pattern.py index a8a704e0..110b6ad5 100644 --- a/borgmatic/actions/pattern.py +++ b/borgmatic/actions/pattern.py @@ -227,23 +227,37 @@ def device_map_patterns(patterns, working_directory=None): ) -def deduplicate_patterns(patterns, config): +def deduplicate_runtime_directory_patterns(patterns, config, borgmatic_runtime_directory=None): ''' - Given a sequence of borgmatic.borg.pattern.Pattern instances and a configuration dict, return - them with all duplicate root child patterns removed. For instance, if two root patterns are - given with paths "/foo" and "/foo/bar", return just the one with "/foo". Non-root patterns are - passed through without modification. + Given a sequence of borgmatic.borg.pattern.Pattern instances, the borgmatic runtime directory, + and a configuration dict, return them without any duplicate root child patterns that contain the + runtime directory. For instance, if two root patterns are given with paths "/foo" and + "/foo/bar", and the runtime directory is "/foo/bar", return just the "/foo" pattern. Non-root + patterns and patterns not containing the runtime directory are passed through without + modification. - The one exception to deduplication is if two paths are on different filesystems (devices) and + One exception to deduplication is if two paths are on different filesystems (devices) and "one_file_system" is True in the given configuration. In that case, the paths won't get deduplicated, because Borg won't cross filesystem boundaries when "one_file_system" is True. - The idea is that if Borg is given a root parent pattern, then it doesn't also need to be given - child patterns, because it will naturally spider the contents of the parent pattern's path. And - there are cases where Borg coming across the same file twice will result in duplicate reads and - even hangs, e.g. when a database hook is using a named pipe for streaming database dumps to - Borg. + The idea is that if Borg is given a root parent pattern containing the borgmatic runtime + directory, then Borg doesn't also need to be given child patterns, because it will naturally + spider the contents of the parent pattern's path. Additionally, there are cases where Borg + coming across the same file twice will result in duplicate reads and even hangs, e.g. when a + database hook in the borgmatic runtime directory is using a named pipe for streaming database + dumps to Borg. + + This deduplication is limited to the borgmatic runtime directory (where borgmatic's named pipes + exist), because there are other legitimate use cases for parent and child patterns to both exist + in patterns. For instance, with some snapshotted filesystems, snapshots don't traverse from a + parent filesystem to a child and therefore both need to remain in patterns. + + And for the case of named pipes outside of the borgmatic runtime directory, there is code + elsewhere (in the "create" action) that auto-excludes special files to prevent Borg hangs. ''' + if borgmatic_runtime_directory is None: + return patterns + deduplicated = {} # Use just the keys as an ordered set. for pattern in patterns: @@ -262,6 +276,8 @@ def deduplicate_patterns(patterns, config): if any( pathlib.PurePath(other_pattern.path) == parent + and pathlib.PurePosixPath(other_pattern.path) + in pathlib.PurePath(borgmatic_runtime_directory).parents and pattern.device is not None and ( other_pattern.device == pattern.device @@ -276,16 +292,22 @@ def deduplicate_patterns(patterns, config): return tuple(deduplicated.keys()) -def process_patterns(patterns, config, working_directory, skip_expand_paths=None): +def process_patterns( + patterns, config, working_directory, borgmatic_runtime_directory=None, skip_expand_paths=None +): ''' - Given a sequence of Borg patterns, a configuration dict, a configured working directory, and a - sequence of paths to skip path expansion for, expand and deduplicate any "root" patterns, - returning the resulting root and non-root patterns as a list. + Given a sequence of Borg patterns, a configuration dict, a configured working directory, the + borgmatic runtime directory, and a sequence of paths to skip path expansion for, expand and + deduplicate any "root" patterns, returning the resulting root and non-root patterns as a list. + + If the borgmatic runtime directory is None, then don't deduplicate patterns. Deduplication is + really only necessary for the "create" action when the runtime directory might contain named + pipes for database dumps. ''' skip_paths = set(skip_expand_paths or ()) return list( - deduplicate_patterns( + deduplicate_runtime_directory_patterns( device_map_patterns( expand_patterns( patterns, @@ -294,5 +316,6 @@ def process_patterns(patterns, config, working_directory, skip_expand_paths=None ), ), config, + borgmatic_runtime_directory, ), ) diff --git a/borgmatic/borg/create.py b/borgmatic/borg/create.py index 2d7d58d5..82f13902 100644 --- a/borgmatic/borg/create.py +++ b/borgmatic/borg/create.py @@ -62,7 +62,7 @@ def validate_planned_backup_paths( excluded. Raise ValueError if the runtime directory has been excluded via "exclude_patterns" or similar, - because any features that rely on the runtime directory getting backed up will break. For + because any features that rely on the runtime directory getting backed up will break. For instance, without the runtime directory, Borg can't consume any database dumps and borgmatic may hang waiting for them to be consumed. ''' diff --git a/tests/unit/actions/test_pattern.py b/tests/unit/actions/test_pattern.py index 399f7728..ad4bb0b5 100644 --- a/tests/unit/actions/test_pattern.py +++ b/tests/unit/actions/test_pattern.py @@ -375,43 +375,97 @@ def test_device_map_patterns_with_existing_device_id_does_not_overwrite_it(): @pytest.mark.parametrize( - 'patterns,expected_patterns,one_file_system', + 'patterns,borgmatic_runtime_directory,expected_patterns,one_file_system', ( - ((Pattern('/', device=1), Pattern('/root', device=1)), (Pattern('/', device=1),), False), - ((Pattern('/', device=1), Pattern('/root/', device=1)), (Pattern('/', device=1),), False), ( - (Pattern('/', device=1), Pattern('/root', device=2)), + (Pattern('/', device=1), Pattern('/root', device=1)), + '/root', (Pattern('/', device=1),), False, ), - ((Pattern('/root', device=1), Pattern('/', device=1)), (Pattern('/', device=1),), False), + # No deduplication is expected when borgmatic runtime directory is None. + ( + (Pattern('/', device=1), Pattern('/root', device=1)), + None, + (Pattern('/', device=1), Pattern('/root', device=1)), + False, + ), + ( + (Pattern('/', device=1), Pattern('/root/', device=1)), + '/root', + (Pattern('/', device=1),), + False, + ), + ( + (Pattern('/', device=1), Pattern('/root', device=2)), + '/root', + (Pattern('/', device=1),), + False, + ), + ( + (Pattern('/', device=1), Pattern('/root', device=2)), + None, + (Pattern('/', device=1), Pattern('/root', device=2)), + False, + ), + ( + (Pattern('/root', device=1), Pattern('/', device=1)), + '/root', + (Pattern('/', device=1),), + False, + ), + ( + (Pattern('/root', device=1), Pattern('/', device=1)), + None, + (Pattern('/root', device=1), Pattern('/', device=1)), + False, + ), ( (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + '/root/foo', (Pattern('/root', device=1),), False, ), + ( + (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + None, + (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + False, + ), + # No deduplication is expected when the runtime directory doesn't match the patterns. + ( + (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + '/other', + (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + False, + ), ( (Pattern('/root/', device=1), Pattern('/root/foo', device=1)), + '/root/foo', (Pattern('/root/', device=1),), False, ), ( (Pattern('/root', device=1), Pattern('/root/foo/', device=1)), + '/root/foo', (Pattern('/root', device=1),), False, ), ( (Pattern('/root', device=1), Pattern('/root/foo', device=2)), + '/root/foo', (Pattern('/root', device=1),), False, ), ( (Pattern('/root/foo', device=1), Pattern('/root', device=1)), + '/root/foo', (Pattern('/root', device=1),), False, ), ( (Pattern('/root', device=None), Pattern('/root/foo', device=None)), + '/root/foo', (Pattern('/root'), Pattern('/root/foo')), False, ), @@ -421,6 +475,7 @@ def test_device_map_patterns_with_existing_device_id_does_not_overwrite_it(): Pattern('/etc', device=1), Pattern('/root/foo/bar', device=1), ), + '/root/foo/bar', (Pattern('/root', device=1), Pattern('/etc', device=1)), False, ), @@ -430,70 +485,126 @@ def test_device_map_patterns_with_existing_device_id_does_not_overwrite_it(): Pattern('/root/foo', device=1), Pattern('/root/foo/bar', device=1), ), + '/root/foo/bar', (Pattern('/root', device=1),), False, ), + ( + ( + Pattern('/root', device=1), + Pattern('/root/foo', device=1), + Pattern('/root/foo/bar', device=1), + ), + None, + ( + Pattern('/root', device=1), + Pattern('/root/foo', device=1), + Pattern('/root/foo/bar', device=1), + ), + False, + ), + ( + ( + Pattern('/root', device=1), + Pattern('/root/foo', device=1), + Pattern('/root/foo/bar', device=1), + ), + '/other', + ( + Pattern('/root', device=1), + Pattern('/root/foo', device=1), + Pattern('/root/foo/bar', device=1), + ), + False, + ), ( (Pattern('/dup', device=1), Pattern('/dup', device=1)), + '/dup', (Pattern('/dup', device=1),), False, ), ( (Pattern('/foo', device=1), Pattern('/bar', device=1)), + '/bar', (Pattern('/foo', device=1), Pattern('/bar', device=1)), False, ), ( (Pattern('/foo', device=1), Pattern('/bar', device=2)), + '/bar', (Pattern('/foo', device=1), Pattern('/bar', device=2)), False, ), - ((Pattern('/root/foo', device=1),), (Pattern('/root/foo', device=1),), False), + ((Pattern('/root/foo', device=1),), '/root/foo', (Pattern('/root/foo', device=1),), False), ( (Pattern('/', device=1), Pattern('/root', Pattern_type.INCLUDE, device=1)), + '/root', (Pattern('/', device=1), Pattern('/root', Pattern_type.INCLUDE, device=1)), False, ), ( (Pattern('/root', Pattern_type.INCLUDE, device=1), Pattern('/', device=1)), + '/root', (Pattern('/root', Pattern_type.INCLUDE, device=1), Pattern('/', device=1)), False, ), - ((Pattern('/', device=1), Pattern('/root', device=1)), (Pattern('/', device=1),), True), - ((Pattern('/', device=1), Pattern('/root/', device=1)), (Pattern('/', device=1),), True), + ( + (Pattern('/', device=1), Pattern('/root', device=1)), + '/root', + (Pattern('/', device=1),), + True, + ), + ( + (Pattern('/', device=1), Pattern('/root/', device=1)), + '/root', + (Pattern('/', device=1),), + True, + ), ( (Pattern('/', device=1), Pattern('/root', device=2)), + '/root', (Pattern('/', device=1), Pattern('/root', device=2)), True, ), - ((Pattern('/root', device=1), Pattern('/', device=1)), (Pattern('/', device=1),), True), + ( + (Pattern('/root', device=1), Pattern('/', device=1)), + '/root', + (Pattern('/', device=1),), + True, + ), ( (Pattern('/root', device=1), Pattern('/root/foo', device=1)), + '/root/foo', (Pattern('/root', device=1),), True, ), ( (Pattern('/root/', device=1), Pattern('/root/foo', device=1)), + '/root/foo', (Pattern('/root/', device=1),), True, ), ( (Pattern('/root', device=1), Pattern('/root/foo/', device=1)), + '/root/foo', (Pattern('/root', device=1),), True, ), ( (Pattern('/root', device=1), Pattern('/root/foo', device=2)), + '/root/foo', (Pattern('/root', device=1), Pattern('/root/foo', device=2)), True, ), ( (Pattern('/root/foo', device=1), Pattern('/root', device=1)), + '/root/foo', (Pattern('/root', device=1),), True, ), ( (Pattern('/root', device=None), Pattern('/root/foo', device=None)), + '/root/foo', (Pattern('/root'), Pattern('/root/foo')), True, ), @@ -503,6 +614,7 @@ def test_device_map_patterns_with_existing_device_id_does_not_overwrite_it(): Pattern('/etc', device=1), Pattern('/root/foo/bar', device=1), ), + '/root/foo/bar', (Pattern('/root', device=1), Pattern('/etc', device=1)), True, ), @@ -512,50 +624,59 @@ def test_device_map_patterns_with_existing_device_id_does_not_overwrite_it(): Pattern('/root/foo', device=1), Pattern('/root/foo/bar', device=1), ), + '/root/foo/bar', (Pattern('/root', device=1),), True, ), ( (Pattern('/dup', device=1), Pattern('/dup', device=1)), + '/dup', (Pattern('/dup', device=1),), True, ), ( (Pattern('/foo', device=1), Pattern('/bar', device=1)), + '/bar', (Pattern('/foo', device=1), Pattern('/bar', device=1)), True, ), ( (Pattern('/foo', device=1), Pattern('/bar', device=2)), + '/bar', (Pattern('/foo', device=1), Pattern('/bar', device=2)), True, ), - ((Pattern('/root/foo', device=1),), (Pattern('/root/foo', device=1),), True), + ((Pattern('/root/foo', device=1),), '/root/foo', (Pattern('/root/foo', device=1),), True), ( (Pattern('/', device=1), Pattern('/root', Pattern_type.INCLUDE, device=1)), + '/root', (Pattern('/', device=1), Pattern('/root', Pattern_type.INCLUDE, device=1)), True, ), ( (Pattern('/root', Pattern_type.INCLUDE, device=1), Pattern('/', device=1)), + '/root', (Pattern('/root', Pattern_type.INCLUDE, device=1), Pattern('/', device=1)), True, ), ), ) -def test_deduplicate_patterns_omits_child_paths_based_on_device_and_one_file_system( +def test_deduplicate_runtime_directory_patterns_omits_child_paths_based_on_device_and_one_file_system( patterns, + borgmatic_runtime_directory, expected_patterns, one_file_system, ): assert ( - module.deduplicate_patterns(patterns, {'one_file_system': one_file_system}) + module.deduplicate_runtime_directory_patterns( + patterns, {'one_file_system': one_file_system}, borgmatic_runtime_directory + ) == expected_patterns ) def test_process_patterns_includes_patterns(): - flexmock(module).should_receive('deduplicate_patterns').and_return( + flexmock(module).should_receive('deduplicate_runtime_directory_patterns').and_return( (Pattern('foo'), Pattern('bar')), ) flexmock(module).should_receive('device_map_patterns').and_return({}) @@ -574,7 +695,7 @@ def test_process_patterns_includes_patterns(): def test_process_patterns_skips_expand_for_requested_paths(): skip_paths = {flexmock()} - flexmock(module).should_receive('deduplicate_patterns').and_return( + flexmock(module).should_receive('deduplicate_runtime_directory_patterns').and_return( (Pattern('foo'), Pattern('bar')), ) flexmock(module).should_receive('device_map_patterns').and_return({})