From 501f12dce3be0a0906595a58eb28a59c69a5e8e9 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Wed, 10 Jun 2026 16:15:32 -0700 Subject: [PATCH] Fix the ZFS hook's overzealous unmounting of snapshot paths when a source dataset is at "/" (#1319). --- NEWS | 2 + borgmatic/hooks/data_source/zfs.py | 68 +++++++++++++++--------- tests/unit/hooks/data_source/test_zfs.py | 48 +++++++++++++++-- 3 files changed, 88 insertions(+), 30 deletions(-) diff --git a/NEWS b/NEWS index 440c8355..495d6086 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ 2.1.7.dev0 + * #1319: Fix the ZFS hook's overzealous unmounting of snapshot paths when a source dataset is at + "/". * Fix a bug in which the "compact" action does not pass a compact threshold of zero to Borg. 2.1.6 diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py index 4212e801..bff7c209 100644 --- a/borgmatic/hooks/data_source/zfs.py +++ b/borgmatic/hooks/data_source/zfs.py @@ -403,41 +403,57 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, p snapshot_dataset_names = { full_snapshot_name.split('@')[0] for full_snapshot_name in full_snapshot_names } + hash_to_dataset_mount_point = {} + + # Make a map from mount point hash to the corresponding (dataset name, mount point) tuple. + for dataset_name, mount_point in dataset_name_to_mount_point.items(): + mount_point_hash = hashlib.shake_256(mount_point.encode('utf-8')).hexdigest( + MOUNT_POINT_HASH_LENGTH + ) + hash_to_dataset_mount_point[mount_point_hash] = (dataset_name, mount_point) for snapshots_directory in glob.glob(snapshots_glob): if not os.path.isdir(snapshots_directory): continue - for dataset_name, mount_point in dataset_name_to_mount_point.items(): - snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep)) + # Get the dataset and mount point corresponding to the hash found in this snapshot directory + # path. If none is found, bail. + try: + (dataset_name, mount_point) = hash_to_dataset_mount_point[ + os.path.basename(snapshots_directory) + ] + except KeyError: + continue - # If this dataset name does not correspond to a known snapshot, then this is probably - # just a "shadow" of a nested dataset and therefore there's nothing to unmount. - if not os.path.isdir(snapshot_mount_path) or dataset_name not in snapshot_dataset_names: + snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep)) + + # If this dataset name doesn't correspond to a known snapshot, then this is probably + # just a "shadow" of a nested dataset and therefore there's nothing to unmount. + if not os.path.isdir(snapshot_mount_path) or dataset_name not in snapshot_dataset_names: + continue + + # This might fail if the path is already mounted, but we swallow errors here since we'll + # do another recursive delete below. The point of doing it here is that we don't want to + # try to unmount a non-mounted directory (which *will* fail), and probing for whether a + # directory is mounted is tough to do in a cross-platform way. + if not dry_run: + shutil.rmtree(snapshot_mount_path, ignore_errors=True) + + # If the delete was successful, that means there's nothing to unmount. + if not os.path.isdir(snapshot_mount_path): continue - # This might fail if the path is already mounted, but we swallow errors here since we'll - # do another recursive delete below. The point of doing it here is that we don't want to - # try to unmount a non-mounted directory (which *will* fail), and probing for whether a - # directory is mounted is tough to do in a cross-platform way. - if not dry_run: - shutil.rmtree(snapshot_mount_path, ignore_errors=True) + logger.debug(f'Unmounting ZFS snapshot at {snapshot_mount_path}{dry_run_label}') - # If the delete was successful, that means there's nothing to unmount. - if not os.path.isdir(snapshot_mount_path): - continue - - logger.debug(f'Unmounting ZFS snapshot at {snapshot_mount_path}{dry_run_label}') - - if not dry_run: - try: - unmount_snapshot(umount_command, snapshot_mount_path) - except FileNotFoundError: - logger.debug(f'Could not find "{umount_command}" command') - return - except subprocess.CalledProcessError as error: - logger.debug(error) - continue + if not dry_run: + try: + unmount_snapshot(umount_command, snapshot_mount_path) + except FileNotFoundError: + logger.debug(f'Could not find "{umount_command}" command') + return + except subprocess.CalledProcessError as error: + logger.debug(error) + continue if not dry_run: shutil.rmtree(snapshot_mount_path, ignore_errors=True) diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py index 539a0da3..0284d08a 100644 --- a/tests/unit/hooks/data_source/test_zfs.py +++ b/tests/unit/hooks/data_source/test_zfs.py @@ -528,6 +528,9 @@ def test_remove_data_source_dumps_unmounts_and_destroys_snapshots(): flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -561,6 +564,9 @@ def test_remove_data_source_dumps_use_custom_commands(): flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -644,6 +650,9 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command(): flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -675,6 +684,9 @@ def test_remove_data_source_dumps_swallows_umount_command_error(): flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -709,6 +721,9 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_directories_that_are_no flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -739,6 +754,9 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_no flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -774,21 +792,37 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_for_unknown flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') - flexmock(module.glob).should_receive('glob').replace_with( - lambda path: [path.replace('*', 'b33f')], + flexmock(module.hashlib).should_receive('shake_256').with_args(b'/mnt/dataset').and_return( + flexmock(hexdigest=lambda length: 'd34d') ) + flexmock(module.hashlib).should_receive('shake_256').with_args( + b'/mnt/dataset/shadow' + ).and_return(flexmock(hexdigest=lambda length: 'b33f')) + flexmock(module.glob).should_receive('glob').replace_with( + lambda path: [ + path.replace('*', 'd34d'), + path.replace('*', 'b33f'), + path.replace('*', 'unknown'), + ], + ) + flexmock(module.os.path).should_receive('isdir').with_args( + '/run/borgmatic/zfs_snapshots/d34d', + ).and_return(True) flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/zfs_snapshots/b33f', ).and_return(True) flexmock(module.os.path).should_receive('isdir').with_args( - '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset', + '/run/borgmatic/zfs_snapshots/d34d/mnt/dataset', ).and_return(True) flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset/shadow', ).and_return(True) + flexmock(module.os.path).should_receive('isdir').with_args( + '/run/borgmatic/zfs_snapshots/unknown', + ).and_return(True) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( - 'umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' + 'umount', '/run/borgmatic/zfs_snapshots/d34d/mnt/dataset' ).once() flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset/shadow' @@ -817,6 +851,9 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_after_rmtre flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], ) @@ -852,6 +889,9 @@ def test_remove_data_source_dumps_with_dry_run_skips_unmount_and_destroy(): flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') + flexmock(module.hashlib).should_receive('shake_256').and_return( + flexmock(hexdigest=lambda length: 'b33f') + ) flexmock(module.glob).should_receive('glob').replace_with( lambda path: [path.replace('*', 'b33f')], )