Fix an error in the Btrfs hook when attempting to snapshot a read-only subvolume (#1023).

NEWS

@@ -7,6 +7,8 @@
  * #1017: Fix a regression in which some MariaDB/MySQL passwords were not escaped correctly.
  * #1021: Fix a regression in which the "exclude_patterns" option didn't expand "~" (the user's
    home directory). This fix means that all "patterns" and "patterns_from" also now expand "~".
+ * #1023: Fix an error in the Btrfs hook when attempting to snapshot a read-only subvolume. Now,
+   read-only subvolumes are ignored since Btrfs can't actually snapshot them.
 
 1.9.13
  * #975: Add a "compression" option to the PostgreSQL database hook.

borgmatic/hooks/data_source/btrfs.py

@@ -48,6 +48,47 @@ def get_subvolume_mount_points(findmnt_command):
 Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_patterns'), defaults=((),))
 
 
+def get_subvolume_property(btrfs_command, subvolume_path, property_name):
+    output = borgmatic.execute.execute_command_and_capture_output(
+        tuple(btrfs_command.split(' '))
+        + (
+            'property',
+            'get',
+            '-t',  # Type.
+            'subvol',
+            subvolume_path,
+            property_name,
+        ),
+    )
+
+    try:
+        value = output.strip().split('=')[1]
+    except IndexError:
+        raise ValueError(f'Invalid {btrfs_command} property output')
+
+    return {
+        'true': True,
+        'false': False,
+    }.get(value, value)
+
+
+def omit_read_only_subvolume_mount_points(btrfs_command, subvolume_paths):
+    '''
+    Given a Btrfs command to run and a sequence of Btrfs subvolume mount points, 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.
+    '''
+    retained_subvolume_paths = []
+
+    for subvolume_path in subvolume_paths:
+        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)
+
+    return tuple(retained_subvolume_paths)
+
+
 def get_subvolumes(btrfs_command, findmnt_command, patterns=None):
     '''
     Given a Btrfs command to run and a sequence of configured patterns, find the intersection
@@ -67,7 +108,11 @@ def get_subvolumes(btrfs_command, findmnt_command, patterns=None):
     # backup. Sort the subvolumes from longest to shortest mount points, so longer mount points 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 mount_point in reversed(get_subvolume_mount_points(findmnt_command)):
+    for mount_point in reversed(
+        omit_read_only_subvolume_mount_points(
+            btrfs_command, get_subvolume_mount_points(findmnt_command)
+        )
+    ):
         subvolumes.extend(
             Subvolume(mount_point, contained_patterns)
             for contained_patterns in (

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

@@ -148,8 +148,9 @@ feedback](https://torsion.org/borgmatic/#issues) you have on this feature.
 
 #### Subvolume discovery
 
-For any subvolume you'd like backed up, add its path to borgmatic's
-`source_directories` option.
+For any read-write subvolume you'd like backed up, add its path to borgmatic's
+`source_directories` option. Btrfs does not support snapshotting read-only
+subvolumes.
 
 <span class="minilink minilink-addedin">New in version 1.9.6</span> Or include
 the mount point as a root pattern with borgmatic's `patterns` or `patterns_from`

tests/unit/hooks/data_source/test_btrfs.py

@@ -49,8 +49,61 @@ def test_get_subvolume_mount_points_with_findmnt_json_missing_filesystems_errors
         module.get_subvolume_mount_points('findmnt')
 
 
+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') == 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') == 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_mount_points_filters_out_read_only():
+    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_mount_points('btrfs', ('/foo', '/bar', '/baz')) == (
+        '/foo',
+        '/baz',
+    )
+
+
 def test_get_subvolumes_collects_subvolumes_matching_patterns():
     flexmock(module).should_receive('get_subvolume_mount_points').and_return(('/mnt1', '/mnt2'))
+    flexmock(module).should_receive('omit_read_only_subvolume_mount_points').and_return(
+        ('/mnt1', '/mnt2')
+    )
 
     contained_pattern = Pattern(
         '/mnt1',
@@ -76,6 +129,9 @@ def test_get_subvolumes_collects_subvolumes_matching_patterns():
 
 def test_get_subvolumes_skips_non_root_patterns():
     flexmock(module).should_receive('get_subvolume_mount_points').and_return(('/mnt1', '/mnt2'))
+    flexmock(module).should_receive('omit_read_only_subvolume_mount_points').and_return(
+        ('/mnt1', '/mnt2')
+    )
 
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_patterns'
@@ -107,6 +163,9 @@ def test_get_subvolumes_skips_non_root_patterns():
 
 def test_get_subvolumes_skips_non_config_patterns():
     flexmock(module).should_receive('get_subvolume_mount_points').and_return(('/mnt1', '/mnt2'))
+    flexmock(module).should_receive('omit_read_only_subvolume_mount_points').and_return(
+        ('/mnt1', '/mnt2')
+    )
 
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_patterns'
@@ -138,6 +197,9 @@ def test_get_subvolumes_skips_non_config_patterns():
 
 def test_get_subvolumes_without_patterns_collects_all_subvolumes():
     flexmock(module).should_receive('get_subvolume_mount_points').and_return(('/mnt1', '/mnt2'))
+    flexmock(module).should_receive('omit_read_only_subvolume_mount_points').and_return(
+        ('/mnt1', '/mnt2')
+    )
 
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_patterns'