From 5dca281439da5cd68f89df721d292c2bf1d851f1 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Tue, 30 Sep 2025 12:19:57 -0700 Subject: [PATCH] Simplify logic around checking for read-only Btrfs subvolumes (#1105). --- borgmatic/hooks/data_source/btrfs.py | 108 ++++++++-------- docs/how-to/snapshot-your-filesystems.md | 7 +- tests/unit/hooks/data_source/test_btrfs.py | 136 +++++++++------------ 3 files changed, 109 insertions(+), 142 deletions(-) diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py index 7157b287..eb7afd23 100644 --- a/borgmatic/hooks/data_source/btrfs.py +++ b/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 ( diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md index 334c4a0f..a2fa8297 100644 --- a/docs/how-to/snapshot-your-filesystems.md +++ b/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. New in version 2.0.7 The path can be either the path of the subvolume itself or the mount point where the diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py index d58d1335..e9e96607 100644 --- a/tests/unit/hooks/data_source/test_btrfs.py +++ b/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',