Don't try to unmount empty directories (#1001).

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)

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)

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(

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(