From 9ba78fa33bdefb44b4aa01977796413b3aed1c41 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Fri, 21 Feb 2025 17:59:45 -0800 Subject: [PATCH] Don't try to unmount empty directories (#1001). --- borgmatic/hooks/data_source/lvm.py | 7 +- borgmatic/hooks/data_source/zfs.py | 7 +- tests/unit/hooks/data_source/test_lvm.py | 87 ++++++++++++++++++++++-- tests/unit/hooks/data_source/test_zfs.py | 41 +++++++++++ 4 files changed, 134 insertions(+), 8 deletions(-) diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py index 186234f3..11cd1c09 100644 --- a/borgmatic/hooks/data_source/lvm.py +++ b/borgmatic/hooks/data_source/lvm.py @@ -376,7 +376,10 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, d snapshot_mount_path = os.path.join( snapshots_directory, logical_volume.mount_point.lstrip(os.path.sep) ) - if not os.path.isdir(snapshot_mount_path): + + # If the snapshot mount path is empty, this is probably just a "shadow" of a nested + # logical volume and therefore there's nothing to unmount. + if not os.path.isdir(snapshot_mount_path) or not os.listdir(snapshot_mount_path): continue # This might fail if the directory is already mounted, but we swallow errors here since @@ -401,7 +404,7 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, d return except subprocess.CalledProcessError as error: logger.debug(error) - return + continue if not dry_run: shutil.rmtree(snapshots_directory) diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py index 66cc4dce..a7e9f01d 100644 --- a/borgmatic/hooks/data_source/zfs.py +++ b/borgmatic/hooks/data_source/zfs.py @@ -381,7 +381,10 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, d # child datasets before the shorter mount point paths of parent datasets. for mount_point in reversed(dataset_mount_points): snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep)) - if not os.path.isdir(snapshot_mount_path): + + # If the snapshot mount path is empty, 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 not os.listdir(snapshot_mount_path): continue # This might fail if the path is already mounted, but we swallow errors here since we'll @@ -408,7 +411,7 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, d continue if not dry_run: - shutil.rmtree(snapshots_directory, ignore_errors=True) + shutil.rmtree(snapshots_directory) # Destroy snapshots. full_snapshot_names = get_all_snapshots(zfs_command) diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py index 9e172de2..34cf5250 100644 --- a/tests/unit/hooks/data_source/test_lvm.py +++ b/tests/unit/hooks/data_source/test_lvm.py @@ -824,6 +824,7 @@ def test_remove_data_source_dumps_unmounts_and_remove_snapshots(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -985,6 +986,72 @@ def test_remove_data_source_dumps_with_missing_snapshot_mount_path_skips_unmount flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2' ).and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) + flexmock(module.shutil).should_receive('rmtree') + flexmock(module).should_receive('unmount_snapshot').with_args( + 'umount', + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume1', + ).never() + flexmock(module).should_receive('unmount_snapshot').with_args( + 'umount', + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2', + ).once() + flexmock(module).should_receive('get_snapshots').and_return( + ( + module.Snapshot('lvolume1_borgmatic-1234', '/dev/lvolume1'), + module.Snapshot('lvolume2_borgmatic-1234', '/dev/lvolume2'), + ), + ) + flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume1').once() + flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume2').once() + + module.remove_data_source_dumps( + hook_config=config['lvm'], + config=config, + borgmatic_runtime_directory='/run/borgmatic', + dry_run=False, + ) + + +def test_remove_data_source_dumps_with_empty_snapshot_mount_path_skips_unmount(): + config = {'lvm': {}} + flexmock(module).should_receive('get_logical_volumes').and_return( + ( + module.Logical_volume( + name='lvolume1', + device_path='/dev/lvolume1', + mount_point='/mnt/lvolume1', + contained_patterns=(Pattern('/mnt/lvolume1/subdir'),), + ), + module.Logical_volume( + name='lvolume2', + device_path='/dev/lvolume2', + mount_point='/mnt/lvolume2', + contained_patterns=(Pattern('/mnt/lvolume2'),), + ), + ) + ) + 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.os.path).should_receive('isdir').with_args( + '/run/borgmatic/lvm_snapshots/b33f' + ).and_return(True) + flexmock(module.os.path).should_receive('isdir').with_args( + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume1' + ).and_return(True) + flexmock(module.os).should_receive('listdir').with_args( + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume1' + ).and_return([]) + flexmock(module.os.path).should_receive('isdir').with_args( + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2' + ).and_return(True) + flexmock(module.os).should_receive('listdir').with_args( + '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2' + ).and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1044,6 +1111,7 @@ def test_remove_data_source_dumps_with_successful_mount_point_removal_skips_unmo flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2' ).and_return(True).and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1095,6 +1163,7 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1115,7 +1184,7 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command(): ) -def test_remove_data_source_dumps_bails_for_umount_command_error(): +def test_remove_data_source_dumps_swallows_umount_command_error(): config = {'lvm': {}} flexmock(module).should_receive('get_logical_volumes').and_return( ( @@ -1140,6 +1209,7 @@ def test_remove_data_source_dumps_bails_for_umount_command_error(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1148,9 +1218,15 @@ def test_remove_data_source_dumps_bails_for_umount_command_error(): flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', '/run/borgmatic/lvm_snapshots/b33f/mnt/lvolume2', - ).never() - flexmock(module).should_receive('get_snapshots').never() - flexmock(module).should_receive('remove_snapshot').never() + ).once() + flexmock(module).should_receive('get_snapshots').and_return( + ( + module.Snapshot('lvolume1_borgmatic-1234', '/dev/lvolume1'), + module.Snapshot('lvolume2_borgmatic-1234', '/dev/lvolume2'), + ), + ) + flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume1').once() + flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume2').once() module.remove_data_source_dumps( hook_config=config['lvm'], @@ -1185,6 +1261,7 @@ def test_remove_data_source_dumps_bails_for_missing_lvs_command(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1230,6 +1307,7 @@ def test_remove_data_source_dumps_bails_for_lvs_command_error(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', @@ -1275,6 +1353,7 @@ def test_remove_data_source_with_dry_run_skips_snapshot_unmount_and_delete(): ).and_return('/run/borgmatic') flexmock(module.glob).should_receive('glob').replace_with(lambda path: [path]) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree').never() flexmock(module).should_receive('unmount_snapshot').never() flexmock(module).should_receive('get_snapshots').and_return( diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py index 5900e037..fbe18fa9 100644 --- a/tests/unit/hooks/data_source/test_zfs.py +++ b/tests/unit/hooks/data_source/test_zfs.py @@ -477,6 +477,7 @@ def test_remove_data_source_dumps_unmounts_and_destroys_snapshots(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( 'umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' @@ -505,6 +506,7 @@ def test_remove_data_source_dumps_use_custom_commands(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( '/usr/local/bin/umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' @@ -580,6 +582,7 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( '/usr/local/bin/umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' @@ -605,6 +608,7 @@ def test_remove_data_source_dumps_swallows_umount_command_error(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').with_args( '/usr/local/bin/umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' @@ -665,6 +669,41 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_no flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' ).and_return(False) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) + flexmock(module.shutil).should_receive('rmtree') + flexmock(module).should_receive('unmount_snapshot').never() + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid') + ) + flexmock(module).should_receive('destroy_snapshot').with_args( + 'zfs', 'dataset@borgmatic-1234' + ).once() + + module.remove_data_source_dumps( + hook_config={}, + config={'source_directories': '/mnt/dataset', 'zfs': {}}, + borgmatic_runtime_directory='/run/borgmatic', + dry_run=False, + ) + + +def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_empty(): + flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + 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.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' + ).and_return(True) + flexmock(module.os).should_receive('listdir').with_args( + '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' + ).and_return([]) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').never() flexmock(module).should_receive('get_all_snapshots').and_return( @@ -696,6 +735,7 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_after_rmtre flexmock(module.os.path).should_receive('isdir').with_args( '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' ).and_return(True).and_return(False) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree') flexmock(module).should_receive('unmount_snapshot').never() flexmock(module).should_receive('get_all_snapshots').and_return( @@ -722,6 +762,7 @@ def test_remove_data_source_dumps_with_dry_run_skips_unmount_and_destroy(): lambda path: [path.replace('*', 'b33f')] ) flexmock(module.os.path).should_receive('isdir').and_return(True) + flexmock(module.os).should_receive('listdir').and_return(['file.txt']) flexmock(module.shutil).should_receive('rmtree').never() flexmock(module).should_receive('unmount_snapshot').never() flexmock(module).should_receive('get_all_snapshots').and_return(