Simplify logic around checking for read-only Btrfs subvolumes (#1105).

borgmatic/hooks/data_source/btrfs.py

@@ -50,46 +50,6 @@ def path_is_a_subvolume(btrfs_command, path):
     return True
 
 
-def get_containing_subvolume_path(btrfs_command, path):
-    '''
-    Given a btrfs command and a path, return the subvolume path that contains the given path (or is
-    the same as the path).
-    '''
-    # Probe the given pattern's path and all of its parents, grandparents, etc. to try to find a
-    # Btrfs subvolume.
-    for candidate_path in (
-        path,
-        *tuple(str(ancestor) for ancestor in pathlib.PurePath(path).parents),
-    ):
-        if path_is_a_subvolume(btrfs_command, candidate_path):
-            logger.debug(f'Path {candidate_path} is a Btrfs subvolume')
-            return candidate_path
-
-    return None
-
-
-def get_all_subvolume_paths(btrfs_command, patterns):
-    '''
-    Given a btrfs command and a sequence of patterns, get the sorted paths for all Btrfs subvolumes
-    containing those patterns.
-    '''
-    return tuple(
-        sorted(
-            {
-                subvolume_path
-                for pattern in patterns
-                if pattern.type == borgmatic.borg.pattern.Pattern_type.ROOT
-                if pattern.source == borgmatic.borg.pattern.Pattern_source.CONFIG
-                for subvolume_path in (get_containing_subvolume_path(btrfs_command, pattern.path),)
-                if subvolume_path
-            }
-        ),
-    )
-
-
-Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_patterns'), defaults=((),))
-
-
 def get_subvolume_property(btrfs_command, subvolume_path, property_name):
     '''
     Given a btrfs command, a subvolume path, and a property name to lookup, return the value of the
@@ -121,26 +81,61 @@ def get_subvolume_property(btrfs_command, subvolume_path, property_name):
     }.get(value, value)
 
 
-def omit_read_only_subvolume_paths(btrfs_command, subvolume_paths):
+def get_containing_subvolume_path(btrfs_command, path):
     '''
-    Given a Btrfs command to run and a sequence of Btrfs subvolume paths, filter them down to just
-    those that are read-write. The idea is that Btrfs can't actually snapshot a read-only subvolume,
-    so we should just ignore them.
+    Given a btrfs command and a path, return the subvolume path that contains the given path (or is
+    the same as the path).
+
+    If there is no such subvolume path or the containing subvolume is read-only, return None.
     '''
-    retained_subvolume_paths = []
+    # Probe the given pattern's path and all of its parents, grandparents, etc. to try to find a
+    # Btrfs subvolume.
+    for candidate_path in (
+        path,
+        *tuple(str(ancestor) for ancestor in pathlib.PurePath(path).parents),
+    ):
+        if not path_is_a_subvolume(btrfs_command, candidate_path):
+            continue
 
-    for subvolume_path in subvolume_paths:
         try:
-            if get_subvolume_property(btrfs_command, subvolume_path, 'ro'):
-                logger.debug(f'Ignoring Btrfs subvolume {subvolume_path} because it is read-only')
-            else:
-                retained_subvolume_paths.append(subvolume_path)
-        except subprocess.CalledProcessError as error:  # noqa: PERF203
+            if get_subvolume_property(btrfs_command, candidate_path, 'ro'):
+                logger.debug(f'Ignoring Btrfs subvolume {candidate_path} because it is read-only')
+
+                return None
+
+            logger.debug(f'Path {candidate_path} is a Btrfs subvolume')
+
+            return candidate_path
+        except subprocess.CalledProcessError as error:
             logger.debug(
-                f'Error determining read-only status of Btrfs subvolume {subvolume_path}: {error}',
+                f'Error determining read-only status of Btrfs subvolume {candidate_path}: {error}',
             )
 
-    return tuple(retained_subvolume_paths)
+            return None
+
+    return None
+
+
+def get_all_subvolume_paths(btrfs_command, patterns):
+    '''
+    Given a btrfs command and a sequence of patterns, get the sorted paths for all Btrfs subvolumes
+    containing those patterns.
+    '''
+    return tuple(
+        sorted(
+            {
+                subvolume_path
+                for pattern in patterns
+                if pattern.type == borgmatic.borg.pattern.Pattern_type.ROOT
+                if pattern.source == borgmatic.borg.pattern.Pattern_source.CONFIG
+                for subvolume_path in (get_containing_subvolume_path(btrfs_command, pattern.path),)
+                if subvolume_path
+            }
+        ),
+    )
+
+
+Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_patterns'), defaults=((),))
 
 
 def get_subvolumes(btrfs_command, patterns):
@@ -161,12 +156,7 @@ def get_subvolumes(btrfs_command, patterns):
     # backup. Sort the subvolumes from longest to shortest mount points, so longer subvolumes get
     # a whack at the candidate pattern piñata before their parents do. (Patterns are consumed during
     # this process, so no two subvolumes end up with the same contained patterns.)
-    for subvolume_path in reversed(
-        omit_read_only_subvolume_paths(
-            btrfs_command,
-            get_all_subvolume_paths(btrfs_command, patterns),
-        ),
-    ):
+    for subvolume_path in reversed(get_all_subvolume_paths(btrfs_command, patterns)):
         subvolumes.extend(
             Subvolume(subvolume_path, contained_patterns)
             for contained_patterns in (

docs/how-to/snapshot-your-filesystems.md

@@ -162,12 +162,11 @@ btrfs:
 ```
 
 No other options are necessary to enable Btrfs support, but if desired you can
-override some of the commands used by the Btrfs hook. For instance:
+override the `btrfs` command used by the Btrfs hook. For instance:
 
 ```yaml
 btrfs:
     btrfs_command: /usr/local/bin/btrfs
-    findmnt_command: /usr/local/bin/findmnt
 ```
 
 If you're using systemd to run borgmatic, you may need to modify the [sample systemd service
@@ -183,8 +182,8 @@ feedback](https://torsion.org/borgmatic/#issues) you have on this feature.
 #### Subvolume discovery
 
 For any read-write subvolume you'd like backed up, add its subvolume path to
-borgmatic's `source_directories` option. Btrfs does not support snapshotting
-read-only subvolumes.
+borgmatic's `source_directories` option. borgmatic does not currently support
+snapshotting read-only subvolumes.
 
 <span class="minilink minilink-addedin">New in version 2.0.7</span> The path can
 be either the path of the subvolume itself or the mount point where the

tests/unit/hooks/data_source/test_btrfs.py

@@ -37,6 +37,39 @@ def test_path_is_a_subvolume_caches_result_after_first_call():
     assert module.path_is_a_subvolume('btrfs', '/mnt0') is True
 
 
+def test_get_subvolume_property_with_invalid_btrfs_output_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output',
+    ).and_return('invalid')
+
+    with pytest.raises(ValueError):
+        module.get_subvolume_property('btrfs', '/foo', 'ro')
+
+
+def test_get_subvolume_property_with_true_output_returns_true_bool():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output',
+    ).and_return('ro=true')
+
+    assert module.get_subvolume_property('btrfs', '/foo', 'ro') is True
+
+
+def test_get_subvolume_property_with_false_output_returns_false_bool():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output',
+    ).and_return('ro=false')
+
+    assert module.get_subvolume_property('btrfs', '/foo', 'ro') is False
+
+
+def test_get_subvolume_property_passes_through_general_value():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output',
+    ).and_return('thing=value')
+
+    assert module.get_subvolume_property('btrfs', '/foo', 'thing') == 'value'
+
+
 def test_get_containing_subvolume_path_with_subvolume_self_returns_it():
     flexmock(module).should_receive('path_is_a_subvolume').with_args(
         'btrfs', '/foo/bar/baz'
@@ -44,6 +77,7 @@ def test_get_containing_subvolume_path_with_subvolume_self_returns_it():
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/foo/bar').never()
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/foo').never()
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/').never()
+    flexmock(module).should_receive('get_subvolume_property').and_return(False)
 
     assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') == '/foo/bar/baz'
 
@@ -57,6 +91,7 @@ def test_get_containing_subvolume_path_with_subvolume_parent_returns_it():
     ).and_return(True)
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/foo').never()
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/').never()
+    flexmock(module).should_receive('get_subvolume_property').and_return(False)
 
     assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') == '/foo/bar'
 
@@ -72,6 +107,7 @@ def test_get_containing_subvolume_path_with_subvolume_grandparent_returns_it():
         True
     )
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/').never()
+    flexmock(module).should_receive('get_subvolume_property').and_return(False)
 
     assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') == '/foo'
 
@@ -87,6 +123,27 @@ def test_get_containing_subvolume_path_without_subvolume_ancestor_returns_none()
         False
     )
     flexmock(module).should_receive('path_is_a_subvolume').with_args('btrfs', '/').and_return(False)
+    flexmock(module).should_receive('get_subvolume_property').and_return(False)
+
+    assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') is None
+
+
+def test_get_containing_subvolume_path_with_read_only_subvolume_returns_none():
+    flexmock(module).should_receive('path_is_a_subvolume').with_args(
+        'btrfs', '/foo/bar/baz'
+    ).and_return(True)
+    flexmock(module).should_receive('get_subvolume_property').and_return(True)
+
+    assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') is None
+
+
+def test_get_containing_subvolume_path_with_read_only_error_returns_none():
+    flexmock(module).should_receive('path_is_a_subvolume').with_args(
+        'btrfs', '/foo/bar/baz'
+    ).and_return(True)
+    flexmock(module).should_receive('get_subvolume_property').and_raise(
+        module.subprocess.CalledProcessError(1, 'wtf')
+    )
 
     assert module.get_containing_subvolume_path('btrfs', '/foo/bar/baz') is None
 
@@ -182,85 +239,8 @@ def test_get_all_subvolume_paths_sorts_subvolume_paths():
     ) == ('/bar', '/baz', '/foo')
 
 
-def test_get_subvolume_property_with_invalid_btrfs_output_errors():
-    flexmock(module.borgmatic.execute).should_receive(
-        'execute_command_and_capture_output',
-    ).and_return('invalid')
-
-    with pytest.raises(ValueError):
-        module.get_subvolume_property('btrfs', '/foo', 'ro')
-
-
-def test_get_subvolume_property_with_true_output_returns_true_bool():
-    flexmock(module.borgmatic.execute).should_receive(
-        'execute_command_and_capture_output',
-    ).and_return('ro=true')
-
-    assert module.get_subvolume_property('btrfs', '/foo', 'ro') is True
-
-
-def test_get_subvolume_property_with_false_output_returns_false_bool():
-    flexmock(module.borgmatic.execute).should_receive(
-        'execute_command_and_capture_output',
-    ).and_return('ro=false')
-
-    assert module.get_subvolume_property('btrfs', '/foo', 'ro') is False
-
-
-def test_get_subvolume_property_passes_through_general_value():
-    flexmock(module.borgmatic.execute).should_receive(
-        'execute_command_and_capture_output',
-    ).and_return('thing=value')
-
-    assert module.get_subvolume_property('btrfs', '/foo', 'thing') == 'value'
-
-
-def test_omit_read_only_subvolume_paths_filters_out_read_only_subvolumes():
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/foo',
-        'ro',
-    ).and_return(False)
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/bar',
-        'ro',
-    ).and_return(True)
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/baz',
-        'ro',
-    ).and_return(False)
-
-    assert module.omit_read_only_subvolume_paths('btrfs', ('/foo', '/bar', '/baz')) == (
-        '/foo',
-        '/baz',
-    )
-
-
-def test_omit_read_only_subvolume_paths_filters_out_erroring_subvolumes():
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/foo',
-        'ro',
-    ).and_raise(module.subprocess.CalledProcessError(1, 'btrfs'))
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/bar',
-        'ro',
-    ).and_return(True)
-    flexmock(module).should_receive('get_subvolume_property').with_args(
-        'btrfs',
-        '/baz',
-        'ro',
-    ).and_return(False)
-
-    assert module.omit_read_only_subvolume_paths('btrfs', ('/foo', '/bar', '/baz')) == ('/baz',)
-
-
 def test_get_subvolumes_collects_subvolumes_matching_patterns():
     flexmock(module).should_receive('get_all_subvolume_paths').and_return(('/mnt1', '/mnt2'))
-    flexmock(module).should_receive('omit_read_only_subvolume_paths').and_return(('/mnt1', '/mnt2'))
 
     contained_pattern = Pattern(
         '/mnt1',
@@ -285,7 +265,6 @@ def test_get_subvolumes_collects_subvolumes_matching_patterns():
 
 def test_get_subvolumes_skips_non_root_patterns():
     flexmock(module).should_receive('get_all_subvolume_paths').and_return(('/mnt1', '/mnt2'))
-    flexmock(module).should_receive('omit_read_only_subvolume_paths').and_return(('/mnt1', '/mnt2'))
 
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_patterns',
@@ -316,7 +295,6 @@ def test_get_subvolumes_skips_non_root_patterns():
 
 def test_get_subvolumes_skips_non_config_patterns():
     flexmock(module).should_receive('get_all_subvolume_paths').and_return(('/mnt1', '/mnt2'))
-    flexmock(module).should_receive('omit_read_only_subvolume_paths').and_return(('/mnt1', '/mnt2'))
 
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_patterns',