From 4ee7f726965d1fb9ec05f43aa9e25fab0bfa1e25 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Sun, 9 Mar 2025 23:04:55 -0700 Subject: [PATCH] Fix an error in the Btrfs hook when attempting to snapshot a read-only subvolume (#1023). --- NEWS | 2 + borgmatic/hooks/data_source/btrfs.py | 47 +++++++++++++++- docs/how-to/snapshot-your-filesystems.md | 5 +- tests/unit/hooks/data_source/test_btrfs.py | 62 ++++++++++++++++++++++ 4 files changed, 113 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 87936e40..16e3db14 100644 --- a/NEWS +++ b/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. diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py index ec6d33df..f46cbc2e 100644 --- a/borgmatic/hooks/data_source/btrfs.py +++ b/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 ( diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md index bd569957..8dd8bad5 100644 --- a/docs/how-to/snapshot-your-filesystems.md +++ b/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. New in version 1.9.6 Or include the mount point as a root pattern with borgmatic's `patterns` or `patterns_from` diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py index 648902c3..2ba0ccf8 100644 --- a/tests/unit/hooks/data_source/test_btrfs.py +++ b/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'