Merge branch 'main' into unified-command-hooks.

NEWS

@@ -1,4 +1,4 @@
-1.9.14.dev0
+1.9.14
  * #409: With the PagerDuty monitoring hook, send borgmatic logs to PagerDuty so they show up in the
    incident UI. See the documentation for more information:
    https://torsion.org/borgmatic/docs/how-to/monitor-your-backups/#pagerduty-hook
@@ -10,6 +10,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

@@ -49,6 +49,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
@@ -68,7 +109,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`

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "borgmatic"
-version = "1.9.14.dev0"
+version = "1.9.14"
 authors = [
   { name="Dan Helfman", email="witten@torsion.org" },
 ]

tests/end-to-end/commands/fake_btrfs.py

@@ -24,7 +24,14 @@ def parse_arguments(*unparsed_arguments):
     delete_parser = subvolume_subparser.add_parser('delete')
     delete_parser.add_argument('snapshot_path')
 
-    return global_parser.parse_args(unparsed_arguments)
+    property_parser = action_parsers.add_parser('property')
+    property_subparser = property_parser.add_subparsers(dest='subaction')
+    get_parser = property_subparser.add_parser('get')
+    get_parser.add_argument('-t', dest='type')
+    get_parser.add_argument('subvolume_path')
+    get_parser.add_argument('property_name')
+
+    return (global_parser, global_parser.parse_args(unparsed_arguments))
 
 
 BUILTIN_SUBVOLUME_LIST_LINES = (
@@ -60,9 +67,13 @@ def print_subvolume_list(arguments, snapshot_paths):
 
 
 def main():
-    arguments = parse_arguments(*sys.argv[1:])
+    (global_parser, arguments) = parse_arguments(*sys.argv[1:])
     snapshot_paths = load_snapshots()
 
+    if not hasattr(arguments, 'subaction'):
+        global_parser.print_help()
+        sys.exit(1)
+
     if arguments.subaction == 'list':
         print_subvolume_list(arguments, snapshot_paths)
     elif arguments.subaction == 'snapshot':
@@ -84,6 +95,8 @@ def main():
             if snapshot_path.endswith('/' + arguments.snapshot_path)
         ]
         save_snapshots(snapshot_paths)
+    elif arguments.action == 'property' and arguments.subaction == 'get':
+        print(f'{arguments.property_name}=false')
 
 
 if __name__ == '__main__':

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') 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_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'