peter/borgmatic
1
0
Fork 0
mirror of https://github.com/borgmatic-collective/borgmatic.git synced 2026-08-01 14:13:01 +02:00
Code Issues Packages Projects Releases Wiki Activity

Fix the ZFS hook to properly unmount snapshots for empty datasets (#1295).

Browse Source
This commit is contained in:
Dan Helfman
2026-04-18 11:43:58 -07:00
parent 3f34d0848e
commit 40d2d521a7
3 changed files with 102 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files
NEWS
+1
View File
@@ -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
borgmatic/hooks/data_source/zfs.py
+22 -16
View File
@@ -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):
tests/unit/hooks/data_source/test_zfs.py
+79 -67
View File
@@ -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(