Merge branch 'main' into same-named-databases

NEWS

@@ -1,6 +1,10 @@
 1.9.5.dev0
  * #418: Backup and restore databases that have the same name but with different ports, hostnames,
    or hooks.
+ * #947: To avoid a hang in the database hooks, error and exit when the borgmatic runtime
+   directory overlaps with the configured excludes.
+ * #954: Fix findmnt command error in the Btrfs hook by switching to parsing JSON output.
+ * When the ZFS, Btrfs, or LVM hooks aren't configured, don't try to cleanup snapshots for them.
 
 1.9.4
  * #80 (beta): Add an LVM hook for snapshotting and backing up LVM logical volumes. See the

borgmatic/borg/create.py

@@ -160,14 +160,24 @@ def any_parent_directories(path, candidate_parents):
 
 
 def collect_special_file_paths(
-    create_command, config, local_path, working_directory, borg_environment, skip_directories
+    create_command,
+    config,
+    local_path,
+    working_directory,
+    borg_environment,
+    borgmatic_runtime_directory,
 ):
     '''
     Given a Borg create command as a tuple, a configuration dict, a local Borg path, a working
-    directory, a dict of environment variables to pass to Borg, and a sequence of parent directories
-    to skip, collect the paths for any special files (character devices, block devices, and named
-    pipes / FIFOs) that Borg would encounter during a create. These are all paths that could cause
-    Borg to hang if its --read-special flag is used.
+    directory, a dict of environment variables to pass to Borg, and the borgmatic runtime directory,
+    collect the paths for any special files (character devices, block devices, and named pipes /
+    FIFOs) that Borg would encounter during a create. These are all paths that could cause Borg to
+    hang if its --read-special flag is used.
+
+    Skip looking for special files in the given borgmatic runtime directory, as borgmatic creates
+    its own special files there for database dumps. And if the borgmatic runtime directory is
+    configured to be excluded from the files Borg backs up, error, because this means Borg won't be
+    able to consume any database dumps and therefore borgmatic will hang.
     '''
     # Omit "--exclude-nodump" from the Borg dry run command, because that flag causes Borg to open
     # files including any named pipe we've created.
@@ -186,12 +196,19 @@ def collect_special_file_paths(
         for path_line in paths_output.split('\n')
         if path_line and path_line.startswith('- ') or path_line.startswith('+ ')
     )
+    skip_paths = {}
 
-    return tuple(
-        path
-        for path in paths
-        if special_file(path) and not any_parent_directories(path, skip_directories)
-    )
+    if os.path.exists(borgmatic_runtime_directory):
+        skip_paths = {
+            path for path in paths if any_parent_directories(path, (borgmatic_runtime_directory,))
+        }
+
+        if not skip_paths:
+            raise ValueError(
+                f'The runtime directory {os.path.normpath(borgmatic_runtime_directory)} overlaps with the configured excludes. Please remove it from excludes or change the runtime directory.'
+            )
+
+    return tuple(path for path in paths if special_file(path) if path not in skip_paths)
 
 
 def check_all_source_directories_exist(source_directories):
@@ -335,9 +352,7 @@ def make_base_create_command(
             local_path,
             working_directory,
             borg_environment,
-            skip_directories=(
-                [borgmatic_runtime_directory] if os.path.exists(borgmatic_runtime_directory) else []
-            ),
+            borgmatic_runtime_directory=borgmatic_runtime_directory,
         )
 
         if special_file_paths:

borgmatic/hooks/data_source/bootstrap.py

