diff --git a/NEWS b/NEWS index ab421e25..7caa1500 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,7 @@ symlinks and the bootstrap data source hook is enabled. * #1294: Fix a regression in which SSH warnings from remote repositories broke the "spot" check and other actions as well. + * #1295: Fix the ZFS hook to properly unmount snapshots for empty datasets. 2.1.4 * #1266: Add a stand-alone borgmatic Linux binary to the release downloads to serve as another way diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py index f8eb840a..4212e801 100644 --- a/borgmatic/hooks/data_source/zfs.py +++ b/borgmatic/hooks/data_source/zfs.py @@ -2,6 +2,7 @@ import collections import glob import hashlib import logging +import operator import os import shutil import subprocess @@ -123,7 +124,8 @@ def get_datasets_to_backup(zfs_command, patterns): def get_all_dataset_mount_points(zfs_command): ''' - Given a ZFS command to run, return all ZFS datasets as a sequence of sorted mount points. + Given a ZFS command to run, return a dict from ZFS dataset name to mount point (reverse sorted + by mount point). ''' list_lines = borgmatic.execute.execute_command_and_capture_output( ( @@ -133,19 +135,23 @@ def get_all_dataset_mount_points(zfs_command): '-t', 'filesystem', '-o', - 'mountpoint', + 'name,mountpoint', ), close_fds=True, ) - return tuple( + return dict( sorted( - { - mount_point + ( + (dataset_name, mount_point) for line in list_lines - for mount_point in (line.rstrip(),) + for (dataset_name, mount_point) in (line.rstrip().split('\t'),) if mount_point != 'none' - }, + ), + key=operator.itemgetter(1), + # Reversing the sorted datasets ensures that we unmount the longer mount point paths of + # child datasets before the shorter mount point paths of parent datasets. + reverse=True, ), ) @@ -376,7 +382,8 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, p zfs_command = hook_config.get('zfs_command', 'zfs') try: - dataset_mount_points = get_all_dataset_mount_points(zfs_command) + dataset_name_to_mount_point = get_all_dataset_mount_points(zfs_command) + full_snapshot_names = get_all_snapshots(zfs_command) except FileNotFoundError: logger.debug(f'Could not find "{zfs_command}" command') return @@ -393,19 +400,20 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, p ) logger.debug(f'Looking for snapshots to remove in {snapshots_glob}{dry_run_label}') umount_command = hook_config.get('umount_command', 'umount') + snapshot_dataset_names = { + full_snapshot_name.split('@')[0] for full_snapshot_name in full_snapshot_names + } for snapshots_directory in glob.glob(snapshots_glob): if not os.path.isdir(snapshots_directory): continue - # Reversing the sorted datasets ensures that we unmount the longer mount point paths of - # child datasets before the shorter mount point paths of parent datasets. - for mount_point in reversed(dataset_mount_points): + 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)) - # 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): + # 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: continue # This might fail if the path is already mounted, but we swallow errors here since we'll @@ -435,8 +443,6 @@ def remove_data_source_dumps(hook_config, config, borgmatic_runtime_directory, p shutil.rmtree(snapshot_mount_path, ignore_errors=True) # Destroy snapshots. - full_snapshot_names = get_all_snapshots(zfs_command) - for full_snapshot_name in full_snapshot_names: # Only destroy snapshots that borgmatic actually created! if not full_snapshot_name.split('@')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX): diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py index ccce419f..539a0da3 100644 --- a/tests/unit/hooks/data_source/test_zfs.py +++ b/tests/unit/hooks/data_source/test_zfs.py @@ -223,40 +223,34 @@ def test_get_datasets_to_backup_with_invalid_list_output_raises(): module.get_datasets_to_backup('zfs', patterns=(Pattern('/foo'), Pattern('/bar'))) -def test_get_all_dataset_mount_points_omits_none(): +def test_get_all_dataset_mount_points_omits_none_and_reverse_orders_by_mount_path(): flexmock(module.borgmatic.execute).should_receive( 'execute_command_and_capture_output', ).and_yield( - '/dataset', - 'none', - '/other', + 'dataset\t/path', + 'thing\tnone', + 'other\t/other', ) - flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive( - 'get_contained_patterns', - ).and_return((Pattern('/dataset'),)) - assert module.get_all_dataset_mount_points('zfs') == ( - ('/dataset'), - ('/other'), + assert tuple(module.get_all_dataset_mount_points('zfs').items()) == ( + ('dataset', '/path'), + ('other', '/other'), ) -def test_get_all_dataset_mount_points_omits_duplicates(): +def test_get_all_dataset_mount_points_omits_duplicates_and_reverse_orders_by_mount_path(): flexmock(module.borgmatic.execute).should_receive( 'execute_command_and_capture_output', ).and_return( - '/dataset', - '/other', - '/dataset', - '/other', + 'dataset\t/path', + 'other\t/other', + 'dataset\t/path', + 'other\t/other', ) - flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive( - 'get_contained_patterns', - ).and_return((Pattern('/dataset'),)) - assert module.get_all_dataset_mount_points('zfs') == ( - ('/dataset'), - ('/other'), + assert tuple(module.get_all_dataset_mount_points('zfs').items()) == ( + ('dataset', '/path'), + ('other', '/other'), ) @@ -525,7 +519,12 @@ def test_get_all_snapshots_parses_list_output(): def test_remove_data_source_dumps_unmounts_and_destroys_snapshots(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -533,15 +532,11 @@ 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', ).once() - 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', @@ -557,7 +552,12 @@ def test_remove_data_source_dumps_unmounts_and_destroys_snapshots(): def test_remove_data_source_dumps_use_custom_commands(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -565,15 +565,11 @@ 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', ).once() - 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( '/usr/local/bin/zfs', 'dataset@borgmatic-1234', @@ -639,7 +635,12 @@ def test_remove_data_source_dumps_bails_for_zfs_command_error(): def test_remove_data_source_dumps_bails_for_missing_umount_command(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -647,13 +648,11 @@ 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', ).and_raise(FileNotFoundError) - flexmock(module).should_receive('get_all_snapshots').never() flexmock(module).should_receive('destroy_snapshot').never() hook_config = {'zfs_command': '/usr/local/bin/zfs', 'umount_command': '/usr/local/bin/umount'} @@ -667,7 +666,12 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command(): def test_remove_data_source_dumps_swallows_umount_command_error(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -675,15 +679,11 @@ 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', ).and_raise(module.subprocess.CalledProcessError(1, 'wtf')) - 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( '/usr/local/bin/zfs', 'dataset@borgmatic-1234', @@ -700,7 +700,12 @@ def test_remove_data_source_dumps_swallows_umount_command_error(): def test_remove_data_source_dumps_skips_unmount_snapshot_directories_that_are_not_actually_directories(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -710,9 +715,6 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_directories_that_are_no flexmock(module.os.path).should_receive('isdir').and_return(False) flexmock(module.shutil).should_receive('rmtree').never() 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', @@ -728,7 +730,12 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_directories_that_are_no def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_not_actually_directories(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -741,12 +748,8 @@ 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', @@ -761,8 +764,13 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_no ) -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',)) +def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_for_unknown_shapshots(): + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset', 'sub': '/mnt/dataset/shadow'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -775,14 +783,16 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_em 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.os.path).should_receive('isdir').with_args( + '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset/shadow', + ).and_return(True) 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('unmount_snapshot').with_args( + 'umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset' + ).once() + flexmock(module).should_receive('unmount_snapshot').with_args( + 'umount', '/run/borgmatic/zfs_snapshots/b33f/mnt/dataset/shadow' + ).never() flexmock(module).should_receive('destroy_snapshot').with_args( 'zfs', 'dataset@borgmatic-1234', @@ -798,7 +808,12 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_em def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_after_rmtree_succeeds(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -811,12 +826,8 @@ 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( - ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), - ) flexmock(module).should_receive('destroy_snapshot').with_args( 'zfs', 'dataset@borgmatic-1234', @@ -832,7 +843,12 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_after_rmtre def test_remove_data_source_dumps_with_dry_run_skips_unmount_and_destroy(): - flexmock(module).should_receive('get_all_dataset_mount_points').and_return(('/mnt/dataset',)) + flexmock(module).should_receive('get_all_dataset_mount_points').and_return( + {'dataset': '/mnt/dataset'} + ) + flexmock(module).should_receive('get_all_snapshots').and_return( + ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), + ) flexmock(module.borgmatic.config.paths).should_receive( 'replace_temporary_subdirectory_with_glob', ).and_return('/run/borgmatic') @@ -840,12 +856,8 @@ 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( - ('dataset@borgmatic-1234', 'dataset@other', 'other@other', 'invalid'), - ) flexmock(module).should_receive('destroy_snapshot').never() module.remove_data_source_dumps(