Fix to snapshot both parent and child volumes/filesystems instead of just the parent (#1156).

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.
 

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]

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,
         ),
     )

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.
     '''

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({})