@@ -34,7 +34,7 @@ def dump_data_sources(
 
     Return an empty sequence, since there are no ongoing dump processes from this hook.
     '''
-    if hook_config.get('store_config_files') is False:
+    if hook_config and hook_config.get('store_config_files') is False:
         return []
 
     borgmatic_manifest_path = os.path.join(

borgmatic/hooks/data_source/btrfs.py

@@ -1,5 +1,6 @@
 import collections
 import glob
+import json
 import logging
 import os
 import shutil
@@ -26,13 +27,21 @@ def get_filesystem_mount_points(findmnt_command):
     findmnt_output = borgmatic.execute.execute_command_and_capture_output(
         tuple(findmnt_command.split(' '))
         + (
-            '-n',  # No headings.
             '-t',  # Filesystem type.
             'btrfs',
+            '--json',
+            '--list',  # Request a flat list instead of a nested subvolume hierarchy.
         )
     )
 
-    return tuple(line.rstrip().split(' ')[0] for line in findmnt_output.splitlines())
+    try:
+        return tuple(
+            filesystem['target'] for filesystem in json.loads(findmnt_output)['filesystems']
+        )
+    except json.JSONDecodeError as error:
+        raise ValueError(f'Invalid {findmnt_command} JSON output: {error}')
+    except KeyError as error:
+        raise ValueError(f'Invalid {findmnt_command} output: Missing key "{error}"')
 
 
 def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
@@ -257,8 +266,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     '''
     Given a Btrfs configuration dict, a configuration dict, a log prefix, the borgmatic runtime
     directory, and whether this is a dry run, delete any Btrfs snapshots created by borgmatic. Use
-    the log prefix in any log entries. If this is a dry run, then don't actually remove anything.
+    the log prefix in any log entries. If this is a dry run or Btrfs isn't configured in borgmatic's
+    configuration, then don't actually remove anything.
     '''
+    if hook_config is None:
+        return
+
     dry_run_label = ' (dry run; not actually removing anything)' if dry_run else ''
 
     btrfs_command = hook_config.get('btrfs_command', 'btrfs')

borgmatic/hooks/data_source/lvm.py

@@ -288,9 +288,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     '''
     Given an LVM configuration dict, a configuration dict, a log prefix, the borgmatic runtime
     directory, and whether this is a dry run, unmount and delete any LVM snapshots created by
-    borgmatic. Use the log prefix in any log entries. If this is a dry run, then don't actually
-    remove anything.
+    borgmatic. Use the log prefix in any log entries. If this is a dry run or LVM isn't configured
+    in borgmatic's configuration, then don't actually remove anything.
     '''
+    if hook_config is None:
+        return
+
     dry_run_label = ' (dry run; not actually removing anything)' if dry_run else ''
 
     # Unmount snapshots.

borgmatic/hooks/data_source/zfs.py

@@ -283,9 +283,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     '''
     Given a ZFS configuration dict, a configuration dict, a log prefix, the borgmatic runtime
     directory, and whether this is a dry run, unmount and destroy any ZFS snapshots created by
-    borgmatic. Use the log prefix in any log entries. If this is a dry run, then don't actually
-    remove anything.
+    borgmatic. Use the log prefix in any log entries. If this is a dry run or ZFS isn't configured
+    in borgmatic's configuration, then don't actually remove anything.
     '''
+    if hook_config is None:
+        return
+
     dry_run_label = ' (dry run; not actually removing anything)' if dry_run else ''
 
     # Unmount snapshots.

borgmatic/hooks/dispatch.py

@@ -32,7 +32,11 @@ def call_hook(function_name, config, log_prefix, hook_name, *args, **kwargs):
     Raise AttributeError if the function name is not found in the module.
     Raise anything else that the called function raises.
     '''
-    hook_config = config.get(hook_name) or config.get(f'{hook_name}_databases') or {}
+    if hook_name in config or f'{hook_name}_databases' in config:
+        hook_config = config.get(hook_name) or config.get(f'{hook_name}_databases') or {}
+    else:
+        hook_config = None
+
     module_name = hook_name.split('_databases')[0]
 
     # Probe for a data source or monitoring hook module corresponding to the hook name.

tests/end-to-end/commands/fake_findmnt.py

@@ -4,29 +4,38 @@ import sys
 
 def parse_arguments(*unparsed_arguments):
     parser = argparse.ArgumentParser(add_help=False)
-    parser.add_argument('-n', dest='headings', action='store_false', default=True)
     parser.add_argument('-t', dest='type')
+    parser.add_argument('--json', action='store_true')
+    parser.add_argument('--list', action='store_true')
 
     return parser.parse_args(unparsed_arguments)
 
 
-BUILTIN_FILESYSTEM_MOUNT_LINES = (
-    '/mnt/subvolume /dev/loop1 btrfs rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/',
-)
+BUILTIN_FILESYSTEM_MOUNT_OUTPUT = '''{
+       "filesystems": [
+          {
+             "target": "/mnt/subvolume",
+             "source": "/dev/loop0",
+             "fstype": "btrfs",
+             "options": "rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/"
+          }
+       ]
+    }
+    '''
 
 
-def print_filesystem_mounts(arguments):
-    for line in BUILTIN_FILESYSTEM_MOUNT_LINES:
-        print(line)
+def print_filesystem_mounts():
+    print(BUILTIN_FILESYSTEM_MOUNT_OUTPUT)
 
 
 def main():
     arguments = parse_arguments(*sys.argv[1:])
 
-    assert not arguments.headings
     assert arguments.type == 'btrfs'
+    assert arguments.json
+    assert arguments.list
 
-    print_filesystem_mounts(arguments)
+    print_filesystem_mounts()
 
 
 if __name__ == '__main__':

tests/unit/borg/test_create.py

@@ -275,7 +275,8 @@ def test_collect_special_file_paths_parses_special_files_from_borg_dry_run_file_
         'Processing files ...\n- /foo\n+ /bar\n- /baz'
     )
     flexmock(module).should_receive('special_file').and_return(True)
-    flexmock(module).should_receive('any_parent_directories').and_return(False)
+    flexmock(module.os.path).should_receive('exists').and_return(False)
+    flexmock(module).should_receive('any_parent_directories').never()
 
     assert module.collect_special_file_paths(
         ('borg', 'create'),
@@ -283,17 +284,24 @@ def test_collect_special_file_paths_parses_special_files_from_borg_dry_run_file_
         local_path=None,
         working_directory=None,
         borg_environment=None,
-        skip_directories=flexmock(),
+        borgmatic_runtime_directory='/run/borgmatic',
     ) == ('/foo', '/bar', '/baz')
 
 
-def test_collect_special_file_paths_excludes_requested_directories():
+def test_collect_special_file_paths_skips_borgmatic_runtime_directory():
     flexmock(module).should_receive('execute_command_and_capture_output').and_return(
-        '+ /foo\n- /bar\n- /baz'
+        '+ /foo\n- /run/borgmatic/bar\n- /baz'
     )
     flexmock(module).should_receive('special_file').and_return(True)
-    flexmock(module).should_receive('any_parent_directories').and_return(False).and_return(
-        True
+    flexmock(module.os.path).should_receive('exists').and_return(True)
+    flexmock(module).should_receive('any_parent_directories').with_args(
+        '/foo', ('/run/borgmatic',)
+    ).and_return(False)
+    flexmock(module).should_receive('any_parent_directories').with_args(
+        '/run/borgmatic/bar', ('/run/borgmatic',)
+    ).and_return(True)
+    flexmock(module).should_receive('any_parent_directories').with_args(
+        '/baz', ('/run/borgmatic',)
     ).and_return(False)
 
     assert module.collect_special_file_paths(
@@ -302,10 +310,29 @@ def test_collect_special_file_paths_excludes_requested_directories():
         local_path=None,
         working_directory=None,
         borg_environment=None,
-        skip_directories=flexmock(),
+        borgmatic_runtime_directory='/run/borgmatic',
     ) == ('/foo', '/baz')
 
 
+def test_collect_special_file_paths_with_borgmatic_runtime_directory_missing_from_paths_output_errors():
+    flexmock(module).should_receive('execute_command_and_capture_output').and_return(
+        '+ /foo\n- /bar\n- /baz'
+    )
+    flexmock(module).should_receive('special_file').and_return(True)
+    flexmock(module.os.path).should_receive('exists').and_return(True)
+    flexmock(module).should_receive('any_parent_directories').and_return(False)
+
+    with pytest.raises(ValueError):
+        module.collect_special_file_paths(
+            ('borg', 'create'),
+            config={},
+            local_path=None,
+            working_directory=None,
+            borg_environment=None,
+            borgmatic_runtime_directory='/run/borgmatic',
+        )
+
+
 def test_collect_special_file_paths_excludes_non_special_files():
     flexmock(module).should_receive('execute_command_and_capture_output').and_return(
         '+ /foo\n+ /bar\n+ /baz'
@@ -313,7 +340,8 @@ def test_collect_special_file_paths_excludes_non_special_files():
     flexmock(module).should_receive('special_file').and_return(True).and_return(False).and_return(
         True
     )
-    flexmock(module).should_receive('any_parent_directories').and_return(False)
+    flexmock(module.os.path).should_receive('exists').and_return(False)
+    flexmock(module).should_receive('any_parent_directories').never()
 
     assert module.collect_special_file_paths(
         ('borg', 'create'),
@@ -321,7 +349,7 @@ def test_collect_special_file_paths_excludes_non_special_files():
         local_path=None,
         working_directory=None,
         borg_environment=None,
-        skip_directories=flexmock(),
+        borgmatic_runtime_directory='/run/borgmatic',
     ) == ('/foo', '/baz')
 
 
@@ -335,7 +363,8 @@ def test_collect_special_file_paths_omits_exclude_no_dump_flag_from_command():
         borg_exit_codes=None,
     ).and_return('Processing files ...\n- /foo\n+ /bar\n- /baz').once()
     flexmock(module).should_receive('special_file').and_return(True)
-    flexmock(module).should_receive('any_parent_directories').and_return(False)
+    flexmock(module.os.path).should_receive('exists').and_return(False)
+    flexmock(module).should_receive('any_parent_directories').never()
 
     module.collect_special_file_paths(
         ('borg', 'create', '--exclude-nodump'),
@@ -343,7 +372,7 @@ def test_collect_special_file_paths_omits_exclude_no_dump_flag_from_command():
         local_path='borg',
         working_directory=None,
         borg_environment=None,
-        skip_directories=flexmock(),
+        borgmatic_runtime_directory='/run/borgmatic',
     )
 
 

tests/unit/hooks/data_source/test_bootstrap.py

@@ -22,7 +22,7 @@ def test_dump_data_sources_creates_manifest_file():
     ).once()
 
     module.dump_data_sources(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         config_paths=('test.yaml',),
@@ -53,7 +53,7 @@ def test_dump_data_sources_with_dry_run_does_not_create_manifest_file():
     flexmock(module.json).should_receive('dump').never()
 
     module.dump_data_sources(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         config_paths=('test.yaml',),
@@ -74,7 +74,7 @@ def test_remove_data_source_dumps_deletes_manifest_and_parent_directory():
     flexmock(module.os).should_receive('rmdir').with_args('/run/borgmatic/bootstrap').once()
 
     module.remove_data_source_dumps(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         borgmatic_runtime_directory='/run/borgmatic',
@@ -91,7 +91,7 @@ def test_remove_data_source_dumps_with_dry_run_bails():
     flexmock(module.os).should_receive('rmdir').never()
 
     module.remove_data_source_dumps(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         borgmatic_runtime_directory='/run/borgmatic',
@@ -110,7 +110,7 @@ def test_remove_data_source_dumps_swallows_manifest_file_not_found_error():
     flexmock(module.os).should_receive('rmdir').with_args('/run/borgmatic/bootstrap').once()
 
     module.remove_data_source_dumps(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         borgmatic_runtime_directory='/run/borgmatic',
@@ -131,7 +131,7 @@ def test_remove_data_source_dumps_swallows_manifest_parent_directory_not_found_e
     ).once()
 
     module.remove_data_source_dumps(
-        hook_config={},
+        hook_config=None,
         config={},
         log_prefix='test',
         borgmatic_runtime_directory='/run/borgmatic',

tests/unit/hooks/data_source/test_btrfs.py

@@ -1,3 +1,4 @@
+import pytest
 from flexmock import flexmock
 
 from borgmatic.hooks.data_source import btrfs as module
@@ -7,13 +8,46 @@ def test_get_filesystem_mount_points_parses_findmnt_output():
     flexmock(module.borgmatic.execute).should_receive(
         'execute_command_and_capture_output'
     ).and_return(
-        '/mnt0   /dev/loop0 btrfs  rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/\n'
-        '/mnt1   /dev/loop1 btrfs  rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/\n'
+        '''{
+           "filesystems": [
+              {
+                 "target": "/mnt0",
+                 "source": "/dev/loop0",
+                 "fstype": "btrfs",
+                 "options": "rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/"
+              },
+              {
+                 "target": "/mnt1",
+                 "source": "/dev/loop0",
+                 "fstype": "btrfs",
+                 "options": "rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/"
+              }
+           ]
+        }
+        '''
     )
 
     assert module.get_filesystem_mount_points('findmnt') == ('/mnt0', '/mnt1')
 
 
+def test_get_filesystem_mount_points_with_invalid_findmnt_json_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return('{')
+
+    with pytest.raises(ValueError):
+        module.get_filesystem_mount_points('findmnt')
+
+
+def test_get_filesystem_mount_points_with_findmnt_json_missing_filesystems_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return('{"wtf": "something is wrong here"}')
+
+    with pytest.raises(ValueError):
+        module.get_filesystem_mount_points('findmnt')
+
+
 def test_get_subvolumes_for_filesystem_parses_subvolume_list_output():
     flexmock(module.borgmatic.execute).should_receive(
         'execute_command_and_capture_output'
@@ -451,6 +485,24 @@ def test_remove_data_source_dumps_deletes_snapshots():
     )
 
 
+def test_remove_data_source_dumps_without_hook_configuration_bails():
+    flexmock(module).should_receive('get_subvolumes').never()
+    flexmock(module).should_receive('make_snapshot_path').never()
+    flexmock(module.borgmatic.config.paths).should_receive(
+        'replace_temporary_subdirectory_with_glob'
+    ).never()
+    flexmock(module).should_receive('delete_snapshot').never()
+    flexmock(module.shutil).should_receive('rmtree').never()
+
+    module.remove_data_source_dumps(
+        hook_config=None,
+        config={'source_directories': '/mnt/subvolume'},
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
 def test_remove_data_source_dumps_with_get_subvolumes_file_not_found_error_bails():
     config = {'btrfs': {}}
     flexmock(module).should_receive('get_subvolumes').and_raise(FileNotFoundError)

tests/unit/hooks/data_source/test_lvm.py

@@ -673,6 +673,23 @@ def test_remove_data_source_dumps_unmounts_and_remove_snapshots():
     )
 
 
+def test_remove_data_source_dumps_bails_for_missing_lvm_configuration():
+    flexmock(module).should_receive('get_logical_volumes').never()
+    flexmock(module.borgmatic.config.paths).should_receive(
+        'replace_temporary_subdirectory_with_glob'
+    ).never()
+    flexmock(module).should_receive('unmount_snapshot').never()
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=None,
+        config={'source_directories': '/mnt/lvolume'},
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
 def test_remove_data_source_dumps_bails_for_missing_lsblk_command():
     config = {'lvm': {}}
     flexmock(module).should_receive('get_logical_volumes').and_raise(FileNotFoundError)

tests/unit/hooks/data_source/test_zfs.py

@@ -321,6 +321,21 @@ def test_remove_data_source_dumps_use_custom_commands():
     )
 
 
+def test_remove_data_source_dumps_bails_for_missing_hook_configuration():
+    flexmock(module).should_receive('get_all_dataset_mount_points').never()
+    flexmock(module.borgmatic.config.paths).should_receive(
+        'replace_temporary_subdirectory_with_glob'
+    ).never()
+
+    module.remove_data_source_dumps(
+        hook_config=None,
+        config={'source_directories': '/mnt/dataset'},
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
 def test_remove_data_source_dumps_bails_for_missing_zfs_command():
     flexmock(module).should_receive('get_all_dataset_mount_points').and_raise(FileNotFoundError)
     flexmock(module.borgmatic.config.paths).should_receive(

tests/unit/hooks/test_dispatch.py

@@ -95,7 +95,7 @@ def test_call_hook_without_hook_config_invokes_module_function_with_arguments_an
         'borgmatic.hooks.monitoring.super_hook'
     ).and_return(test_module)
     flexmock(test_module).should_receive('hook_function').with_args(
-        {}, config, 'prefix', 55, value=66
+        None, config, 'prefix', 55, value=66
     ).and_return(expected_return_value).once()
 
     return_value = module.call_hook('hook_function', config, 'prefix', 'super_hook', 55, value=66)