From 27d167b071eaa5a5950dc41fb2c5ef31074a5489 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Sun, 1 Dec 2024 20:13:02 -0800
Subject: [PATCH 01/33] LVM snapshots WIP (#80).

---
 borgmatic/config/schema.yaml       |  37 ++++
 borgmatic/hooks/data_source/lvm.py | 343 +++++++++++++++++++++++++++++
 2 files changed, 380 insertions(+)
 create mode 100644 borgmatic/hooks/data_source/lvm.py

diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml
index c2e900ef..b06ffd5b 100644
--- a/borgmatic/config/schema.yaml
+++ b/borgmatic/config/schema.yaml
@@ -2304,3 +2304,40 @@ properties:
                 example: /usr/local/bin/findmnt
         description: |
             Configuration for integration with the Btrfs filesystem.
+    lvm:
+        type: ["object", "null"]
+        additionalProperties: false
+        properties:
+            lvcreate_command:
+                type: string
+                description: |
+                    Command to use instead of "lvcreate".
+                example: /usr/local/bin/lvcreate
+            lvremove_command:
+                type: string
+                description: |
+                    Command to use instead of "lvremove".
+                example: /usr/local/bin/lvremove
+            lvs_command:
+                type: string
+                description: |
+                    Command to use instead of "lvs".
+                example: /usr/local/bin/lvs
+            lsbrk_command:
+                type: string
+                description: |
+                    Command to use instead of "lsbrk".
+                example: /usr/local/bin/lsbrk
+            mount_command:
+                type: string
+                description: |
+                    Command to use instead of "mount".
+                example: /usr/local/bin/mount
+            umount_command:
+                type: string
+                description: |
+                    Command to use instead of "umount".
+                example: /usr/local/bin/umount
+        description: |
+            Configuration for integration with Linux LVM (Logical Volume
+            Manger).
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
new file mode 100644
index 00000000..8a1b78ca
--- /dev/null
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -0,0 +1,343 @@
+import glob
+import json
+import logging
+import os
+import shutil
+import subprocess
+
+import borgmatic.config.paths
+import borgmatic.execute
+
+logger = logging.getLogger(__name__)
+
+
+def use_streaming(hook_config, config, log_prefix):  # pragma: no cover
+    '''
+    Return whether dump streaming is used for this hook. (Spoiler: It isn't.)
+    '''
+    return False
+
+
+BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
+
+
+def get_logical_volumes(lsblk_command, source_directories=None):
+    '''
+    Given an lsblk command to run and a sequence of configured source directories, find the
+    intersection between the current LVM logical volume mount points and the configured borgmatic
+    source directories. The idea is that these are the requested logical volumes to snapshot.
+
+    If source directories is None, include all logical volume mounts points, not just those in
+    source directories.
+
+    Return the result as a sequence of (device name, device path, mount point) pairs.
+    '''
+    try:
+        devices_info = json.loads(
+            subprocess.check_output(
+                (
+                    # Use lsblk instead of lvs here because lvs can't show active mounts.
+                    lsblk_command,
+                    '--output',
+                    'name,path,mountpoint,type',
+                    '--json',
+                    '--list',
+                )
+            )
+        )
+    except json.JSONDecodeError as error:
+        raise ValueError('Invalid {lsblk_command} JSON output: {error}')
+
+    source_directories_set = set(source_directories or ())
+
+    try:
+        return tuple(
+            (device['name'], device['path'], device['mountpoint'])
+            for device in devices_info['blockdevices']
+            if device['mountpoint'] and device['type'] == 'lvm'
+            if not source_directories or device['mountpoint'] in source_directories_set
+        )
+    except KeyError as error:
+        raise ValueError(f'Invalid {lsblk_command} output: Missing key "{error}"')
+
+
+def snapshot_logical_volume(
+    lvcreate_command, snapshot_name, logical_volume_device
+):  # pragma: no cover
+    '''
+    Given an lvcreate command to run, a snapshot name, and the path to the logical volume device to
+    snapshot, create a new LVM snapshot.
+    '''
+    borgmatic.execute.execute_command(
+        (
+            lvcreate_command,
+            '--snapshot',
+            '--extents',
+            '1',  # The snapshot doesn't need much disk space because it's read-only.
+            '--name',
+            snapshot_name,
+            logical_volume_device,
+        ),
+        output_log_level=logging.DEBUG,
+    )
+
+
+def mount_snapshot(mount_command, snapshot_device, snapshot_mount_path):  # pragma: no cover
+    '''
+    Given a mount command to run, the device path for an existing snapshot, and the path where the
+    snapshot should be mounted, mount the snapshot as read-only (making any necessary directories
+    first).
+    '''
+    os.makedirs(snapshot_mount_path, mode=0o700, exist_ok=True)
+
+    borgmatic.execute.execute_command(
+        (
+            mount_command,
+            '-o',
+            'ro',
+            snapshot_device,
+            snapshot_mount_path,
+        ),
+        output_log_level=logging.DEBUG,
+    )
+
+
+def dump_data_sources(
+    hook_config,
+    config,
+    log_prefix,
+    config_paths,
+    borgmatic_runtime_directory,
+    source_directories,
+    dry_run,
+):
+    '''
+    Given an LVM configuration dict, a configuration dict, a log prefix, the borgmatic configuration
+    file paths, the borgmatic runtime directory, the configured source directories, and whether this
+    is a dry run, auto-detect and snapshot any LVM logical volume mount points listed in the given
+    source directories. Also update those source directories, replacing logical volume mount points
+    with corresponding snapshot directories so they get stored in the Borg archive instead. Use the
+    log prefix in any log entries.
+
+    Return an empty sequence, since there are no ongoing dump processes from this hook.
+
+    If this is a dry run, then don't actually snapshot anything.
+    '''
+    dry_run_label = ' (dry run; not actually snapshotting anything)' if dry_run else ''
+    logger.info(f'{log_prefix}: Snapshotting LVM logical volumes{dry_run_label}')
+
+    # List logical volumes to get their mount points.
+    lsblk_command = hook_config.get('lsblk_command', 'lsblk')
+    requested_logical_volumes = get_logical_volumes(lsblk_command, source_directories)
+
+    # Snapshot each logical volume, rewriting source directories to use the snapshot paths.
+    snapshot_suffix = f'{BORGMATIC_SNAPSHOT_PREFIX}{os.getpid()}'
+
+    if not requested_logical_volumes:
+        logger.warning(f'{log_prefix}: No LVM logical volumes found to snapshot{dry_run_label}')
+
+    for device_name, device_path, mount_point in requested_logical_volumes:
+        snapshot_name = f'{device_name}_{snapshot_suffix}'
+        logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name}{dry_run_label}')
+
+        if not dry_run:
+            snapshot_logical_volume(
+                hook_config.get('lvcreate_command', 'lvcreate'), snapshot_name, device_path
+            )
+
+        # Get the device path for the device path for the snapshot we just created.
+        try:
+            (_, snapshot_device_path) = get_snapshots(hook_config.get('lvs_command', 'lvs'), snapshot_name=snapshot_name)[0]
+        except IndexError:
+            raise ValueError(f'Cannot find LVM snapshot {snapshot_name}')
+
+        # Mount the snapshot into a particular named temporary directory so that the snapshot ends
+        # up in the Borg archive at the "original" logical volume mount point path.
+        snapshot_mount_path_for_borg = os.path.join(
+            os.path.normpath(borgmatic_runtime_directory),
+            'lvm_snapshots',
+            '.',  # Borg 1.4+ "slashdot" hack.
+            mount_point.lstrip(os.path.sep),
+        )
+        snapshot_mount_path = os.path.normpath(snapshot_mount_path_for_borg)
+
+        logger.debug(
+            f'{log_prefix}: Mounting LVM snapshot {snapshot_name} at {snapshot_mount_path}{dry_run_label}'
+        )
+
+        if not dry_run:
+            mount_snapshot(
+                hook_config.get('mount_command', 'mount'), snapshot_device_path, snapshot_mount_path
+            )
+
+            if mount_point in source_directories:
+                source_directories.remove(mount_point)
+
+            source_directories.append(snapshot_mount_path_for_borg)
+
+    return []
+
+
+def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
+    '''
+    Given a umount command to run and the mount path of a snapshot, unmount it.
+    '''
+    borgmatic.execute.execute_command(
+        (
+            umount_command,
+            snapshot_mount_path,
+        ),
+        output_log_level=logging.DEBUG,
+    )
+
+
+def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
+    '''
+    Given an lvremote command to run and the device path of a snapshot, remove it it.
+    '''
+    borgmatic.execute.execute_command(
+        (
+            lvremove_command,
+            '--force',  # Suppress an interactive "are you sure?" type prompt.
+            snapshot_device_path,
+        ),
+        output_log_level=logging.DEBUG,
+    )
+
+
+def get_snapshots(lvs_command, snapshot_name=None):
+    '''
+    Given an lvs command to run, return all LVM snapshots as a sequence of (snapshot name, snapshot
+    device path) pairs.
+
+    If a snapshot name is given, filter the results to that snapshot.
+    '''
+    try:
+        snapshot_info = json.loads(
+            borgmatic.execute.execute_command_and_capture_output(
+                (
+                    # Use lvs instead of lsblk here because lsblk can't filter to just snapshots.
+                    lvs_command,
+                    '--report-format',
+                    'json',
+                    '--options',
+                    'lv_name,lv_path',
+                    '--select',
+                    'lv_attr =~ ^s',  # Filter to just snapshots.
+                )
+            )
+        )
+    except json.JSONDecodeError as error:
+        raise ValueError('Invalid {lvs_command} JSON output: {error}')
+
+    try:
+        return tuple(
+            (snapshot['lv_name'], snapshot['lv_path'])
+            for snapshot in snapshot_info['report'][0]['lv']
+            if snapshot_name is None or snapshot['lv_name'] == snapshot_name
+        )
+    except KeyError as error:
+        raise ValueError(f'Invalid {lvs_command} output: Missing key "{error}"')
+
+
+def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_directory, dry_run):
+    '''
+    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.
+    '''
+    dry_run_label = ' (dry run; not actually removing anything)' if dry_run else ''
+
+    # Unmount snapshots.
+    try:
+        logical_volumes = get_logical_volumes(hook_config.get('lsblk_command', 'lsblk'))
+    except FileNotFoundError:
+        logger.debug(f'{log_prefix}: Could not find "{lsblk_command}" command')
+        return
+    except subprocess.CalledProcessError as error:
+        logger.debug(f'{log_prefix}: {error}')
+        return
+
+    snapshots_glob = os.path.join(
+        borgmatic.config.paths.replace_temporary_subdirectory_with_glob(
+            os.path.normpath(borgmatic_runtime_directory),
+        ),
+        'lvm_snapshots',
+    )
+    logger.debug(
+        f'{log_prefix}: Looking for snapshots to remove in {snapshots_glob}{dry_run_label}'
+    )
+    umount_command = hook_config.get('umount_command', 'umount')
+
+    for snapshots_directory in glob.glob(snapshots_glob):
+        if not os.path.isdir(snapshots_directory):
+            continue
+
+        # This might fail if the directory is already mounted, but we swallow errors here since
+        # we'll try again below. The point of doing it here is that we don't want to try to unmount
+        # a non-mounted directory (which *will* fail).
+        if not dry_run:
+            shutil.rmtree(snapshots_directory, ignore_errors=True)
+
+        for _, _, mount_point in logical_volumes:
+            snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep))
+            if not os.path.isdir(snapshot_mount_path):
+                continue
+
+            logger.debug(
+                f'{log_prefix}: Unmounting LVM snapshot at {snapshot_mount_path}{dry_run_label}'
+            )
+
+            if not dry_run:
+                try:
+                    unmount_snapshot(umount_command, snapshot_mount_path)
+                except FileNotFoundError:
+                    logger.debug(f'{log_prefix}: Could not find "{umount_command}" command')
+                    return
+                except subprocess.CalledProcessError as error:
+                    logger.debug(f'{log_prefix}: {error}')
+                    return
+
+        if not dry_run:
+            shutil.rmtree(snapshots_directory)
+
+    # Delete snapshots.
+    lvremove_command = hook_config.get('lvremove_command', 'lvremove')
+
+    for snapshot_name, snapshot_device_path in get_snapshots(
+        hook_config.get('lvs_command', 'lvs')
+    ):
+        # Only delete snapshots that borgmatic actually created!
+        if not snapshot_name.split('_')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX):
+            continue
+
+        logger.debug(f'{log_prefix}: Deleting LVM snapshot {snapshot_name}{dry_run_label}')
+
+        if not dry_run:
+            delete_snapshot(lvremove_command, snapshot_device_path)
+
+
+def make_data_source_dump_patterns(
+    hook_config, config, log_prefix, borgmatic_runtime_directory, name=None
+):  # pragma: no cover
+    '''
+    Restores aren't implemented, because stored files can be extracted directly with "extract".
+    '''
+    return ()
+
+
+def restore_data_source_dump(
+    hook_config,
+    config,
+    log_prefix,
+    data_source,
+    dry_run,
+    extract_process,
+    connection_params,
+    borgmatic_runtime_directory,
+):  # pragma: no cover
+    '''
+    Restores aren't implemented, because stored files can be extracted directly with "extract".
+    '''
+    raise NotImplementedError()

From 1e8f73779f5ab5a5dda952369475ae595e8e4af2 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Sun, 1 Dec 2024 20:25:16 -0800
Subject: [PATCH 02/33] Fix typo in schema comment (#80).

---
 borgmatic/config/schema.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml
index b06ffd5b..98e59e82 100644
--- a/borgmatic/config/schema.yaml
+++ b/borgmatic/config/schema.yaml
@@ -2340,4 +2340,4 @@ properties:
                 example: /usr/local/bin/umount
         description: |
             Configuration for integration with Linux LVM (Logical Volume
-            Manger).
+            Manager).

From cd654cbb57fc1516adb4f8e9ffb9771c1969eb93 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Sun, 1 Dec 2024 21:00:11 -0800
Subject: [PATCH 03/33] Fix a few docstring typos (#80).

---
 borgmatic/hooks/data_source/lvm.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 8a1b78ca..6a3b7568 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -30,7 +30,7 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     If source directories is None, include all logical volume mounts points, not just those in
     source directories.
 
-    Return the result as a sequence of (device name, device path, mount point) pairs.
+    Return the result as a sequence of (device name, device path, mount point) tuples.
     '''
     try:
         devices_info = json.loads(
@@ -193,7 +193,7 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
 
 def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
     '''
-    Given an lvremote command to run and the device path of a snapshot, remove it it.
+    Given an lvremove command to run and the device path of a snapshot, remove it it.
     '''
     borgmatic.execute.execute_command(
         (

From 6367a0001320f50e793022b467363cbc1fd69fe6 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Mon, 2 Dec 2024 11:09:07 -0800
Subject: [PATCH 04/33] Add snapshot_size option (#80).

---
 borgmatic/config/schema.yaml             | 10 +++
 borgmatic/hooks/data_source/lvm.py       | 16 +++--
 docs/how-to/snapshot-your-filesystems.md | 89 ++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml
index 98e59e82..e0a7faf3 100644
--- a/borgmatic/config/schema.yaml
+++ b/borgmatic/config/schema.yaml
@@ -2308,6 +2308,16 @@ properties:
         type: ["object", "null"]
         additionalProperties: false
         properties:
+            snapshot_size:
+                type: string
+                description: |
+                    Size to allocate for each snapshot taken, including the
+                    units to use for that size. Defaults to "10%ORIGIN" (10%
+                    of the size of logical volume being snapshotted). See the
+                    lvcreate "--size" and "--extents" documentation for more
+                    information:
+                    https://www.man7.org/linux/man-pages/man8/lvcreate.8.html
+                example: 5GB
             lvcreate_command:
                 type: string
                 description: |
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 6a3b7568..cbca7ff3 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -62,18 +62,18 @@ def get_logical_volumes(lsblk_command, source_directories=None):
 
 
 def snapshot_logical_volume(
-    lvcreate_command, snapshot_name, logical_volume_device
+    lvcreate_command, snapshot_name, logical_volume_device, snapshot_size,
 ):  # pragma: no cover
     '''
-    Given an lvcreate command to run, a snapshot name, and the path to the logical volume device to
-    snapshot, create a new LVM snapshot.
+    Given an lvcreate command to run, a snapshot name, the path to the logical volume device to
+    snapshot, and a snapshot size string, create a new LVM snapshot.
     '''
     borgmatic.execute.execute_command(
         (
             lvcreate_command,
             '--snapshot',
-            '--extents',
-            '1',  # The snapshot doesn't need much disk space because it's read-only.
+            ('--extents' if '%' in snapshot_size else '--size'),
+            snapshot_size,
             '--name',
             snapshot_name,
             logical_volume_device,
@@ -102,6 +102,9 @@ def mount_snapshot(mount_command, snapshot_device, snapshot_mount_path):  # prag
     )
 
 
+DEFAULT_SNAPSHOT_SIZE = '10%ORIGIN'
+
+
 def dump_data_sources(
     hook_config,
     config,
@@ -142,7 +145,8 @@ def dump_data_sources(
 
         if not dry_run:
             snapshot_logical_volume(
-                hook_config.get('lvcreate_command', 'lvcreate'), snapshot_name, device_path
+                hook_config.get('lvcreate_command', 'lvcreate'), snapshot_name, device_path,
+                hook_config.get('snapshot_size', DEFAULT_SNAPSHOT_SIZE),
             )
 
         # Get the device path for the device path for the snapshot we just created.
diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md
index d8f5327f..1330ccee 100644
--- a/docs/how-to/snapshot-your-filesystems.md
+++ b/docs/how-to/snapshot-your-filesystems.md
@@ -145,3 +145,92 @@ Subvolume snapshots are stored in a Borg archive as normal files, so you can use
 the standard [extract
 action](https://torsion.org/borgmatic/docs/how-to/extract-a-backup/) to extract
 them.
+
+
+### LVM
+
+<span class="minilink minilink-addedin">New in version 1.9.4</span> <span
+class="minilink minilink-addedin">Beta feature</span> borgmatic supports
+taking snapshots with [LVM](https://sourceware.org/lvm2/) (Linux Logical
+Volume Manager) and sending those snapshots to Borg for backup. LVM isn't
+itself a filesystem, but it can take snapshots at the layer right below your
+filesystem.
+
+To use this feature, first you need one or more mounted LVM logical volumes.
+Then, enable LVM within borgmatic by adding the following line to your
+configuration file:
+
+```yaml
+lvm:
+```
+
+No other options are necessary to enable LVM support, but if desired you can
+override some of the options used by the LVM hook. For instance:
+
+```yaml
+lvm:
+    snapshot_size: 5GB  # See below for details.
+    lvcreate_command: /usr/local/bin/lvcreate
+    lvremove_command: /usr/local/bin/lvremove
+    lvs_command: /usr/local/bin/lvs
+    lsbrk_command: /usr/local/bin/lsbrk
+    mount_command: /usr/local/bin/mount
+    umount_command: /usr/local/bin/umount
+```
+
+As long as the LVM hook is in beta, it may be subject to breaking changes
+and/or may not work well for your use cases. But feel free to use it in
+production if you're okay with these caveats, and please [provide any
+feedback](https://torsion.org/borgmatic/#issues) you have on this feature.
+
+
+#### Snapshot size
+
+The `snapshot_size` option is the size to allocate for each snapshot taken,
+including the units to use for that size. While borgmatic's snapshots
+themselves are read-only and don't change during backups, the logical volume
+being snapshotted *can* change—therefore requiring additional snapshot storage
+since LVM snapshots are copy-on-write. And if the configured snapshot size is
+too small (and LVM isn't configured to grow snapshots automatically), then the
+snapshots will fail to allocate enough space, resulting in a broken backup.
+
+If not specified, the `snapshot_size` option defaults to `10%ORIGIN`, which
+means 10% of the size of logical volume being snapshotted. See the [`lvcreate
+--size` and `--extents`
+documentation](https://www.man7.org/linux/man-pages/man8/lvcreate.8.html) for
+more information about possible values here. (Under the hood, borgmatic uses
+`lvcreate --extents` if the `snapshot_size` is a percentage value, and
+`lvcreate --size` otherwise.)
+
+
+#### Logical volume discovery
+
+For any logical volume you'd like backed up, add its mount point to
+borgmatic's `source_directories` option.
+
+During a backup, borgmatic automatically snapshots these discovered logical
+volumes (non-recursively), temporary mounts the snapshots within its [runtime
+directory](https://torsion.org/borgmatic/docs/how-to/backup-your-databases/#runtime-directory),
+and includes the snapshotted files in the paths sent to Borg. borgmatic is
+also responsible for cleaning up (deleting) these snapshots after a backup
+completes.
+
+Additionally, borgmatic rewrites the snapshot file paths so that they appear
+at their original logical volume locations in a Borg archive. For instance, if
+your logical volume is mounted at `/mnt/lvolume`, then the snapshotted files
+will appear in an archive at `/mnt/lvolume` as well.
+
+<span class="minilink minilink-addedin">With Borg version 1.2 and
+earlier</span>Snapshotted files are instead stored at a path dependent on the
+[runtime
+directory](https://torsion.org/borgmatic/docs/how-to/backup-your-databases/#runtime-directory)
+in use at the time the archive was created, as Borg 1.2 and earlier do not
+support path rewriting.
+
+
+#### Extract a logical volume
+
+Logical volume snapshots are stored in a Borg archive as normal files, so
+you can use the standard
+[extract action](https://torsion.org/borgmatic/docs/how-to/extract-a-backup/) to
+extract them.

From 4453c2d49c17f5326ce843c28669cafad50354fb Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Mon, 2 Dec 2024 11:28:57 -0800
Subject: [PATCH 05/33] Add LVM logo to integrations docs.

---
 README.md           |   1 +
 docs/static/lvm.png | Bin 0 -> 5216 bytes
 2 files changed, 1 insertion(+)
 create mode 100644 docs/static/lvm.png

diff --git a/README.md b/README.md
index 5861a9a7..a7e52cb1 100644
--- a/README.md
+++ b/README.md
@@ -63,6 +63,7 @@ borgmatic is powered by [Borg Backup](https://www.borgbackup.org/).
 <a href="https://sqlite.org/"><img src="docs/static/sqlite.png" alt="SQLite" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
 <a href="https://openzfs.org/"><img src="docs/static/openzfs.png" alt="OpenZFS" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
 <a href="https://btrfs.readthedocs.io/"><img src="docs/static/btrfs.png" alt="Btrfs" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
+<a href="https://sourceware.org/lvm2/"><img src="docs/static/lvm.png" alt="LVM" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
 <a href="https://rclone.org"><img src="docs/static/rclone.png" alt="rclone" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
 <a href="https://healthchecks.io/"><img src="docs/static/healthchecks.png" alt="Healthchecks" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
 <a href="https://uptime.kuma.pet/"><img src="docs/static/uptimekuma.png" alt="Uptime Kuma" height="60px" style="margin-bottom:20px; margin-right:20px;"></a>
diff --git a/docs/static/lvm.png b/docs/static/lvm.png
new file mode 100644
index 0000000000000000000000000000000000000000..b271d879a62febd3535d5bab62aaff53c3f1523b
GIT binary patch
literal 5216
zcmcgw_dgrl_ovmOrKqZ?6|*)mON|Cmv1`>{F&@;Ym@!LQE2?5|^#rvywQHutXwBMc
zG>w%i8neE6zJJ8$zFv2oU+%f*p7;BlbK{K+p3>iXaEpS1f*z`^X-q*u$wXeC(cC1D
zZyeyEWI>F(hK3PTLj&mR=i}=B!i9oDC?z~aMZ5nA$DqB*AcM%QJ>dGg_e%2+2!|(2
zf)UU3t!kw#o3k7G5775mpY}Gdtzn&JdIv3^Q$GhYUQoOY^fmTRxqFp)&uSppcPTlD
z#1%xOc>l~XXNHAZc3FOE7@0LzouE=n1AYR~_?epRG72@m)xEL9McKVF7@Tp{riP8m
zX$3Gc#&3*Dd9Bm0XX`JnbsBOw3RnuLJ}i(TL=Pm}JXem`u<^HC%Vv^EEqM5rF>&yY
zxl&fTUQ%$*qY4kDt=pE#X5-d*%{ocV`@{BtksVQ-O_^c>|NVL@THbrpMp`%Iotm~W
z7TQx0eOR^iGXX#0O_7)s7^T@i?`+jhxG(l+MfCI$(DzhdF4rG%!=6k?e5raoPO9IK
zHH=!5@hxe+oVh7=4n=fTLz1I<z1G&-tA#^9ZQYT&aQ<Ans{_tae^1d__*G#m>X-PJ
zo*&1v3s}PSckgSA6+ZRyNZDa_0}6vkfq(uGv&|#bBxd{)m%y!5<X5@rr2ABpJd@k0
zMn@!hqV?6bgi}yR@cq{*gG<x`$wF#>s6K@HC*3t>cCLG{OBS+-%^zanui@k6<>Kv6
zq2cG^=<nhT408AX2ly1KZ}j}t9aaj8yNXavuxaqZ-m-NQCnO(xU~jg!B&F3C#cHJO
zPo*_`*H?=z;K{v#Cr_f0zD(NJURH<Xld|z41!YJIMJSc@^?0Q9M=>@wiGPa>K2e%l
zEV1QLQ8CfH+TG8I-It2>HJH3w=pyZRou6BZGK|!Ow|5|}Ix5<??eCHm{9m`Cpz*fq
zmGgftS=AqfY8sv<uAD1x<#my+`Zy0ASI#31Iy0V49>lcUmz|LE_~+IU=VduH8v)&~
z=ZFNrMpwLxWU6fLmxEVtF<bErwS~Hf7jf9`qA5aH*<e6xXN{x>j4_225#aRolm~ej
z&-t3M&k0%x73+5WhAqUOri)j%oA8If0#3$=;k?t2bes;jnN(Fzm`d#u=NiR2>M6^7
z3otR(Cychwc1X=fe~CKnf+Y^918ekKLF7*w{8I5i12Z~MVU5Ezlo<J~0dby%xlO}+
ztPH8mNE6WdZRV8>TscoitLt!#A6``4%pYM9--)&Zx=VmDT#{ITkD*wH%_Qo+{o~BD
z4N>amLhmnaVGvrMc&%><?LBp4U`-z6H07+}8^&?3oq!#Gr7~TstZ&hl%T0#|E3Gb3
z$A_RH6B=H*?Z6I_HbSue@$>@aEZ6vDx}Q;pdxVg?`R(>1(dpPWQ1u;NbTOtGx)*{s
z$n-b8T|56CZEWp0_W>~Noui>nqigl$Mpvg~Z}}EjGo+w4^~lc3fmEIAw${~SGdYSZ
zwp7eoo<4V|+G)tMn9Gc{+lh(8%<+2}h=snMTrom5U=&QYfPZjF;d+mQaxi?@6UVj&
zABOu*acTIFZHHAR9;A%5e|fhsq1O1;AE~(I%2qmYTsq9k=g?HIs&bt(T019}|1!vl
z!eflWli?6nAWmIREin{a_<ciE_D$k%bsAP3r4Q@Jh%6vl!d*NT?p=@RxAqPH0kz7o
z_#?s+^R549sYUc$v_Tns0WiC)(|?SJ1EN<-PU|tPCf6<j3Na5`)g{W$WEZiaTWiNg
zkMM!oXGz~x0j+FRPqBa=yMsnUZA1)k(t7pu%|OF*j|;d7CU8-{641$&VTtyZ3Hz%I
zJ;Vx~SabOXi8$&ggE#vP-%t1UyoN2%t?W>-O!b@85b827CPojJ8af>V=eHju0*ZO5
z4Q8Sm^#`x1L~Ao}1&m+v9$=+gu2-#`Ps2lPCS&@Q{{S|`pvl?`?k_Cgc9OM!gW;=a
z8R%Hvk7?4}PC47O;U(08I=SGcdbbU~A_Jrw`M1z5Y$JIVE!|pOF1Br=%&^X<lU&i-
z^bKO?3eegz*<9Rw9<0Ph>2Ex;n7A1dVXF}xmj^GEdkm9e59y}EF?W($GkcLh<Ctai
z?86LBtTJCY-oFc@rm%M6mc|g<;hd`-5U&;dtbp{_ss8|PnX%_p4iYprk?~EM{2|97
zO4tOo-@^_74EMi3knddyP$k2n-)R3GV_(R>RxigDaZB$CD>-C-uT0%7AD#jYMH+@(
zLy_Ngo6Zcs)Q@ga*pzkEQ<9i^P|j2!<7b09@TrJ+G!%@(T)D4|MFFKR8H56ie9MRU
zPKk6SlAQL-b%#dtC2bm$5*kv<`g~dYy-KH`iNosDYYB=lZm{zM%Z%mXCe$zZ5YMSk
zcH3SmEyP}ux{RGwlYcJ(wBmlQGMmd5PN9UBkPG|!GW?NkNr;&e*co99ofFL9JeANd
zz?WaIV4p_m?VH?S3L#dzbDMi0UHLLAN7P(|8e9^9nvCTKu}$XgY~PbJk7k%ukNNW9
ztNKE$?6O7ebEDx_S*?`-=j!bb&vKGpjIpEfr}|VWe|5%b)JCpXZE?^Zc0IeJr8ZMH
zHQ$<i6^J_U-eB-D{ej}x&THIFxx^$uf~Znd6&Oqkt5_?4T&mn;(Qvj<>NfmkIrZr<
z8#MTG#rfi`z=KzikS&Wm7YF|-(^h3L#&y}7<PDt=yqC|e{_!8Gm>;~*LS3P=STS6S
z{9afoV)32JyjR243}<@&?<(8ua`Wn`y98|b2i|gngp{4hBdMZ=Y4U{y(jzKuM4i4K
zDe^rWsiS?K9rn{Q`tw?Z1(fYO3}v5sw8f+}W2lH9hYHPY>g~&vx}UA4{o+%p@3qZl
z8RFe3JH4Xi4gZ~^dL~lkvGsNok@H(ic2yHJ^q9X(RtL<FYshsOb;fu3cz8OOCN!&>
z?Ts|-4iOMdWD7NG8M`^!RUpd;G|)$6vdCVl%#sl57AI(J1j+f8bu#uTipaB6dL)+G
zbGW~yrApc6G6i)^ctp@Rx=$I_OcC-#2#K_iLUwaOQbIL#Yaw-iGi|{9M@R^n!=&s|
z<1a7V4hNK?<Mg*hM~4{d{zw%e(i1Rh|Nc}HxEv48&yjcAoEmR~3>8J%SHH^n=+OQO
zE!~&-xSo4z%_LWnaDntAGRUNc>g!=wn;;=8=Qw9}tW|AJD-js??ZPsi?R&2Br|m97
z&enNe!VFWS)QhyenI_L~_jz#Q6OGCf3eyo+jI5&y%iCNcp{^Q+o-<LkkknnFRSC<g
ze?I%*J<)Gvn&OD*N7um4CtTvG`xcz$616@#(`{m8GIHP$nGW^gbnqG|R`yK%NpZb`
zDuq%Pag8JuJU_t>I?kF!>AGZ`LarRDaE=pf+2X?uRqJDZ-gICLkAkhS)`?%pWi&bD
zU{MO|cEq%pQJ6^yTK)l!F5o}Q-kq2J>4g@*aSe=_2&~lZhP~y}Uq~pj0iGNnixspH
z@H`RO@8VNcjuIofpV_6q*1~w=FwdPlv`*4>vaFUKRw5peULK~w$CZX;s;K+i^5JFd
zs#ll<TMAlU{SLxinLchN^+%PI*vS6?&fM;ttbJAssy6ZT+@zv+d}h}M%^_}tFlpYj
z$c7qj3hpj0YW!%P*{~`kgPEToz7fEo<vAd&fLSEw)6nvEE&E%$aLh;+{%|Wl)52X!
z>9(NOLc6}!>Fo!EK1Cm2NPKl*wTLj8{A#Ag0b3Gurv_u%+C|U@7Ox%05p+BkPbTg8
z?=2UpCjC-?OtEeWjM5X%8>phSi3-azIyjohk~d8(;O($<5U0`doTMq8rd18DhzwDF
z!nJ;Ukih~h6FBK0DWI2|fw_@yfIs5lAmYg!&~iTk^jB|^Xl-1UWRpftV(cfCHBxr?
z_juw^QPUXZet?6x+lO|fU*)24;a0ISF4+f|_&Y~dflR-RT+)4oqJ%n1E{~@zDVJS=
zvY^yZ?yd*0&M^yDm%>QHr;2z3xu&-f<xTunR4F|1=vl$ETtb(<>W3IQ9<dDnVD+z_
zH@}K`qmh)ueCKUKwWE>NZ8z9pd4O-t>m++?6L>y`#GFv`U=38GBbrg`WgYHn8JLN=
z0@;0t7-M@mbboW7?4(SZT6MHy?r`-s7=ONW)W+i_#h}u*c(YNN;QH6c*~8tc9`P6r
z4`NK9(ki;?i_xYlG?F(C<?6{#VEGCBvnSsAn1*)Cy!E5%+;EF(g=FIIs_PY(Bkbyo
zY$)n9`K)Hs7MEq=vF~YZbQ76lXDx^!#h{*gy=A!8kCBY4KZD+_p^{+drjjNt*US)u
z^WwbZ2C4Tyt&+S2N$nW1k#il}NM&T59%Lo6LY}UDoSa@Ms6<vd(Z88VJ`2_pzFt9m
zs7J~D*fPUw_K00&uXe|RRrk5zkC5(&FLQA~<DO~MFMIP8Vv~1l1>jfXSv)4+v@Edv
z^DWnBbH$xKD9~TqEmhC2$B4i$tZ~4}!iSNQ4$f*fEpSWBGR)(S^m3=Y&2@q99+qjD
zbx@Mg9}3>MBkZ36)GyX@fOe(H$kOXooim1tgX_tkA350d=P^kV{QB{$A{};ADIoN3
z{z5W6e;k6HyunBcq*?19ha=s?J}Z(qkKd(raL`@lz5kr#GM5OHmKfPo@!`K$U9Fq?
zO=ZDM;z@t3_5HbFdA_zXNXV>ydu&_2Fc=fLVpdNqhoTNDbr$tJ><^uBD)n=MNtS}7
zoX-2O&KV0=$hyxivq%f(!?rL>ZYs0p&@X~q6B{|>&~?tW|1t$?1=IANjhuf@%LA>P
zCpw3A$bd$7)ENo2WIJW*<V@kX1IMd%gbqcaJ;e*9vwMNWl%ocakbQ~~Lx#7J{O<lk
zFlLqYk=NdEFAuUr<zk-M2^E=d^p<kw`T!CsQ4#QLr5-etO1He)A<#wk0a!WP_nCeQ
zPp6=thL#zZqH=+sG9dP>TXHU`-qG6qR=v{QTw;Y%4|SE~zZ1uvfnbv!|Gqg&@EuTQ
zg>_C?ly2vx0149lq`x3sgLP~;n1(kd1W|3Ku5}{KT>+h^S9<gvz-Cl!CpjD2cd*rS
zeP)>)2ti@QV;x;2gpmzQK3?*93hT^Wy3y+)TdBvbrY2hw+xOX0{{|B?%|CQ#2|5YJ
z$>Is=zcFf~S*?e@_$A@%N-gq>;^)Pk+#G8?6Y%kTuaoXQG?zeJ(+(k^6@$Oq&c^y6
zy7vZcKsz6<q0mU<2ckaEy&J>J^26UY8*cW($7)8?q~-?LIf|5zsk}QPo+A722z5Rj
zcene>s#%O2QX<3&AXp;t;*v@Lx4TX{RPK2%%o+`p{#N!}N9%-KuE>al?KgskY8M&G
z!?Tw>otK4u+V@AW`EWNt_x62gB!67fR3*B_;`Shb;alIY70J|>ZXJM)e6(GW6`K3j
z927C(+hsU;kG(vWalFCbB@fQ5-+qTGrFS)0xKTZm<EJ2e-1eWEi~DYVwZl6ul|{b=
z7<j^b64sn7UyyV%Tu6+X3wxAMLjZ166wj#V21b3&u{wAyhTF#|AucG{@TdU*Y5Cl(
zlph!Iggs3P9|x3<7_BuW&6NmuNSizR)q-0WFUw!#hKlDb?d_6lmkRAgqh^7Q?w7C9
z79E6abCMd|kXPNM_FUe3BfC7Qq2V*ai^ZK9a_+kE$`cdiR$mlJ?-G`HsF?U&kcHNF
zKJD$?ZXm+_8Zn%v8eR;qY1!^?g%Eol6Cx$#FUkY-l_R4+|MFHoEOJF1^a*=w-!zRS
zhUl(j0X9}NA=rwuQYWIbWgs7zD%C4!T*XNY-hzoi?2cU2Ebgsfw`xygodFI3<KY^R
zgeW50-vL*s53^>Sq-Ww<5o6ybtuFR+L2`P4bT~P!8mlPC3Vn-76>Qe7XtJ4;J6@vY
z?e97mW&8fHwgmcfq%#+Pdb1|XemBE04;LC)0pH(D;EvQ9oTiATQ9b+D{9vU=8o!i%
zc12%PRU4=<{1M4nO5qc-(X0+JCyC7BGW3&xlMU<>3td^v>L=X^X791br#`)|p{KP8
zuV*T&=c}KRuX?FEbRq+jvT)rSd4wmsQ>M*+cV)mX@6d?}RFX;4x9IPI2keOjzqN!x
zyxx9{hoxPuQy8Wu6jn^YFG0ltFq_>jdnv8-t0>ed&o<g+-kf-OrE}&#$lFLa<_JZ|
z;2QKV2B_ERq1I+E##au5(tW~*Wa6~X&Ox5^7a9flWO~8`F~vU#6S+G@N*xZbBTY7k
z6wEPc$`8Nuh}x$cTf?A!vGn9p&QShWpz0vH-ym`UuMY*{WU;!SA+LbQa`=7()X*=4
zT13QAh7_(`!!$pc2Ar&N^;Ns6#%9E6fb&!DDzUOqkS2-GEOU^dOoBbkUW)xO`1|-p
zf{M#FB${TIL5gKKdK--fJUAvJ%ZeE^v}m@uArJ>FXS11ODAe2U_p$UgONpI%5GJTa
zP^RS_Ycw&Quzd&Eu$2o+av&A(d$u}bKWufA^=g!xJaiDUhRK&oRJ#i)=xpEB{8D-D
zGC`v@pGXcc8`AQ9J|h9TW-;Su&mM!Lwc*E;=#N(Pi{?dRPN;M%zVWu$!91@2^)Jf*
bpO<yX{c(E)i82JQ{<nuh3^Z%h9ismatzpy<

literal 0
HcmV?d00001


From 27305ec2bfd42a39e8ad0ce3dde1bedc79103e7f Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Mon, 2 Dec 2024 12:01:04 -0800
Subject: [PATCH 06/33] Clarify the path rewriting for LVM (but also ZFS +
 Btrfs) (#80).

---
 docs/how-to/snapshot-your-filesystems.md | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md
index 1330ccee..8df985ab 100644
--- a/docs/how-to/snapshot-your-filesystems.md
+++ b/docs/how-to/snapshot-your-filesystems.md
@@ -71,8 +71,10 @@ completes.
 
 Additionally, borgmatic rewrites the snapshot file paths so that they appear
 at their original dataset locations in a Borg archive. For instance, if your
-dataset is mounted at `/mnt/dataset`, then the snapshotted files will appear
-in an archive at `/mnt/dataset` as well.
+dataset is mounted at `/var/dataset`, then the snapshotted files will appear
+in an archive at `/var/dataset` as well—even if borgmatic has to mount the
+snapshot somewhere in `/run/user/1000/borgmatic/zfs_snapshots/` to perform the
+backup.
 
 <span class="minilink minilink-addedin">With Borg version 1.2 and
 earlier</span>Snapshotted files are instead stored at a path dependent on the
@@ -128,10 +130,12 @@ subvolumes (non-recursively) and includes the snapshotted files in the paths
 sent to Borg. borgmatic is also responsible for cleaning up (deleting) these
 snapshots after a backup completes.
 
-Additionally, borgmatic rewrites the snapshot file paths so that they appear at
-their original subvolume locations in a Borg archive. For instance, if your
-subvolume exists at `/mnt/subvolume`, then the snapshotted files will appear in
-an archive at `/mnt/subvolume` as well.
+Additionally, borgmatic rewrites the snapshot file paths so that they appear
+at their original subvolume locations in a Borg archive. For instance, if your
+subvolume exists at `/var/subvolume`, then the snapshotted files will appear
+in an archive at `/var/subvolume` as well—even if borgmatic has to mount the
+snapshot somewhere in `/var/subvolume/.borgmatic-snapshot-1234/` to perform
+the backup.
 
 <span class="minilink minilink-addedin">With Borg version 1.2 and
 earlier</span>Snapshotted files are instead stored at a path dependent on the
@@ -217,8 +221,10 @@ completes.
 
 Additionally, borgmatic rewrites the snapshot file paths so that they appear
 at their original logical volume locations in a Borg archive. For instance, if
-your logical volume is mounted at `/mnt/lvolume`, then the snapshotted files
-will appear in an archive at `/mnt/lvolume` as well.
+your logical volume is mounted at `/var/lvolume`, then the snapshotted files
+will appear in an archive at `/var/lvolume` as well—even if borgmatic has to
+mount the snapshot somewhere in `/run/user/1000/borgmatic/lvm_snapshots/` to
+perform the backup.
 
 <span class="minilink minilink-addedin">With Borg version 1.2 and
 earlier</span>Snapshotted files are instead stored at a path dependent on the

From 88fd1ae454ff7ad5aca682af353b686b30a69b80 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Mon, 2 Dec 2024 20:58:50 -0800
Subject: [PATCH 07/33] Discover parent/grandparent/etc. logical volumes of
 source directories (#80).

---
 borgmatic/hooks/data_source/lvm.py       | 69 +++++++++++++++++-------
 docs/how-to/snapshot-your-filesystems.md |  6 +++
 2 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index cbca7ff3..4a91332c 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -2,6 +2,7 @@ import glob
 import json
 import logging
 import os
+import pathlib
 import shutil
 import subprocess
 
@@ -21,6 +22,23 @@ def use_streaming(hook_config, config, log_prefix):  # pragma: no cover
 BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
 
 
+def get_contained_source_directories(mount_point, source_directories):
+    '''
+    Given a mount point and a sequence of source directories, get the subset of source directories
+    for which the mount point is the same as that source directory, a parent of it, a grandparent,
+    etc. The idea is if, say, /var/log and /var/lib are source directories, but there's a logical
+    volume mount point at /var, then /var is what we want to snapshot.
+    '''
+    if not source_directories:
+        return ()
+
+    return tuple(
+        source_directory
+        for source_directory in source_directories
+        if mount_point == source_directory or pathlib.PurePosixPath(mount_point) in pathlib.PurePath(source_directory).parents
+    )
+
+
 def get_logical_volumes(lsblk_command, source_directories=None):
     '''
     Given an lsblk command to run and a sequence of configured source directories, find the
@@ -30,7 +48,8 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     If source directories is None, include all logical volume mounts points, not just those in
     source directories.
 
-    Return the result as a sequence of (device name, device path, mount point) tuples.
+    Return the result as a sequence of (device name, device path, mount point, sequence of contained
+    source directories) tuples.
     '''
     try:
         devices_info = json.loads(
@@ -48,14 +67,13 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     except json.JSONDecodeError as error:
         raise ValueError('Invalid {lsblk_command} JSON output: {error}')
 
-    source_directories_set = set(source_directories or ())
-
     try:
         return tuple(
-            (device['name'], device['path'], device['mountpoint'])
+            (device['name'], device['path'], device['mountpoint'], contained_source_directories)
             for device in devices_info['blockdevices']
             if device['mountpoint'] and device['type'] == 'lvm'
-            if not source_directories or device['mountpoint'] in source_directories_set
+            for contained_source_directories in (get_contained_source_directories(device['mountpoint'], source_directories),)
+            if not source_directories or contained_source_directories
         )
     except KeyError as error:
         raise ValueError(f'Invalid {lsblk_command} output: Missing key "{error}"')
@@ -135,11 +153,12 @@ def dump_data_sources(
 
     # Snapshot each logical volume, rewriting source directories to use the snapshot paths.
     snapshot_suffix = f'{BORGMATIC_SNAPSHOT_PREFIX}{os.getpid()}'
+    normalized_runtime_directory = os.path.normpath(borgmatic_runtime_directory)
 
     if not requested_logical_volumes:
         logger.warning(f'{log_prefix}: No LVM logical volumes found to snapshot{dry_run_label}')
 
-    for device_name, device_path, mount_point in requested_logical_volumes:
+    for device_name, device_path, mount_point, contained_source_directories in requested_logical_volumes:
         snapshot_name = f'{device_name}_{snapshot_suffix}'
         logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name}{dry_run_label}')
 
@@ -157,28 +176,40 @@ def dump_data_sources(
 
         # Mount the snapshot into a particular named temporary directory so that the snapshot ends
         # up in the Borg archive at the "original" logical volume mount point path.
-        snapshot_mount_path_for_borg = os.path.join(
-            os.path.normpath(borgmatic_runtime_directory),
+        snapshot_mount_path = os.path.join(
+            normalized_runtime_directory,
             'lvm_snapshots',
-            '.',  # Borg 1.4+ "slashdot" hack.
             mount_point.lstrip(os.path.sep),
         )
-        snapshot_mount_path = os.path.normpath(snapshot_mount_path_for_borg)
 
         logger.debug(
             f'{log_prefix}: Mounting LVM snapshot {snapshot_name} at {snapshot_mount_path}{dry_run_label}'
         )
 
-        if not dry_run:
-            mount_snapshot(
-                hook_config.get('mount_command', 'mount'), snapshot_device_path, snapshot_mount_path
+        if dry_run:
+            continue
+
+        mount_snapshot(
+            hook_config.get('mount_command', 'mount'), snapshot_device_path, snapshot_mount_path
+        )
+
+        # Update the path for each contained source directory, so Borg sees it within the
+        # mounted snapshot.
+        for source_directory in contained_source_directories:
+            try:
+                source_directories.remove(source_directory)
+            except ValueError:
+                pass
+
+            source_directories.append(
+                os.path.join(
+                    normalized_runtime_directory,
+                    'lvm_snapshots',
+                    '.',  # Borg 1.4+ "slashdot" hack.
+                    source_directory.lstrip(os.path.sep),
+                )
             )
 
-            if mount_point in source_directories:
-                source_directories.remove(mount_point)
-
-            source_directories.append(snapshot_mount_path_for_borg)
-
     return []
 
 
@@ -284,7 +315,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         if not dry_run:
             shutil.rmtree(snapshots_directory, ignore_errors=True)
 
-        for _, _, mount_point in logical_volumes:
+        for _, _, mount_point, _ in logical_volumes:
             snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep))
             if not os.path.isdir(snapshot_mount_path):
                 continue
diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md
index 8df985ab..c572b067 100644
--- a/docs/how-to/snapshot-your-filesystems.md
+++ b/docs/how-to/snapshot-your-filesystems.md
@@ -219,6 +219,12 @@ and includes the snapshotted files in the paths sent to Borg. borgmatic is
 also responsible for cleaning up (deleting) these snapshots after a backup
 completes.
 
+borgmatic is smart enough to look at the parent (and grandparent, etc.)
+directories of each of your `source_directories` to discover any logical
+volumes. For instance, let's say you add `/var/log` and `/var/lib` to your
+source directories, but `/var` is a logical volume. borgmatic will discover
+that and snapshot `/var` accordingly.
+
 Additionally, borgmatic rewrites the snapshot file paths so that they appear
 at their original logical volume locations in a Borg archive. For instance, if
 your logical volume is mounted at `/var/lvolume`, then the snapshotted files

From 9aaa3c925f8a2925193cdcd6556ba63070d1ab4c Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Mon, 2 Dec 2024 21:01:34 -0800
Subject: [PATCH 08/33] Code formatting (#80).

---
 borgmatic/hooks/data_source/lvm.py | 31 +++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 4a91332c..2948a047 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -35,7 +35,8 @@ def get_contained_source_directories(mount_point, source_directories):
     return tuple(
         source_directory
         for source_directory in source_directories
-        if mount_point == source_directory or pathlib.PurePosixPath(mount_point) in pathlib.PurePath(source_directory).parents
+        if mount_point == source_directory
+        or pathlib.PurePosixPath(mount_point) in pathlib.PurePath(source_directory).parents
     )
 
 
@@ -72,7 +73,9 @@ def get_logical_volumes(lsblk_command, source_directories=None):
             (device['name'], device['path'], device['mountpoint'], contained_source_directories)
             for device in devices_info['blockdevices']
             if device['mountpoint'] and device['type'] == 'lvm'
-            for contained_source_directories in (get_contained_source_directories(device['mountpoint'], source_directories),)
+            for contained_source_directories in (
+                get_contained_source_directories(device['mountpoint'], source_directories),
+            )
             if not source_directories or contained_source_directories
         )
     except KeyError as error:
@@ -80,7 +83,10 @@ def get_logical_volumes(lsblk_command, source_directories=None):
 
 
 def snapshot_logical_volume(
-    lvcreate_command, snapshot_name, logical_volume_device, snapshot_size,
+    lvcreate_command,
+    snapshot_name,
+    logical_volume_device,
+    snapshot_size,
 ):  # pragma: no cover
     '''
     Given an lvcreate command to run, a snapshot name, the path to the logical volume device to
@@ -158,19 +164,28 @@ def dump_data_sources(
     if not requested_logical_volumes:
         logger.warning(f'{log_prefix}: No LVM logical volumes found to snapshot{dry_run_label}')
 
-    for device_name, device_path, mount_point, contained_source_directories in requested_logical_volumes:
+    for (
+        device_name,
+        device_path,
+        mount_point,
+        contained_source_directories,
+    ) in requested_logical_volumes:
         snapshot_name = f'{device_name}_{snapshot_suffix}'
         logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name}{dry_run_label}')
 
         if not dry_run:
             snapshot_logical_volume(
-                hook_config.get('lvcreate_command', 'lvcreate'), snapshot_name, device_path,
+                hook_config.get('lvcreate_command', 'lvcreate'),
+                snapshot_name,
+                device_path,
                 hook_config.get('snapshot_size', DEFAULT_SNAPSHOT_SIZE),
             )
 
         # Get the device path for the device path for the snapshot we just created.
         try:
-            (_, snapshot_device_path) = get_snapshots(hook_config.get('lvs_command', 'lvs'), snapshot_name=snapshot_name)[0]
+            (_, snapshot_device_path) = get_snapshots(
+                hook_config.get('lvs_command', 'lvs'), snapshot_name=snapshot_name
+            )[0]
         except IndexError:
             raise ValueError(f'Cannot find LVM snapshot {snapshot_name}')
 
@@ -340,9 +355,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     # Delete snapshots.
     lvremove_command = hook_config.get('lvremove_command', 'lvremove')
 
-    for snapshot_name, snapshot_device_path in get_snapshots(
-        hook_config.get('lvs_command', 'lvs')
-    ):
+    for snapshot_name, snapshot_device_path in get_snapshots(hook_config.get('lvs_command', 'lvs')):
         # Only delete snapshots that borgmatic actually created!
         if not snapshot_name.split('_')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX):
             continue

From 8a6225b7c2f61740a7be8e151804e55a54787041 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 08:51:10 -0800
Subject: [PATCH 09/33] Factor out logic for finding contained source
 directories in a parent directory (#80).

---
 borgmatic/hooks/data_source/lvm.py      | 26 +++++--------------------
 borgmatic/hooks/data_source/snapshot.py | 23 ++++++++++++++++++++++
 2 files changed, 28 insertions(+), 21 deletions(-)
 create mode 100644 borgmatic/hooks/data_source/snapshot.py

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 2948a047..feef0e33 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -2,11 +2,11 @@ import glob
 import json
 import logging
 import os
-import pathlib
 import shutil
 import subprocess
 
 import borgmatic.config.paths
+import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
 
 logger = logging.getLogger(__name__)
@@ -22,24 +22,6 @@ def use_streaming(hook_config, config, log_prefix):  # pragma: no cover
 BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
 
 
-def get_contained_source_directories(mount_point, source_directories):
-    '''
-    Given a mount point and a sequence of source directories, get the subset of source directories
-    for which the mount point is the same as that source directory, a parent of it, a grandparent,
-    etc. The idea is if, say, /var/log and /var/lib are source directories, but there's a logical
-    volume mount point at /var, then /var is what we want to snapshot.
-    '''
-    if not source_directories:
-        return ()
-
-    return tuple(
-        source_directory
-        for source_directory in source_directories
-        if mount_point == source_directory
-        or pathlib.PurePosixPath(mount_point) in pathlib.PurePath(source_directory).parents
-    )
-
-
 def get_logical_volumes(lsblk_command, source_directories=None):
     '''
     Given an lsblk command to run and a sequence of configured source directories, find the
@@ -74,7 +56,9 @@ def get_logical_volumes(lsblk_command, source_directories=None):
             for device in devices_info['blockdevices']
             if device['mountpoint'] and device['type'] == 'lvm'
             for contained_source_directories in (
-                get_contained_source_directories(device['mountpoint'], source_directories),
+                borgmatic.hooks.data_source.snapshot.get_contained_directories(
+                    device['mountpoint'], source_directories
+                ),
             )
             if not source_directories or contained_source_directories
         )
@@ -171,7 +155,7 @@ def dump_data_sources(
         contained_source_directories,
     ) in requested_logical_volumes:
         snapshot_name = f'{device_name}_{snapshot_suffix}'
-        logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name}{dry_run_label}')
+        logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name} of {mount_point}{dry_run_label}')
 
         if not dry_run:
             snapshot_logical_volume(
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
new file mode 100644
index 00000000..097b51ea
--- /dev/null
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -0,0 +1,23 @@
+import pathlib
+
+
+IS_A_HOOK = False
+
+
+def get_contained_directories(parent_directory, candidate_contained_directories):
+    '''
+    Given a parent directory and a sequence of candiate directories potentially inside it, get the
+    subset of contained directories for which the parent directory is actually the parent, a
+    grandparent, the very same directory, etc. The idea is if, say, /var/log and /var/lib are
+    candidate contained directories, but there's a parent directory (logical volume, dataset,
+    subvolume, etc.) at /var, then /var is what we want to snapshot.
+    '''
+    if not candidate_contained_directories:
+        return ()
+
+    return tuple(
+        candidate
+        for candidate in candidate_contained_directories
+        if parent_directory == candidate
+        or pathlib.PurePosixPath(parent_directory) in pathlib.PurePath(candidate).parents
+    )

From bfeea5d394e7be20b46b67ad13c4b3cc7b578da1 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 08:52:05 -0800
Subject: [PATCH 10/33] Code formatting (#80).

---
 borgmatic/hooks/data_source/lvm.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index feef0e33..8ee1d03c 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -155,7 +155,9 @@ def dump_data_sources(
         contained_source_directories,
     ) in requested_logical_volumes:
         snapshot_name = f'{device_name}_{snapshot_suffix}'
-        logger.debug(f'{log_prefix}: Creating LVM snapshot {snapshot_name} of {mount_point}{dry_run_label}')
+        logger.debug(
+            f'{log_prefix}: Creating LVM snapshot {snapshot_name} of {mount_point}{dry_run_label}'
+        )
 
         if not dry_run:
             snapshot_logical_volume(

From 9b77de3d660f62947a9b830b611dbe9fd764b886 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 11:05:45 -0800
Subject: [PATCH 11/33] Port the parent directory discovery logic from LVM to
 ZFS (#80).

---
 borgmatic/hooks/data_source/lvm.py       |   5 +-
 borgmatic/hooks/data_source/snapshot.py  |  19 +++--
 borgmatic/hooks/data_source/zfs.py       | 102 ++++++++++++++++-------
 docs/how-to/snapshot-your-filesystems.md |   8 ++
 4 files changed, 95 insertions(+), 39 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 8ee1d03c..ffdf9e14 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -50,6 +50,9 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     except json.JSONDecodeError as error:
         raise ValueError('Invalid {lsblk_command} JSON output: {error}')
 
+
+    candidate_source_directories = set(source_directories or ())
+
     try:
         return tuple(
             (device['name'], device['path'], device['mountpoint'], contained_source_directories)
@@ -57,7 +60,7 @@ def get_logical_volumes(lsblk_command, source_directories=None):
             if device['mountpoint'] and device['type'] == 'lvm'
             for contained_source_directories in (
                 borgmatic.hooks.data_source.snapshot.get_contained_directories(
-                    device['mountpoint'], source_directories
+                    device['mountpoint'], candidate_source_directories
                 ),
             )
             if not source_directories or contained_source_directories
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index 097b51ea..b03b01d9 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -1,3 +1,4 @@
+import itertools
 import pathlib
 
 
@@ -6,18 +7,24 @@ IS_A_HOOK = False
 
 def get_contained_directories(parent_directory, candidate_contained_directories):
     '''
-    Given a parent directory and a sequence of candiate directories potentially inside it, get the
-    subset of contained directories for which the parent directory is actually the parent, a
-    grandparent, the very same directory, etc. The idea is if, say, /var/log and /var/lib are
-    candidate contained directories, but there's a parent directory (logical volume, dataset,
-    subvolume, etc.) at /var, then /var is what we want to snapshot.
+    Given a parent directory and a set of candiate directories potentially inside it, get the subset
+    of contained directories for which the parent directory is actually the parent, a grandparent,
+    the very same directory, etc. The idea is if, say, /var/log and /var/lib are candidate contained
+    directories, but there's a parent directory (logical volume, dataset, subvolume, etc.) at /var,
+    then /var is what we want to snapshot.
+
+    Also mutate the given set of candidate contained directories to remove any actually contained
+    directories from it.
     '''
     if not candidate_contained_directories:
         return ()
 
-    return tuple(
+    contained = tuple(
         candidate
         for candidate in candidate_contained_directories
         if parent_directory == candidate
         or pathlib.PurePosixPath(parent_directory) in pathlib.PurePath(candidate).parents
     )
+    candidate_contained_directories -= set(contained)
+
+    return contained
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 5f011a1c..a56489e0 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -1,3 +1,4 @@
+import collections
 import glob
 import logging
 import os
@@ -5,6 +6,7 @@ import shutil
 import subprocess
 
 import borgmatic.config.paths
+import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
 
 logger = logging.getLogger(__name__)
@@ -21,6 +23,9 @@ BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
 BORGMATIC_USER_PROPERTY = 'org.torsion.borgmatic:backup'
 
 
+Dataset = collections.namedtuple('Dataset', ('name', 'mount_point', 'user_property_value', 'contained_source_directories'))
+
+
 def get_datasets_to_backup(zfs_command, source_directories):
     '''
     Given a ZFS command to run and a sequence of configured source directories, find the
@@ -29,7 +34,7 @@ def get_datasets_to_backup(zfs_command, source_directories):
     datasets tagged with a borgmatic-specific user property, whether or not they appear in source
     directories.
 
-    Return the result as a sequence of (dataset name, mount point) pairs.
+    Return the result as a sequence of Dataset instances, sorted by mount point.
     '''
     list_output = borgmatic.execute.execute_command_and_capture_output(
         (
@@ -42,23 +47,44 @@ def get_datasets_to_backup(zfs_command, source_directories):
             f'name,mountpoint,{BORGMATIC_USER_PROPERTY}',
         )
     )
-    source_directories_set = set(source_directories)
 
     try:
-        return tuple(
-            (dataset_name, mount_point)
-            for line in list_output.splitlines()
-            for (dataset_name, mount_point, user_property_value) in (line.rstrip().split('\t'),)
-            if mount_point in source_directories_set or user_property_value == 'auto'
+        # Sort from longest to shortest mount points, so longer mount points get a whack at the
+        # candidate source directory piñata before their parents do. (Source directories are
+        # consumed during the second loop below, so no two datasets get the same contained source
+        # directories.)
+        datasets = sorted(
+            (
+                Dataset(dataset_name, mount_point, user_property_value, ())
+                for line in list_output.splitlines()
+                for (dataset_name, mount_point, user_property_value) in (line.rstrip().split('\t'),)
+            ),
+            key=lambda dataset: dataset.mount_point,
+            reverse=True,
         )
     except ValueError:
         raise ValueError('Invalid {zfs_command} list output')
 
+    candidate_source_directories = set(source_directories)
 
-def get_all_datasets(zfs_command):
+    return sorted(
+        tuple(
+            Dataset(dataset.name, dataset.mount_point, dataset.user_property_value, contained_source_directories)
+            for dataset in datasets
+            for contained_source_directories in (
+                borgmatic.hooks.data_source.snapshot.get_contained_directories(
+                    dataset.mount_point, candidate_source_directories
+                ),
+            )
+            if contained_source_directories or dataset.user_property_value == 'auto'
+        ),
+        key=lambda dataset: dataset.mount_point,
+    )
+
+
+def get_all_dataset_mount_points(zfs_command):
     '''
-    Given a ZFS command to run, return all ZFS datasets as a sequence of (dataset name, mount point)
-    pairs.
+    Given a ZFS command to run, return all ZFS datasets as a sequence of sorted mount points.
     '''
     list_output = borgmatic.execute.execute_command_and_capture_output(
         (
@@ -68,15 +94,13 @@ def get_all_datasets(zfs_command):
             '-t',
             'filesystem',
             '-o',
-            'name,mountpoint',
+            'mountpoint',
         )
     )
 
     try:
         return tuple(
-            (dataset_name, mount_point)
-            for line in list_output.splitlines()
-            for (dataset_name, mount_point) in (line.rstrip().split('\t'),)
+            sorted(line.rstrip() for line in list_output.splitlines())
         )
     except ValueError:
         raise ValueError('Invalid {zfs_command} list output')
@@ -147,40 +171,52 @@ def dump_data_sources(
 
     # Snapshot each dataset, rewriting source directories to use the snapshot paths.
     snapshot_name = f'{BORGMATIC_SNAPSHOT_PREFIX}{os.getpid()}'
+    normalized_runtime_directory = os.path.normpath(borgmatic_runtime_directory)
 
     if not requested_datasets:
         logger.warning(f'{log_prefix}: No ZFS datasets found to snapshot{dry_run_label}')
 
-    for dataset_name, mount_point in requested_datasets:
-        full_snapshot_name = f'{dataset_name}@{snapshot_name}'
-        logger.debug(f'{log_prefix}: Creating ZFS snapshot {full_snapshot_name}{dry_run_label}')
+    for dataset in requested_datasets:
+        full_snapshot_name = f'{dataset.name}@{snapshot_name}'
+        logger.debug(f'{log_prefix}: Creating ZFS snapshot {full_snapshot_name} of {dataset.mount_point}{dry_run_label}')
 
         if not dry_run:
             snapshot_dataset(zfs_command, full_snapshot_name)
 
         # Mount the snapshot into a particular named temporary directory so that the snapshot ends
         # up in the Borg archive at the "original" dataset mount point path.
-        snapshot_mount_path_for_borg = os.path.join(
-            os.path.normpath(borgmatic_runtime_directory),
+        snapshot_mount_path = os.path.join(
+            normalized_runtime_directory,
             'zfs_snapshots',
-            '.',  # Borg 1.4+ "slashdot" hack.
-            mount_point.lstrip(os.path.sep),
+            dataset.mount_point.lstrip(os.path.sep),
         )
-        snapshot_mount_path = os.path.normpath(snapshot_mount_path_for_borg)
+
         logger.debug(
             f'{log_prefix}: Mounting ZFS snapshot {full_snapshot_name} at {snapshot_mount_path}{dry_run_label}'
         )
 
-        if not dry_run:
-            mount_snapshot(
-                hook_config.get('mount_command', 'mount'), full_snapshot_name, snapshot_mount_path
+        if dry_run:
+            continue
+
+        mount_snapshot(
+            hook_config.get('mount_command', 'mount'), full_snapshot_name, snapshot_mount_path
+        )
+
+        for source_directory in dataset.contained_source_directories:
+            try:
+                source_directories.remove(source_directory)
+            except ValueError:
+                pass
+
+            source_directories.append(
+                os.path.join(
+                    normalized_runtime_directory,
+                    'zfs_snapshots',
+                    '.',  # Borg 1.4+ "slashdot" hack.
+                    source_directory.lstrip(os.path.sep),
+                )
             )
 
-            if mount_point in source_directories:
-                source_directories.remove(mount_point)
-
-            source_directories.append(snapshot_mount_path_for_borg)
-
     return []
 
 
@@ -245,7 +281,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     zfs_command = hook_config.get('zfs_command', 'zfs')
 
     try:
-        datasets = get_all_datasets(zfs_command)
+        dataset_mount_points = get_all_dataset_mount_points(zfs_command)
     except FileNotFoundError:
         logger.debug(f'{log_prefix}: Could not find "{zfs_command}" command')
         return
@@ -275,7 +311,9 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         if not dry_run:
             shutil.rmtree(snapshots_directory, ignore_errors=True)
 
-        for _, mount_point in datasets:
+        # 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):
             snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep))
             if not os.path.isdir(snapshot_mount_path):
                 continue
diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md
index c572b067..6f7680e5 100644
--- a/docs/how-to/snapshot-your-filesystems.md
+++ b/docs/how-to/snapshot-your-filesystems.md
@@ -76,6 +76,14 @@ in an archive at `/var/dataset` as well—even if borgmatic has to mount the
 snapshot somewhere in `/run/user/1000/borgmatic/zfs_snapshots/` to perform the
 backup.
 
+<span class="minilink minilink-addedin">New in version 1.9.4</span> borgmatic
+is smart enough to look at the parent (and grandparent, etc.) directories of
+each of your `source_directories` to discover any datasets. For instance,
+let's say you add `/var/log` and `/var/lib` to your source directories, but
+`/var` is a dataset. borgmatic will discover that and snapshot `/var`
+accordingly. This also works even with nested datasets; borgmatic selects
+the dataset that's the "closest" parent to your source directories.
+
 <span class="minilink minilink-addedin">With Borg version 1.2 and
 earlier</span>Snapshotted files are instead stored at a path dependent on the
 [runtime

From 8c4b899a13dcc4941d0b32f55420437417a8b05f Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 11:12:27 -0800
Subject: [PATCH 12/33] Use a namedtuple for logical volume metadata (#80).

---
 borgmatic/hooks/data_source/lvm.py | 35 +++++++++++++++---------------
 borgmatic/hooks/data_source/zfs.py | 19 +++++++++++-----
 2 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index ffdf9e14..110c2e9d 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -1,3 +1,4 @@
+import collections
 import glob
 import json
 import logging
@@ -20,6 +21,9 @@ def use_streaming(hook_config, config, log_prefix):  # pragma: no cover
 
 
 BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
+Logical_volume = collections.namedtuple(
+    'Logical_volume', ('name', 'device_path', 'mount_point', 'contained_source_directories')
+)
 
 
 def get_logical_volumes(lsblk_command, source_directories=None):
@@ -31,8 +35,7 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     If source directories is None, include all logical volume mounts points, not just those in
     source directories.
 
-    Return the result as a sequence of (device name, device path, mount point, sequence of contained
-    source directories) tuples.
+    Return the result as a sequence of Logical_volume instances.
     '''
     try:
         devices_info = json.loads(
@@ -50,12 +53,13 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     except json.JSONDecodeError as error:
         raise ValueError('Invalid {lsblk_command} JSON output: {error}')
 
-
     candidate_source_directories = set(source_directories or ())
 
     try:
         return tuple(
-            (device['name'], device['path'], device['mountpoint'], contained_source_directories)
+            Logical_volume(
+                device['name'], device['path'], device['mountpoint'], contained_source_directories
+            )
             for device in devices_info['blockdevices']
             if device['mountpoint'] and device['type'] == 'lvm'
             for contained_source_directories in (
@@ -151,22 +155,17 @@ def dump_data_sources(
     if not requested_logical_volumes:
         logger.warning(f'{log_prefix}: No LVM logical volumes found to snapshot{dry_run_label}')
 
-    for (
-        device_name,
-        device_path,
-        mount_point,
-        contained_source_directories,
-    ) in requested_logical_volumes:
-        snapshot_name = f'{device_name}_{snapshot_suffix}'
+    for logical_volume in requested_logical_volumes:
+        snapshot_name = f'{logical_volume.name}_{snapshot_suffix}'
         logger.debug(
-            f'{log_prefix}: Creating LVM snapshot {snapshot_name} of {mount_point}{dry_run_label}'
+            f'{log_prefix}: Creating LVM snapshot {snapshot_name} of {logical_volume.mount_point}{dry_run_label}'
         )
 
         if not dry_run:
             snapshot_logical_volume(
                 hook_config.get('lvcreate_command', 'lvcreate'),
                 snapshot_name,
-                device_path,
+                logical_volume.device_path,
                 hook_config.get('snapshot_size', DEFAULT_SNAPSHOT_SIZE),
             )
 
@@ -183,7 +182,7 @@ def dump_data_sources(
         snapshot_mount_path = os.path.join(
             normalized_runtime_directory,
             'lvm_snapshots',
-            mount_point.lstrip(os.path.sep),
+            logical_volume.mount_point.lstrip(os.path.sep),
         )
 
         logger.debug(
@@ -199,7 +198,7 @@ def dump_data_sources(
 
         # Update the path for each contained source directory, so Borg sees it within the
         # mounted snapshot.
-        for source_directory in contained_source_directories:
+        for source_directory in logical_volume.contained_source_directories:
             try:
                 source_directories.remove(source_directory)
             except ValueError:
@@ -319,8 +318,10 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         if not dry_run:
             shutil.rmtree(snapshots_directory, ignore_errors=True)
 
-        for _, _, mount_point, _ in logical_volumes:
-            snapshot_mount_path = os.path.join(snapshots_directory, mount_point.lstrip(os.path.sep))
+        for logical_volume in logical_volumes:
+            snapshot_mount_path = os.path.join(
+                snapshots_directory, logical_volume.mount_point.lstrip(os.path.sep)
+            )
             if not os.path.isdir(snapshot_mount_path):
                 continue
 
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index a56489e0..225ca91f 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -23,7 +23,9 @@ BORGMATIC_SNAPSHOT_PREFIX = 'borgmatic-'
 BORGMATIC_USER_PROPERTY = 'org.torsion.borgmatic:backup'
 
 
-Dataset = collections.namedtuple('Dataset', ('name', 'mount_point', 'user_property_value', 'contained_source_directories'))
+Dataset = collections.namedtuple(
+    'Dataset', ('name', 'mount_point', 'user_property_value', 'contained_source_directories')
+)
 
 
 def get_datasets_to_backup(zfs_command, source_directories):
@@ -69,7 +71,12 @@ def get_datasets_to_backup(zfs_command, source_directories):
 
     return sorted(
         tuple(
-            Dataset(dataset.name, dataset.mount_point, dataset.user_property_value, contained_source_directories)
+            Dataset(
+                dataset.name,
+                dataset.mount_point,
+                dataset.user_property_value,
+                contained_source_directories,
+            )
             for dataset in datasets
             for contained_source_directories in (
                 borgmatic.hooks.data_source.snapshot.get_contained_directories(
@@ -99,9 +106,7 @@ def get_all_dataset_mount_points(zfs_command):
     )
 
     try:
-        return tuple(
-            sorted(line.rstrip() for line in list_output.splitlines())
-        )
+        return tuple(sorted(line.rstrip() for line in list_output.splitlines()))
     except ValueError:
         raise ValueError('Invalid {zfs_command} list output')
 
@@ -178,7 +183,9 @@ def dump_data_sources(
 
     for dataset in requested_datasets:
         full_snapshot_name = f'{dataset.name}@{snapshot_name}'
-        logger.debug(f'{log_prefix}: Creating ZFS snapshot {full_snapshot_name} of {dataset.mount_point}{dry_run_label}')
+        logger.debug(
+            f'{log_prefix}: Creating ZFS snapshot {full_snapshot_name} of {dataset.mount_point}{dry_run_label}'
+        )
 
         if not dry_run:
             snapshot_dataset(zfs_command, full_snapshot_name)

From 9b9ecad299cfe967bdc1d946e31d9fad2061a3b5 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 12:15:34 -0800
Subject: [PATCH 13/33] Port the parent directory discovery logic from LVM to
 Btrfs (#80).

---
 borgmatic/hooks/data_source/btrfs.py     | 100 +++++++++++++++--------
 docs/how-to/snapshot-your-filesystems.md |   8 ++
 2 files changed, 75 insertions(+), 33 deletions(-)

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 3d812356..927e81cb 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -1,3 +1,4 @@
+import collections
 import glob
 import logging
 import os
@@ -5,6 +6,7 @@ import shutil
 import subprocess
 
 import borgmatic.config.paths
+import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
 
 logger = logging.getLogger(__name__)
@@ -34,8 +36,8 @@ def get_filesystem_mount_points(findmnt_command):
 
 def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
     '''
-    Given a Btrfs command to run and a Btrfs filesystem mount point, get the subvolumes for that
-    filesystem.
+    Given a Btrfs command to run and a Btrfs filesystem mount point, get the sorted subvolumes for
+    that filesystem. Include the filesystem itself.
     '''
     btrfs_output = borgmatic.execute.execute_command_and_capture_output(
         (
@@ -46,43 +48,54 @@ def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
         )
     )
 
-    return tuple(
-        subvolume_path
-        for line in btrfs_output.splitlines()
-        for subvolume_subpath in (line.rstrip().split(' ')[-1],)
-        for subvolume_path in (os.path.join(filesystem_mount_point, subvolume_subpath),)
-        if subvolume_subpath.strip()
-        if filesystem_mount_point.strip()
+    return (filesystem_mount_point,) + tuple(
+        sorted(
+            subvolume_path
+            for line in btrfs_output.splitlines()
+            for subvolume_subpath in (line.rstrip().split(' ')[-1],)
+            for subvolume_path in (os.path.join(filesystem_mount_point, subvolume_subpath),)
+            if subvolume_subpath.strip()
+            if filesystem_mount_point.strip()
+        )
     )
 
 
+Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_source_directories'))
+
+
 def get_subvolumes(btrfs_command, findmnt_command, source_directories=None):
     '''
     Given a Btrfs command to run and a sequence of configured source directories, find the
     intersection between the current Btrfs filesystem and subvolume mount points and the configured
     borgmatic source directories. The idea is that these are the requested subvolumes to snapshot.
 
-    If the source directories is None, then return all subvolumes.
+    If the source directories is None, then return all subvolumes, sorted by path.
 
     Return the result as a sequence of matching subvolume mount points.
     '''
-    source_directories_lookup = set(source_directories or ())
+    candidate_source_directories = set(source_directories or ())
     subvolumes = []
 
     # For each filesystem mount point, find its subvolumes and match them again the given source
-    # directories to find the subvolumes to backup. Also try to match the filesystem mount point
-    # itself (since it's implicitly a subvolume).
+    # directories to find the subvolumes to backup. And within this loop, sort the subvolumes from
+    # longest to shortest mount points, so longer mount points get a whack at the candidate source
+    # directory piñata before their parents do. (Source directories are consumed during this
+    # process, so no two datasets get the same contained source directories.)
     for mount_point in get_filesystem_mount_points(findmnt_command):
-        if source_directories is None or mount_point in source_directories_lookup:
-            subvolumes.append(mount_point)
-
         subvolumes.extend(
-            subvolume_path
-            for subvolume_path in get_subvolumes_for_filesystem(btrfs_command, mount_point)
-            if source_directories is None or subvolume_path in source_directories_lookup
+            Subvolume(subvolume_path, contained_source_directories)
+            for subvolume_path in reversed(
+                get_subvolumes_for_filesystem(btrfs_command, mount_point)
+            )
+            for contained_source_directories in (
+                borgmatic.hooks.data_source.snapshot.get_contained_directories(
+                    subvolume_path, candidate_source_directories
+                ),
+            )
+            if source_directories is None or contained_source_directories
         )
 
-    return tuple(subvolumes)
+    return tuple(sorted(subvolumes, key=lambda subvolume: subvolume.path))
 
 
 BORGMATIC_SNAPSHOT_PREFIX = '.borgmatic-snapshot-'
@@ -95,7 +108,6 @@ def make_snapshot_path(subvolume_path):  # pragma: no cover
     return os.path.join(
         subvolume_path,
         f'{BORGMATIC_SNAPSHOT_PREFIX}{os.getpid()}',
-        '.',  # Borg 1.4+ "slashdot" hack.
         # Included so that the snapshot ends up in the Borg archive at the "original" subvolume
         # path.
         subvolume_path.lstrip(os.path.sep),
@@ -129,6 +141,20 @@ def make_snapshot_exclude_path(subvolume_path):  # pragma: no cover
     )
 
 
+def make_borg_source_directory_path(subvolume_path, source_directory):
+    '''
+    Given the path to a subvolume and a source directory inside it, make a corresponding path for
+    the source directory within a snapshot path intended for giving to Borg.
+    '''
+    return os.path.join(
+        subvolume_path,
+        f'{BORGMATIC_SNAPSHOT_PREFIX}{os.getpid()}',
+        '.',  # Borg 1.4+ "slashdot" hack.
+        # Included so that the source directory ends up in the Borg archive at its "original" path.
+        source_directory.lstrip(os.path.sep),
+    )
+
+
 def snapshot_subvolume(btrfs_command, subvolume_path, snapshot_path):  # pragma: no cover
     '''
     Given a Btrfs command to run, the path to a subvolume, and the path for a snapshot, create a new
@@ -182,21 +208,27 @@ def dump_data_sources(
         logger.warning(f'{log_prefix}: No Btrfs subvolumes found to snapshot{dry_run_label}')
 
     # Snapshot each subvolume, rewriting source directories to use their snapshot paths.
-    for subvolume_path in subvolumes:
-        logger.debug(f'{log_prefix}: Creating Btrfs snapshot for {subvolume_path} subvolume')
+    for subvolume in subvolumes:
+        logger.debug(f'{log_prefix}: Creating Btrfs snapshot for {subvolume.path} subvolume')
 
-        snapshot_path = make_snapshot_path(subvolume_path)
+        snapshot_path = make_snapshot_path(subvolume.path)
 
         if dry_run:
             continue
 
-        snapshot_subvolume(btrfs_command, subvolume_path, snapshot_path)
+        snapshot_subvolume(btrfs_command, subvolume.path, snapshot_path)
 
-        if subvolume_path in source_directories:
-            source_directories.remove(subvolume_path)
+        for source_directory in subvolume.contained_source_directories:
+            try:
+                source_directories.remove(source_directory)
+            except ValueError:
+                pass
 
-        source_directories.append(snapshot_path)
-        config.setdefault('exclude_patterns', []).append(make_snapshot_exclude_path(subvolume_path))
+            source_directories.append(
+                make_borg_source_directory_path(subvolume.path, source_directory)
+            )
+
+        config.setdefault('exclude_patterns', []).append(make_snapshot_exclude_path(subvolume.path))
 
     return []
 
@@ -228,7 +260,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     findmnt_command = hook_config.get('findmnt_command', 'findmnt')
 
     try:
-        all_subvolume_paths = get_subvolumes(btrfs_command, findmnt_command)
+        all_subvolumes = get_subvolumes(btrfs_command, findmnt_command)
     except FileNotFoundError as error:
         logger.debug(f'{log_prefix}: Could not find "{error.filename}" command')
         return
@@ -236,9 +268,11 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         logger.debug(f'{log_prefix}: {error}')
         return
 
-    for subvolume_path in all_subvolume_paths:
+    # Reversing the sorted subvolumes ensures that we remove longer mount point paths of child
+    # subvolumes before the shorter mount point paths of parent subvolumes.
+    for subvolume in reversed(all_subvolumes):
         subvolume_snapshots_glob = borgmatic.config.paths.replace_temporary_subdirectory_with_glob(
-            os.path.normpath(make_snapshot_path(subvolume_path)),
+            os.path.normpath(make_snapshot_path(subvolume.path)),
             temporary_directory_prefix=BORGMATIC_SNAPSHOT_PREFIX,
         )
 
@@ -266,7 +300,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
 
             # Strip off the subvolume path from the end of the snapshot path and then delete the
             # resulting directory.
-            shutil.rmtree(snapshot_path.rsplit(subvolume_path, 1)[0])
+            shutil.rmtree(snapshot_path.rsplit(subvolume.path, 1)[0])
 
 
 def make_data_source_dump_patterns(
diff --git a/docs/how-to/snapshot-your-filesystems.md b/docs/how-to/snapshot-your-filesystems.md
index 6f7680e5..c1c0e0c3 100644
--- a/docs/how-to/snapshot-your-filesystems.md
+++ b/docs/how-to/snapshot-your-filesystems.md
@@ -138,6 +138,14 @@ subvolumes (non-recursively) and includes the snapshotted files in the paths
 sent to Borg. borgmatic is also responsible for cleaning up (deleting) these
 snapshots after a backup completes.
 
+borgmatic is smart enough to look at the parent (and grandparent, etc.)
+directories of each of your `source_directories` to discover any subvolumes.
+For instance, let's say you add `/var/log` and `/var/lib` to your source
+directories, but `/var` is a subvolume. borgmatic will discover that and
+snapshot `/var` accordingly. This also works even with nested subvolumes;
+borgmatic selects the subvolume that's the "closest" parent to your source
+directories.
+
 Additionally, borgmatic rewrites the snapshot file paths so that they appear
 at their original subvolume locations in a Borg archive. For instance, if your
 subvolume exists at `/var/subvolume`, then the snapshotted files will appear

From 399bb6ef6899a1af8b7d34d4d82abbf3e8b6ad6e Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 12:22:43 -0800
Subject: [PATCH 14/33] Add recent LVM and ZFS work to NEWS (#80).

---
 NEWS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/NEWS b/NEWS
index e8f686ff..2353a55e 100644
--- a/NEWS
+++ b/NEWS
@@ -1,8 +1,13 @@
 1.9.4.dev0
+ * #80 (beta): Add an LVM hook for snapshotting and backing up LVM logical volumes. See the
+   documentation for more information:
+   https://torsion.org/borgmatic/docs/how-to/snapshot-your-filesystems/
  * #251 (beta): Add a Btrfs hook for snapshotting and backing up Btrfs subvolumes. See the
    documentation for more information:
    https://torsion.org/borgmatic/docs/how-to/snapshot-your-filesystems/
  * #926: Fix library error when running within a PyInstaller bundle.
+ * Update the ZFS hook to discover and snapshot ZFS datasets even if they are parent/grandparent
+   directories of your source directories.
  * Reorganize data source and monitoring hooks to make developing new hooks easier.
 
 1.9.3

From 347a4c3dd5ac85adec649465071d2e048d64ba96 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 15:43:50 -0800
Subject: [PATCH 15/33] Fix breakage of ZFS user property auto-backup (#80).

---
 borgmatic/hooks/data_source/zfs.py | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 225ca91f..552aadbe 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -24,7 +24,7 @@ BORGMATIC_USER_PROPERTY = 'org.torsion.borgmatic:backup'
 
 
 Dataset = collections.namedtuple(
-    'Dataset', ('name', 'mount_point', 'user_property_value', 'contained_source_directories')
+    'Dataset', ('name', 'mount_point', 'auto_backup', 'contained_source_directories')
 )
 
 
@@ -57,7 +57,7 @@ def get_datasets_to_backup(zfs_command, source_directories):
         # directories.)
         datasets = sorted(
             (
-                Dataset(dataset_name, mount_point, user_property_value, ())
+                Dataset(dataset_name, mount_point, (user_property_value == 'auto'), ())
                 for line in list_output.splitlines()
                 for (dataset_name, mount_point, user_property_value) in (line.rstrip().split('\t'),)
             ),
@@ -74,16 +74,19 @@ def get_datasets_to_backup(zfs_command, source_directories):
             Dataset(
                 dataset.name,
                 dataset.mount_point,
-                dataset.user_property_value,
+                dataset.auto_backup,
                 contained_source_directories,
             )
             for dataset in datasets
             for contained_source_directories in (
-                borgmatic.hooks.data_source.snapshot.get_contained_directories(
-                    dataset.mount_point, candidate_source_directories
+                (
+                    ((dataset.mount_point,) if dataset.auto_backup else ())
+                    + borgmatic.hooks.data_source.snapshot.get_contained_directories(
+                        dataset.mount_point, candidate_source_directories
+                    )
                 ),
             )
-            if contained_source_directories or dataset.user_property_value == 'auto'
+            if contained_source_directories
         ),
         key=lambda dataset: dataset.mount_point,
     )

From 87f37468815d5cc82d14802974f897baf9e76223 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 15:56:03 -0800
Subject: [PATCH 16/33] Fix a ZFS edge case in which the hook tries to
 unmounted a non-mounted directory (#80).

---
 borgmatic/hooks/data_source/zfs.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 552aadbe..6b688421 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -314,13 +314,6 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         if not os.path.isdir(snapshots_directory):
             continue
 
-        # This might fail if the directory is already mounted, but we swallow errors here since
-        # we'll try again below. The point of doing it here is that we don't want to try to unmount
-        # a non-mounted directory (which *will* fail), and probing for whether a directory is
-        # mounted is tough to do in a cross-platform way.
-        if not dry_run:
-            shutil.rmtree(snapshots_directory, ignore_errors=True)
-
         # 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):
@@ -328,6 +321,13 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not os.path.isdir(snapshot_mount_path):
                 continue
 
+            # This might fail if the path is already mounted, but we swallow errors here since we'll
+            # do another recursive delete below. The point of doing it here is that we don't want to
+            # try to unmount a non-mounted directory (which *will* fail), and probing for whether a
+            # directory is mounted is tough to do in a cross-platform way.
+            if not dry_run:
+                shutil.rmtree(snapshot_mount_path, ignore_errors=True)
+
             logger.debug(
                 f'{log_prefix}: Unmounting ZFS snapshot at {snapshot_mount_path}{dry_run_label}'
             )

From cc11ed78e02d636a830db658556beb608e000454 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 19:12:41 -0800
Subject: [PATCH 17/33] Put LVM snapshots into a data structure for convenience
 (#80).

---
 borgmatic/hooks/data_source/lvm.py      | 22 +++++++++++++---------
 borgmatic/hooks/data_source/snapshot.py |  4 +++-
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 110c2e9d..0319f3a2 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -171,7 +171,7 @@ def dump_data_sources(
 
         # Get the device path for the device path for the snapshot we just created.
         try:
-            (_, snapshot_device_path) = get_snapshots(
+            snapshot = get_snapshots(
                 hook_config.get('lvs_command', 'lvs'), snapshot_name=snapshot_name
             )[0]
         except IndexError:
@@ -193,7 +193,7 @@ def dump_data_sources(
             continue
 
         mount_snapshot(
-            hook_config.get('mount_command', 'mount'), snapshot_device_path, snapshot_mount_path
+            hook_config.get('mount_command', 'mount'), snapshot.device_path, snapshot_mount_path
         )
 
         # Update the path for each contained source directory, so Borg sees it within the
@@ -243,10 +243,14 @@ def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
     )
 
 
+Snapshot = collections.namedtuple(
+    'Snapshot', ('name', 'device_path'),
+)
+
+
 def get_snapshots(lvs_command, snapshot_name=None):
     '''
-    Given an lvs command to run, return all LVM snapshots as a sequence of (snapshot name, snapshot
-    device path) pairs.
+    Given an lvs command to run, return all LVM snapshots as a sequence of Snapshot instances.
 
     If a snapshot name is given, filter the results to that snapshot.
     '''
@@ -270,7 +274,7 @@ def get_snapshots(lvs_command, snapshot_name=None):
 
     try:
         return tuple(
-            (snapshot['lv_name'], snapshot['lv_path'])
+            Snapshot(snapshot['lv_name'], snapshot['lv_path'])
             for snapshot in snapshot_info['report'][0]['lv']
             if snapshot_name is None or snapshot['lv_name'] == snapshot_name
         )
@@ -345,15 +349,15 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     # Delete snapshots.
     lvremove_command = hook_config.get('lvremove_command', 'lvremove')
 
-    for snapshot_name, snapshot_device_path in get_snapshots(hook_config.get('lvs_command', 'lvs')):
+    for snapshot in get_snapshots(hook_config.get('lvs_command', 'lvs')):
         # Only delete snapshots that borgmatic actually created!
-        if not snapshot_name.split('_')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX):
+        if not snapshot.name.split('_')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX):
             continue
 
-        logger.debug(f'{log_prefix}: Deleting LVM snapshot {snapshot_name}{dry_run_label}')
+        logger.debug(f'{log_prefix}: Deleting LVM snapshot {snapshot.name}{dry_run_label}')
 
         if not dry_run:
-            delete_snapshot(lvremove_command, snapshot_device_path)
+            delete_snapshot(lvremove_command, snapshot.device_path)
 
 
 def make_data_source_dump_patterns(
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index b03b01d9..f5ef4575 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -14,7 +14,9 @@ def get_contained_directories(parent_directory, candidate_contained_directories)
     then /var is what we want to snapshot.
 
     Also mutate the given set of candidate contained directories to remove any actually contained
-    directories from it.
+    directories from it. That way, this function can be called multiple times, successively
+    processing candidate directories until none are left—and avoiding assigning any candidate
+    directory to more than one parent directory.
     '''
     if not candidate_contained_directories:
         return ()

From 9afdaca98512ceed02bea05a7b1987c9246f53d9 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Tue, 3 Dec 2024 19:19:22 -0800
Subject: [PATCH 18/33] Before unmounting, remove the snapshot mount path
 instead of the parent snapshot directory (#80).

---
 borgmatic/hooks/data_source/lvm.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 0319f3a2..ce9aa5a0 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -169,7 +169,7 @@ def dump_data_sources(
                 hook_config.get('snapshot_size', DEFAULT_SNAPSHOT_SIZE),
             )
 
-        # Get the device path for the device path for the snapshot we just created.
+        # Get the device path for the snapshot we just created.
         try:
             snapshot = get_snapshots(
                 hook_config.get('lvs_command', 'lvs'), snapshot_name=snapshot_name
@@ -316,12 +316,6 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         if not os.path.isdir(snapshots_directory):
             continue
 
-        # This might fail if the directory is already mounted, but we swallow errors here since
-        # we'll try again below. The point of doing it here is that we don't want to try to unmount
-        # a non-mounted directory (which *will* fail).
-        if not dry_run:
-            shutil.rmtree(snapshots_directory, ignore_errors=True)
-
         for logical_volume in logical_volumes:
             snapshot_mount_path = os.path.join(
                 snapshots_directory, logical_volume.mount_point.lstrip(os.path.sep)
@@ -329,6 +323,12 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not os.path.isdir(snapshot_mount_path):
                 continue
 
+            # This might fail if the directory is already mounted, but we swallow errors here since
+            # we'll do another recursive delete below. The point of doing it here is that we don't
+            # want to try to unmount a non-mounted directory (which *will* fail).
+            if not dry_run:
+                shutil.rmtree(snapshot_mount_path, ignore_errors=True)
+
             logger.debug(
                 f'{log_prefix}: Unmounting LVM snapshot at {snapshot_mount_path}{dry_run_label}'
             )

From d0e92493f6a4efd9fa8deeb2196469c5d7d9813b Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Wed, 4 Dec 2024 14:48:13 -0800
Subject: [PATCH 19/33] Fix broken ZFS tests (#80).

---
 borgmatic/hooks/data_source/lvm.py       |  3 +-
 borgmatic/hooks/data_source/zfs.py       | 52 +++++++------
 tests/unit/hooks/data_source/test_zfs.py | 96 +++++++++++++++---------
 3 files changed, 91 insertions(+), 60 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index ce9aa5a0..a88fe053 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -244,7 +244,8 @@ def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
 
 
 Snapshot = collections.namedtuple(
-    'Snapshot', ('name', 'device_path'),
+    'Snapshot',
+    ('name', 'device_path'),
 )
 
 
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 6b688421..c472484d 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -24,7 +24,9 @@ BORGMATIC_USER_PROPERTY = 'org.torsion.borgmatic:backup'
 
 
 Dataset = collections.namedtuple(
-    'Dataset', ('name', 'mount_point', 'auto_backup', 'contained_source_directories')
+    'Dataset',
+    ('name', 'mount_point', 'auto_backup', 'contained_source_directories'),
+    defaults=(False, ()),
 )
 
 
@@ -69,26 +71,29 @@ def get_datasets_to_backup(zfs_command, source_directories):
 
     candidate_source_directories = set(source_directories)
 
-    return sorted(
-        tuple(
-            Dataset(
-                dataset.name,
-                dataset.mount_point,
-                dataset.auto_backup,
-                contained_source_directories,
-            )
-            for dataset in datasets
-            for contained_source_directories in (
-                (
-                    ((dataset.mount_point,) if dataset.auto_backup else ())
-                    + borgmatic.hooks.data_source.snapshot.get_contained_directories(
-                        dataset.mount_point, candidate_source_directories
-                    )
-                ),
-            )
-            if contained_source_directories
-        ),
-        key=lambda dataset: dataset.mount_point,
+    return tuple(
+        sorted(
+            (
+                Dataset(
+                    dataset.name,
+                    dataset.mount_point,
+                    dataset.auto_backup,
+                    contained_source_directories,
+                )
+                for dataset in datasets
+                for contained_source_directories in (
+                    (
+                        (dataset.mount_point,)
+                        if dataset.auto_backup
+                        else borgmatic.hooks.data_source.snapshot.get_contained_directories(
+                            dataset.mount_point, candidate_source_directories
+                        )
+                    ),
+                )
+                if contained_source_directories
+            ),
+            key=lambda dataset: dataset.mount_point,
+        )
     )
 
 
@@ -108,10 +113,7 @@ def get_all_dataset_mount_points(zfs_command):
         )
     )
 
-    try:
-        return tuple(sorted(line.rstrip() for line in list_output.splitlines()))
-    except ValueError:
-        raise ValueError('Invalid {zfs_command} list output')
+    return tuple(sorted(line.rstrip() for line in list_output.splitlines()))
 
 
 def snapshot_dataset(zfs_command, full_snapshot_name):  # pragma: no cover
diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py
index 2eea24f7..954074fe 100644
--- a/tests/unit/hooks/data_source/test_zfs.py
+++ b/tests/unit/hooks/data_source/test_zfs.py
@@ -1,3 +1,5 @@
+import os
+
 import pytest
 from flexmock import flexmock
 
@@ -10,10 +12,18 @@ def test_get_datasets_to_backup_filters_datasets_by_source_directories():
     ).and_return(
         'dataset\t/dataset\t-\nother\t/other\t-',
     )
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/dataset', object).and_return(('/dataset',))
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/other', object).and_return(())
 
     assert module.get_datasets_to_backup(
         'zfs', source_directories=('/foo', '/dataset', '/bar')
-    ) == (('dataset', '/dataset'),)
+    ) == (
+        module.Dataset(name='dataset', mount_point='/dataset', contained_source_directories=('/dataset',)),
+    )
 
 
 def test_get_datasets_to_backup_filters_datasets_by_user_property():
@@ -22,9 +32,20 @@ def test_get_datasets_to_backup_filters_datasets_by_user_property():
     ).and_return(
         'dataset\t/dataset\tauto\nother\t/other\t-',
     )
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/dataset', object).never()
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/other', object).and_return(())
 
     assert module.get_datasets_to_backup('zfs', source_directories=('/foo', '/bar')) == (
-        ('dataset', '/dataset'),
+        module.Dataset(
+            name='dataset',
+            mount_point='/dataset',
+            auto_backup=True,
+            contained_source_directories=('/dataset',),
+        ),
     )
 
 
@@ -34,38 +55,39 @@ def test_get_datasets_to_backup_with_invalid_list_output_raises():
     ).and_return(
         'dataset',
     )
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).never()
 
     with pytest.raises(ValueError, match='zfs'):
         module.get_datasets_to_backup('zfs', source_directories=('/foo', '/bar'))
 
 
-def test_get_get_all_datasets_does_not_filter_datasets():
+def test_get_all_dataset_mount_points_does_not_filter_datasets():
     flexmock(module.borgmatic.execute).should_receive(
         'execute_command_and_capture_output'
     ).and_return(
-        'dataset\t/dataset\nother\t/other',
+        '/dataset\n/other',
     )
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).and_return(('/dataset',))
 
-    assert module.get_all_datasets('zfs') == (
-        ('dataset', '/dataset'),
-        ('other', '/other'),
+    assert module.get_all_dataset_mount_points('zfs') == (
+        ('/dataset'),
+        ('/other'),
     )
 
 
-def test_get_all_datasets_with_invalid_list_output_raises():
-    flexmock(module.borgmatic.execute).should_receive(
-        'execute_command_and_capture_output'
-    ).and_return(
-        'dataset',
-    )
-
-    with pytest.raises(ValueError, match='zfs'):
-        module.get_all_datasets('zfs')
-
-
 def test_dump_data_sources_snapshots_and_mounts_and_updates_source_directories():
     flexmock(module).should_receive('get_datasets_to_backup').and_return(
-        (('dataset', '/mnt/dataset'),)
+        (
+            flexmock(
+                name='dataset',
+                mount_point='/mnt/dataset',
+                contained_source_directories=('/mnt/dataset/subdir',),
+            )
+        )
     )
     flexmock(module.os).should_receive('getpid').and_return(1234)
     full_snapshot_name = 'dataset@borgmatic-1234'
@@ -79,7 +101,7 @@ def test_dump_data_sources_snapshots_and_mounts_and_updates_source_directories()
         full_snapshot_name,
         module.os.path.normpath(snapshot_mount_path),
     ).once()
-    source_directories = ['/mnt/dataset']
+    source_directories = ['/mnt/dataset/subdir']
 
     assert (
         module.dump_data_sources(
@@ -94,7 +116,7 @@ def test_dump_data_sources_snapshots_and_mounts_and_updates_source_directories()
         == []
     )
 
-    assert source_directories == [snapshot_mount_path]
+    assert source_directories == [os.path.join(snapshot_mount_path, 'subdir')]
 
 
 def test_dump_data_sources_snapshots_with_no_datasets_skips_snapshots():
@@ -122,7 +144,13 @@ def test_dump_data_sources_snapshots_with_no_datasets_skips_snapshots():
 
 def test_dump_data_sources_uses_custom_commands():
     flexmock(module).should_receive('get_datasets_to_backup').and_return(
-        (('dataset', '/mnt/dataset'),)
+        (
+            flexmock(
+                name='dataset',
+                mount_point='/mnt/dataset',
+                contained_source_directories=('/mnt/dataset/subdir',),
+            )
+        )
     )
     flexmock(module.os).should_receive('getpid').and_return(1234)
     full_snapshot_name = 'dataset@borgmatic-1234'
@@ -136,7 +164,7 @@ def test_dump_data_sources_uses_custom_commands():
         full_snapshot_name,
         module.os.path.normpath(snapshot_mount_path),
     ).once()
-    source_directories = ['/mnt/dataset']
+    source_directories = ['/mnt/dataset/subdir']
     hook_config = {
         'zfs_command': '/usr/local/bin/zfs',
         'mount_command': '/usr/local/bin/mount',
@@ -158,12 +186,12 @@ def test_dump_data_sources_uses_custom_commands():
         == []
     )
 
-    assert source_directories == [snapshot_mount_path]
+    assert source_directories == [os.path.join(snapshot_mount_path, 'subdir')]
 
 
 def test_dump_data_sources_with_dry_run_skips_commands_and_does_not_touch_source_directories():
     flexmock(module).should_receive('get_datasets_to_backup').and_return(
-        (('dataset', '/mnt/dataset'),)
+        (flexmock(name='dataset', mount_point='/mnt/dataset'),)
     )
     flexmock(module.os).should_receive('getpid').and_return(1234)
     flexmock(module).should_receive('snapshot_dataset').never()
@@ -197,7 +225,7 @@ def test_get_all_snapshots_parses_list_output():
 
 
 def test_remove_data_source_dumps_unmounts_and_destroys_snapshots():
-    flexmock(module).should_receive('get_all_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -224,7 +252,7 @@ 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_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -252,7 +280,7 @@ def test_remove_data_source_dumps_use_custom_commands():
 
 
 def test_remove_data_source_dumps_bails_for_missing_zfs_command():
-    flexmock(module).should_receive('get_all_datasets').and_raise(FileNotFoundError)
+    flexmock(module).should_receive('get_all_dataset_mount_points').and_raise(FileNotFoundError)
     flexmock(module.borgmatic.config.paths).should_receive(
         'replace_temporary_subdirectory_with_glob'
     ).never()
@@ -268,7 +296,7 @@ def test_remove_data_source_dumps_bails_for_missing_zfs_command():
 
 
 def test_remove_data_source_dumps_bails_for_zfs_command_error():
-    flexmock(module).should_receive('get_all_datasets').and_raise(
+    flexmock(module).should_receive('get_all_dataset_mount_points').and_raise(
         module.subprocess.CalledProcessError(1, 'wtf')
     )
     flexmock(module.borgmatic.config.paths).should_receive(
@@ -286,7 +314,7 @@ 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_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -310,7 +338,7 @@ def test_remove_data_source_dumps_bails_for_missing_umount_command():
 
 
 def test_remove_data_source_dumps_bails_for_umount_command_error():
-    flexmock(module).should_receive('get_all_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -334,7 +362,7 @@ def test_remove_data_source_dumps_bails_for_umount_command_error():
 
 
 def test_remove_data_source_dumps_skips_unmount_snapshot_directories_that_are_not_actually_directories():
-    flexmock(module).should_receive('get_all_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -359,7 +387,7 @@ 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_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')
@@ -389,7 +417,7 @@ def test_remove_data_source_dumps_skips_unmount_snapshot_mount_paths_that_are_no
 
 
 def test_remove_data_source_dumps_with_dry_run_skips_unmount_and_destroy():
-    flexmock(module).should_receive('get_all_datasets').and_return((('dataset', '/mnt/dataset'),))
+    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')

From 49b8b693af925f1fc5d754df71d3bf45dc600801 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Wed, 4 Dec 2024 15:39:04 -0800
Subject: [PATCH 20/33] Don't try to unmount a ZFS snapshot if it's already
 deleted (#80).

---
 borgmatic/hooks/data_source/btrfs.py       |  2 +-
 borgmatic/hooks/data_source/lvm.py         |  4 ++++
 borgmatic/hooks/data_source/snapshot.py    |  4 ++--
 borgmatic/hooks/data_source/zfs.py         |  4 ++++
 tests/unit/hooks/data_source/test_btrfs.py | 20 +++++++++++++-------
 5 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 927e81cb..5d6aa134 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -60,7 +60,7 @@ def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
     )
 
 
-Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_source_directories'))
+Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_source_directories'), defaults=(()))
 
 
 def get_subvolumes(btrfs_command, findmnt_command, source_directories=None):
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index a88fe053..27a83485 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -330,6 +330,10 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not dry_run:
                 shutil.rmtree(snapshot_mount_path, ignore_errors=True)
 
+            # If the delete was successful, that means there's nothing to unmount.
+            if not os.path.isdir(snapshot_mount_path):
+                continue
+
             logger.debug(
                 f'{log_prefix}: Unmounting LVM snapshot at {snapshot_mount_path}{dry_run_label}'
             )
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index f5ef4575..9567d226 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -24,8 +24,8 @@ def get_contained_directories(parent_directory, candidate_contained_directories)
     contained = tuple(
         candidate
         for candidate in candidate_contained_directories
-        if parent_directory == candidate
-        or pathlib.PurePosixPath(parent_directory) in pathlib.PurePath(candidate).parents
+        if pathlib.PurePath(parent_directory) == pathlib.PurePath(candidate)
+        or pathlib.PurePath(parent_directory) in pathlib.PurePath(candidate).parents
     )
     candidate_contained_directories -= set(contained)
 
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index c472484d..794712ae 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -330,6 +330,10 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not dry_run:
                 shutil.rmtree(snapshot_mount_path, ignore_errors=True)
 
+            # If the delete was successful, that means there's nothing to unmount.
+            if not os.path.isdir(snapshot_mount_path):
+                continue
+
             logger.debug(
                 f'{log_prefix}: Unmounting ZFS snapshot at {snapshot_mount_path}{dry_run_label}'
             )
diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py
index f983a66d..4a9f3fb6 100644
--- a/tests/unit/hooks/data_source/test_btrfs.py
+++ b/tests/unit/hooks/data_source/test_btrfs.py
@@ -51,9 +51,12 @@ def test_get_subvolumes_collects_subvolumes_matching_source_directories_from_all
         'btrfs', '/mnt2'
     ).and_return(('/three', '/four'))
 
+    for path in ('/one', '/two', '/three', '/four'):
+        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive('get_contained_directories').with_args(path).and_return((path,))
+
     assert module.get_subvolumes(
         'btrfs', 'findmnt', source_directories=['/one', '/four', '/five', '/six', '/mnt2', '/mnt3']
-    ) == ('/one', '/mnt2', '/four')
+    ) == (module.Subvolume('/one'), module.Subvolume('/mnt2'), module.Subvolume('/four'))
 
 
 def test_get_subvolumes_without_source_directories_collects_all_subvolumes_from_all_filesystems():
@@ -65,13 +68,16 @@ def test_get_subvolumes_without_source_directories_collects_all_subvolumes_from_
         'btrfs', '/mnt2'
     ).and_return(('/three', '/four'))
 
+    for path in ('/one', '/two', '/three', '/four'):
+        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive('get_contained_directories').with_args(path).and_return((path,))
+
     assert module.get_subvolumes('btrfs', 'findmnt') == (
-        '/mnt1',
-        '/one',
-        '/two',
-        '/mnt2',
-        '/three',
-        '/four',
+        module.Subvolume('/mnt1'),
+        module.Subvolume('/one'),
+        module.Subvolume('/two'),
+        module.Subvolume('/mnt2'),
+        module.Subvolume('/three'),
+        module.Subvolume('/four'),
     )
 
 

From 51a7f50e3aa981c82ae9bf978e6753eb187bdd98 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Wed, 4 Dec 2024 15:43:05 -0800
Subject: [PATCH 21/33] Add ZFS snapshot unmount error fix to NEWS (#950).

---
 NEWS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 2353a55e..129dc91e 100644
--- a/NEWS
+++ b/NEWS
@@ -5,7 +5,8 @@
  * #251 (beta): Add a Btrfs hook for snapshotting and backing up Btrfs subvolumes. See the
    documentation for more information:
    https://torsion.org/borgmatic/docs/how-to/snapshot-your-filesystems/
- * #926: Fix library error when running within a PyInstaller bundle.
+ * #926: Fix a library error when running within a PyInstaller bundle.
+ * #950: Fix a snapshot unmount error in the ZFS hook when using nested datasets.
  * Update the ZFS hook to discover and snapshot ZFS datasets even if they are parent/grandparent
    directories of your source directories.
  * Reorganize data source and monitoring hooks to make developing new hooks easier.

From d95707ff9bb48af84a8dd1d2ecabce065c1b44b5 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Wed, 4 Dec 2024 20:22:59 -0800
Subject: [PATCH 22/33] Get existing tests passing (#80).

---
 borgmatic/hooks/data_source/btrfs.py       |  8 +-
 borgmatic/hooks/data_source/lvm.py         |  4 +-
 borgmatic/hooks/data_source/snapshot.py    |  1 +
 tests/unit/hooks/data_source/test_btrfs.py | 95 +++++++++++++++++-----
 tests/unit/hooks/data_source/test_zfs.py   |  4 +-
 5 files changed, 85 insertions(+), 27 deletions(-)

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 5d6aa134..5302c6cd 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -48,6 +48,9 @@ def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
         )
     )
 
+    if not filesystem_mount_point.strip():
+        return ()
+
     return (filesystem_mount_point,) + tuple(
         sorted(
             subvolume_path
@@ -55,12 +58,13 @@ def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
             for subvolume_subpath in (line.rstrip().split(' ')[-1],)
             for subvolume_path in (os.path.join(filesystem_mount_point, subvolume_subpath),)
             if subvolume_subpath.strip()
-            if filesystem_mount_point.strip()
         )
     )
 
 
-Subvolume = collections.namedtuple('Subvolume', ('path', 'contained_source_directories'), defaults=(()))
+Subvolume = collections.namedtuple(
+    'Subvolume', ('path', 'contained_source_directories'), defaults=((),)
+)
 
 
 def get_subvolumes(btrfs_command, findmnt_command, source_directories=None):
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 27a83485..717f60ec 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -295,8 +295,8 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     # Unmount snapshots.
     try:
         logical_volumes = get_logical_volumes(hook_config.get('lsblk_command', 'lsblk'))
-    except FileNotFoundError:
-        logger.debug(f'{log_prefix}: Could not find "{lsblk_command}" command')
+    except FileNotFoundError as error:
+        logger.debug(f'{log_prefix}: Could not find "{error.filename}" command')
         return
     except subprocess.CalledProcessError as error:
         logger.debug(f'{log_prefix}: {error}')
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index 9567d226..43e4be01 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -18,6 +18,7 @@ def get_contained_directories(parent_directory, candidate_contained_directories)
     processing candidate directories until none are left—and avoiding assigning any candidate
     directory to more than one parent directory.
     '''
+    1 / 0
     if not candidate_contained_directories:
         return ()
 
diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py
index 4a9f3fb6..b6a9834a 100644
--- a/tests/unit/hooks/data_source/test_btrfs.py
+++ b/tests/unit/hooks/data_source/test_btrfs.py
@@ -21,7 +21,7 @@ def test_get_subvolumes_for_filesystem_parses_subvolume_list_output():
         'ID 270 gen 107 top level 5 path subvol1\nID 272 gen 74 top level 5 path subvol2\n'
     )
 
-    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == ('/mnt/subvol1', '/mnt/subvol2')
+    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == ('/mnt', '/mnt/subvol1', '/mnt/subvol2')
 
 
 def test_get_subvolumes_for_filesystem_skips_empty_subvolume_paths():
@@ -29,7 +29,7 @@ def test_get_subvolumes_for_filesystem_skips_empty_subvolume_paths():
         'execute_command_and_capture_output'
     ).and_return('\n \nID 272 gen 74 top level 5 path subvol2\n')
 
-    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == ('/mnt/subvol2',)
+    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == ('/mnt', '/mnt/subvol2')
 
 
 def test_get_subvolumes_for_filesystem_skips_empty_filesystem_mount_points():
@@ -51,12 +51,21 @@ def test_get_subvolumes_collects_subvolumes_matching_source_directories_from_all
         'btrfs', '/mnt2'
     ).and_return(('/three', '/four'))
 
-    for path in ('/one', '/two', '/three', '/four'):
-        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive('get_contained_directories').with_args(path).and_return((path,))
+    for path in ('/one', '/four'):
+        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+            'get_contained_directories'
+        ).with_args(path, object).and_return((path,))
+    for path in ('/two', '/three'):
+        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+            'get_contained_directories'
+        ).with_args(path, object).and_return(())
 
     assert module.get_subvolumes(
         'btrfs', 'findmnt', source_directories=['/one', '/four', '/five', '/six', '/mnt2', '/mnt3']
-    ) == (module.Subvolume('/one'), module.Subvolume('/mnt2'), module.Subvolume('/four'))
+    ) == (
+        module.Subvolume('/four', contained_source_directories=('/four',)),
+        module.Subvolume('/one', contained_source_directories=('/one',)),
+    )
 
 
 def test_get_subvolumes_without_source_directories_collects_all_subvolumes_from_all_filesystems():
@@ -69,22 +78,27 @@ def test_get_subvolumes_without_source_directories_collects_all_subvolumes_from_
     ).and_return(('/three', '/four'))
 
     for path in ('/one', '/two', '/three', '/four'):
-        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive('get_contained_directories').with_args(path).and_return((path,))
+        flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+            'get_contained_directories'
+        ).with_args(path, object).and_return((path,))
 
     assert module.get_subvolumes('btrfs', 'findmnt') == (
-        module.Subvolume('/mnt1'),
-        module.Subvolume('/one'),
-        module.Subvolume('/two'),
-        module.Subvolume('/mnt2'),
-        module.Subvolume('/three'),
-        module.Subvolume('/four'),
+        module.Subvolume('/four', contained_source_directories=('/four',)),
+        module.Subvolume('/one', contained_source_directories=('/one',)),
+        module.Subvolume('/three', contained_source_directories=('/three',)),
+        module.Subvolume('/two', contained_source_directories=('/two',)),
     )
 
 
 def test_dump_data_sources_snapshots_each_subvolume_and_updates_source_directories():
     source_directories = ['/foo', '/mnt/subvol1']
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -103,6 +117,12 @@ def test_dump_data_sources_snapshots_each_subvolume_and_updates_source_directori
     flexmock(module).should_receive('make_snapshot_exclude_path').with_args(
         '/mnt/subvol2'
     ).and_return('/mnt/subvol2/.borgmatic-1234/mnt/subvol2/.borgmatic-1234')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol1', object
+    ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol2', object
+    ).and_return('/mnt/subvol2/.borgmatic-1234/mnt/subvol2')
 
     assert (
         module.dump_data_sources(
@@ -134,7 +154,9 @@ def test_dump_data_sources_snapshots_each_subvolume_and_updates_source_directori
 def test_dump_data_sources_uses_custom_btrfs_command_in_commands():
     source_directories = ['/foo', '/mnt/subvol1']
     config = {'btrfs': {'btrfs_command': '/usr/local/bin/btrfs'}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1',))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),)
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -144,6 +166,9 @@ def test_dump_data_sources_uses_custom_btrfs_command_in_commands():
     flexmock(module).should_receive('make_snapshot_exclude_path').with_args(
         '/mnt/subvol1'
     ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1/.borgmatic-1234')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol1', object
+    ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1')
 
     assert (
         module.dump_data_sources(
@@ -177,7 +202,7 @@ def test_dump_data_sources_uses_custom_findmnt_command_in_commands():
     config = {'btrfs': {'findmnt_command': '/usr/local/bin/findmnt'}}
     flexmock(module).should_receive('get_subvolumes').with_args(
         'btrfs', '/usr/local/bin/findmnt', source_directories
-    ).and_return(('/mnt/subvol1',)).once()
+    ).and_return((module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),)).once()
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -187,6 +212,9 @@ def test_dump_data_sources_uses_custom_findmnt_command_in_commands():
     flexmock(module).should_receive('make_snapshot_exclude_path').with_args(
         '/mnt/subvol1'
     ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1/.borgmatic-1234')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol1', object
+    ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1')
 
     assert (
         module.dump_data_sources(
@@ -218,7 +246,9 @@ def test_dump_data_sources_uses_custom_findmnt_command_in_commands():
 def test_dump_data_sources_with_dry_run_skips_snapshot_and_source_directories_update():
     source_directories = ['/foo', '/mnt/subvol1']
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1',))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),)
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -270,7 +300,9 @@ def test_dump_data_sources_without_matching_subvolumes_skips_snapshot_and_source
 def test_dump_data_sources_snapshots_adds_to_existing_exclude_patterns():
     source_directories = ['/foo', '/mnt/subvol1']
     config = {'btrfs': {}, 'exclude_patterns': ['/bar']}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -289,6 +321,12 @@ def test_dump_data_sources_snapshots_adds_to_existing_exclude_patterns():
     flexmock(module).should_receive('make_snapshot_exclude_path').with_args(
         '/mnt/subvol2'
     ).and_return('/mnt/subvol2/.borgmatic-1234/mnt/subvol2/.borgmatic-1234')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol1', object
+    ).and_return('/mnt/subvol1/.borgmatic-1234/mnt/subvol1')
+    flexmock(module).should_receive('make_borg_source_directory_path').with_args(
+        '/mnt/subvol2', object
+    ).and_return('/mnt/subvol2/.borgmatic-1234/mnt/subvol2')
 
     assert (
         module.dump_data_sources(
@@ -320,7 +358,9 @@ def test_dump_data_sources_snapshots_adds_to_existing_exclude_patterns():
 
 def test_remove_data_source_dumps_deletes_snapshots():
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
     )
@@ -441,7 +481,9 @@ def test_remove_data_source_dumps_with_get_subvolumes_called_process_error_bails
 
 def test_remove_data_source_dumps_with_dry_run_skips_deletes():
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
     )
@@ -519,7 +561,9 @@ def test_remove_data_source_dumps_without_subvolumes_skips_deletes():
 
 def test_remove_data_source_without_snapshots_skips_deletes():
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
     )
@@ -558,7 +602,9 @@ def test_remove_data_source_without_snapshots_skips_deletes():
 
 def test_remove_data_source_dumps_with_delete_snapshot_file_not_found_error_bails():
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
     )
@@ -617,7 +663,12 @@ def test_remove_data_source_dumps_with_delete_snapshot_file_not_found_error_bail
 
 def test_remove_data_source_dumps_with_delete_snapshot_called_process_error_bails():
     config = {'btrfs': {}}
-    flexmock(module).should_receive('get_subvolumes').and_return(('/mnt/subvol1', '/mnt/subvol2'))
+    flexmock(module).should_receive('get_subvolumes').and_return(
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
+    )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
     )
diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py
index 954074fe..7323cabd 100644
--- a/tests/unit/hooks/data_source/test_zfs.py
+++ b/tests/unit/hooks/data_source/test_zfs.py
@@ -22,7 +22,9 @@ def test_get_datasets_to_backup_filters_datasets_by_source_directories():
     assert module.get_datasets_to_backup(
         'zfs', source_directories=('/foo', '/dataset', '/bar')
     ) == (
-        module.Dataset(name='dataset', mount_point='/dataset', contained_source_directories=('/dataset',)),
+        module.Dataset(
+            name='dataset', mount_point='/dataset', contained_source_directories=('/dataset',)
+        ),
     )
 
 

From f8df06fb925c08326181554773cc434e1351100b Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Wed, 4 Dec 2024 20:33:59 -0800
Subject: [PATCH 23/33] Remove divison by zero (#80).

---
 borgmatic/hooks/data_source/snapshot.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index 43e4be01..9567d226 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -18,7 +18,6 @@ def get_contained_directories(parent_directory, candidate_contained_directories)
     processing candidate directories until none are left—and avoiding assigning any candidate
     directory to more than one parent directory.
     '''
-    1 / 0
     if not candidate_contained_directories:
         return ()
 

From f1c5f114228cd3a698e3a149d589fed1fc879503 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Thu, 5 Dec 2024 11:18:53 -0800
Subject: [PATCH 24/33] Add an end-to-end test for the ZFS hook using a fake
 ZFS binary (#80).

---
 borgmatic/hooks/data_source/btrfs.py     |  16 ++--
 borgmatic/hooks/data_source/lvm.py       |  26 +++---
 borgmatic/hooks/data_source/zfs.py       |  30 +++----
 tests/end-to-end/commands/__init__.py    |   0
 tests/end-to-end/commands/fake_mount.py  |  26 ++++++
 tests/end-to-end/commands/fake_umount.py |  22 +++++
 tests/end-to-end/commands/fake_zfs.py    | 103 +++++++++++++++++++++++
 tests/end-to-end/test_zfs.py             |  62 ++++++++++++++
 8 files changed, 249 insertions(+), 36 deletions(-)
 create mode 100644 tests/end-to-end/commands/__init__.py
 create mode 100755 tests/end-to-end/commands/fake_mount.py
 create mode 100755 tests/end-to-end/commands/fake_umount.py
 create mode 100755 tests/end-to-end/commands/fake_zfs.py
 create mode 100644 tests/end-to-end/test_zfs.py

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 5302c6cd..c99ee04b 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -24,8 +24,8 @@ def get_filesystem_mount_points(findmnt_command):
     Given a findmnt command to run, get all top-level Btrfs filesystem mount points.
     '''
     findmnt_output = borgmatic.execute.execute_command_and_capture_output(
-        (
-            findmnt_command,
+        tuple(findmnt_command.split(' '))
+        + (
             '-nt',
             'btrfs',
         )
@@ -40,8 +40,8 @@ def get_subvolumes_for_filesystem(btrfs_command, filesystem_mount_point):
     that filesystem. Include the filesystem itself.
     '''
     btrfs_output = borgmatic.execute.execute_command_and_capture_output(
-        (
-            btrfs_command,
+        tuple(btrfs_command.split(' '))
+        + (
             'subvolume',
             'list',
             filesystem_mount_point,
@@ -167,8 +167,8 @@ def snapshot_subvolume(btrfs_command, subvolume_path, snapshot_path):  # pragma:
     os.makedirs(os.path.dirname(snapshot_path), mode=0o700, exist_ok=True)
 
     borgmatic.execute.execute_command(
-        (
-            btrfs_command,
+        tuple(btrfs_command.split(' '))
+        + (
             'subvolume',
             'snapshot',
             '-r',  # Read-only,
@@ -242,8 +242,8 @@ def delete_snapshot(btrfs_command, snapshot_path):  # pragma: no cover
     Given a Btrfs command to run and the name of a snapshot path, delete it.
     '''
     borgmatic.execute.execute_command(
-        (
-            btrfs_command,
+        tuple(btrfs_command.split(' '))
+        + (
             'subvolume',
             'delete',
             snapshot_path,
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 717f60ec..48418f43 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -51,7 +51,7 @@ def get_logical_volumes(lsblk_command, source_directories=None):
             )
         )
     except json.JSONDecodeError as error:
-        raise ValueError('Invalid {lsblk_command} JSON output: {error}')
+        raise ValueError(f'Invalid {lsblk_command} JSON output: {error}')
 
     candidate_source_directories = set(source_directories or ())
 
@@ -84,8 +84,8 @@ def snapshot_logical_volume(
     snapshot, and a snapshot size string, create a new LVM snapshot.
     '''
     borgmatic.execute.execute_command(
-        (
-            lvcreate_command,
+        tuple(lvcreate_command.split(' '))
+        + (
             '--snapshot',
             ('--extents' if '%' in snapshot_size else '--size'),
             snapshot_size,
@@ -106,8 +106,8 @@ def mount_snapshot(mount_command, snapshot_device, snapshot_mount_path):  # prag
     os.makedirs(snapshot_mount_path, mode=0o700, exist_ok=True)
 
     borgmatic.execute.execute_command(
-        (
-            mount_command,
+        tuple(mount_command.split(' '))
+        + (
             '-o',
             'ro',
             snapshot_device,
@@ -221,8 +221,8 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
     Given a umount command to run and the mount path of a snapshot, unmount it.
     '''
     borgmatic.execute.execute_command(
-        (
-            umount_command,
+        tuple(umount_command.split(' '))
+        + (
             snapshot_mount_path,
         ),
         output_log_level=logging.DEBUG,
@@ -234,8 +234,8 @@ def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
     Given an lvremove command to run and the device path of a snapshot, remove it it.
     '''
     borgmatic.execute.execute_command(
-        (
-            lvremove_command,
+        tuple(lvremove_command.split(' '))
+        + (
             '--force',  # Suppress an interactive "are you sure?" type prompt.
             snapshot_device_path,
         ),
@@ -258,9 +258,9 @@ def get_snapshots(lvs_command, snapshot_name=None):
     try:
         snapshot_info = json.loads(
             borgmatic.execute.execute_command_and_capture_output(
-                (
-                    # Use lvs instead of lsblk here because lsblk can't filter to just snapshots.
-                    lvs_command,
+                # Use lvs instead of lsblk here because lsblk can't filter to just snapshots.
+                tuple(lvs_command.split(' '))
+                + (
                     '--report-format',
                     'json',
                     '--options',
@@ -271,7 +271,7 @@ def get_snapshots(lvs_command, snapshot_name=None):
             )
         )
     except json.JSONDecodeError as error:
-        raise ValueError('Invalid {lvs_command} JSON output: {error}')
+        raise ValueError(f'Invalid {lvs_command} JSON output: {error}')
 
     try:
         return tuple(
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 794712ae..d5dac5b0 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -41,8 +41,8 @@ def get_datasets_to_backup(zfs_command, source_directories):
     Return the result as a sequence of Dataset instances, sorted by mount point.
     '''
     list_output = borgmatic.execute.execute_command_and_capture_output(
-        (
-            zfs_command,
+        tuple(zfs_command.split(' '))
+        + (
             'list',
             '-H',
             '-t',
@@ -67,7 +67,7 @@ def get_datasets_to_backup(zfs_command, source_directories):
             reverse=True,
         )
     except ValueError:
-        raise ValueError('Invalid {zfs_command} list output')
+        raise ValueError(f'Invalid {zfs_command} list output')
 
     candidate_source_directories = set(source_directories)
 
@@ -102,8 +102,8 @@ 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.
     '''
     list_output = borgmatic.execute.execute_command_and_capture_output(
-        (
-            zfs_command,
+        tuple(zfs_command.split(' '))
+        + (
             'list',
             '-H',
             '-t',
@@ -122,8 +122,8 @@ def snapshot_dataset(zfs_command, full_snapshot_name):  # pragma: no cover
     snapshot.
     '''
     borgmatic.execute.execute_command(
-        (
-            zfs_command,
+        tuple(zfs_command.split(' '))
+        + (
             'snapshot',
             full_snapshot_name,
         ),
@@ -140,8 +140,8 @@ def mount_snapshot(mount_command, full_snapshot_name, snapshot_mount_path):  # p
     os.makedirs(snapshot_mount_path, mode=0o700, exist_ok=True)
 
     borgmatic.execute.execute_command(
-        (
-            mount_command,
+        tuple(mount_command.split(' '))
+        + (
             '-t',
             'zfs',
             full_snapshot_name,
@@ -237,8 +237,8 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
     Given a umount command to run and the mount path of a snapshot, unmount it.
     '''
     borgmatic.execute.execute_command(
-        (
-            umount_command,
+        tuple(umount_command.split(' '))
+        + (
             snapshot_mount_path,
         ),
         output_log_level=logging.DEBUG,
@@ -251,8 +251,8 @@ def destroy_snapshot(zfs_command, full_snapshot_name):  # pragma: no cover
     it.
     '''
     borgmatic.execute.execute_command(
-        (
-            zfs_command,
+        tuple(zfs_command.split(' '))
+        + (
             'destroy',
             full_snapshot_name,
         ),
@@ -266,8 +266,8 @@ def get_all_snapshots(zfs_command):
     form "dataset@snapshot".
     '''
     list_output = borgmatic.execute.execute_command_and_capture_output(
-        (
-            zfs_command,
+        tuple(zfs_command.split(' '))
+        + (
             'list',
             '-H',
             '-t',
diff --git a/tests/end-to-end/commands/__init__.py b/tests/end-to-end/commands/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/end-to-end/commands/fake_mount.py b/tests/end-to-end/commands/fake_mount.py
new file mode 100755
index 00000000..21e161c9
--- /dev/null
+++ b/tests/end-to-end/commands/fake_mount.py
@@ -0,0 +1,26 @@
+import argparse
+import os
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+    parser.add_argument('-t', dest='type')
+    parser.add_argument('snapshot_name')
+    parser.add_argument('mount_point')
+
+    return parser.parse_args(unparsed_arguments)
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+
+    subdirectory = os.path.join(arguments.mount_point, 'subdir')
+    os.mkdir(subdirectory)
+    test_file = open(os.path.join(subdirectory, 'file.txt'), 'w')
+    test_file.write('contents')
+    test_file.close()
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_umount.py b/tests/end-to-end/commands/fake_umount.py
new file mode 100755
index 00000000..e386c802
--- /dev/null
+++ b/tests/end-to-end/commands/fake_umount.py
@@ -0,0 +1,22 @@
+import argparse
+import os
+import shutil
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+    parser.add_argument('mount_point')
+
+    return parser.parse_args(unparsed_arguments)
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+
+    subdirectory = os.path.join(arguments.mount_point, 'subdir')
+    shutil.rmtree(subdirectory)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_zfs.py b/tests/end-to-end/commands/fake_zfs.py
new file mode 100755
index 00000000..88c4d1e3
--- /dev/null
+++ b/tests/end-to-end/commands/fake_zfs.py
@@ -0,0 +1,103 @@
+import argparse
+import json
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    global_parser = argparse.ArgumentParser(add_help=False)
+    action_parsers = global_parser.add_subparsers(dest='action')
+
+    list_parser = action_parsers.add_parser('list')
+    list_parser.add_argument('-H', dest='header', action='store_false', default=True)
+    list_parser.add_argument('-t', dest='type', default='filesystem')
+    list_parser.add_argument('-o', dest='properties', default='name,used,avail,refer,mountpoint')
+
+    snapshot_parser = action_parsers.add_parser('snapshot')
+    snapshot_parser.add_argument('name')
+
+    destroy_parser = action_parsers.add_parser('destroy')
+    destroy_parser.add_argument('name')
+
+    return global_parser.parse_args(unparsed_arguments)
+
+
+BUILTIN_DATASETS = (
+    {
+        'name': 'pool',
+        'used': '256K',
+        'avail': '23.7M',
+        'refer': '25K',
+        'mountpoint': '/pool',
+    },
+    {
+        'name': 'pool/dataset',
+        'used': '256K',
+        'avail': '23.7M',
+        'refer': '25K',
+        'mountpoint': '/pool/dataset',
+    },
+)
+
+
+
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_zfs.json'))
+    except FileNotFoundError:
+        return []
+
+
+def save_snapshots(snapshots):
+    json.dump(snapshots, open('/tmp/fake_zfs.json', 'w'))
+
+
+def print_dataset_list(arguments, datasets, snapshots):
+    properties = arguments.properties.split(',')
+    data = (
+        (
+            tuple(property_name.upper() for property_name in properties),
+        )
+        if arguments.header else ()
+    ) + tuple(
+        tuple(
+            dataset.get(property_name, '-') for property_name in properties
+        )
+        for dataset in (snapshots if arguments.type == 'snapshot' else datasets)
+    )
+
+    if not data:
+        return
+
+    for data_row in data:
+        print('\t'.join(data_row))
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+    snapshots = load_snapshots()
+
+    if arguments.action == 'list':
+        print_dataset_list(arguments, BUILTIN_DATASETS, snapshots)
+    elif arguments.action == 'snapshot':
+        snapshots.append(
+            {
+                'name': arguments.name,
+                'used': '0B',
+                'avail': '-',
+                'refer': '25K',
+                'mountpoint': '-',
+            },
+        )
+        save_snapshots(snapshots)
+    elif arguments.action == 'destroy':
+        snapshots = [
+            snapshot for snapshot in snapshots
+            if snapshot['name'] != arguments.name
+        ]
+        save_snapshots(snapshots)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/test_zfs.py b/tests/end-to-end/test_zfs.py
new file mode 100644
index 00000000..9a173157
--- /dev/null
+++ b/tests/end-to-end/test_zfs.py
@@ -0,0 +1,62 @@
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+
+def generate_configuration(config_path, repository_path):
+    '''
+    Generate borgmatic configuration into a file at the config path, and update the defaults so as
+    to work for testing (including injecting the given repository path and tacking on an encryption
+    passphrase).
+    '''
+    subprocess.check_call(f'borgmatic config generate --destination {config_path}'.split(' '))
+    config = (
+        open(config_path)
+        .read()
+        .replace('ssh://user@backupserver/./sourcehostname.borg', repository_path)
+        .replace('- path: /mnt/backup', '')
+        .replace('label: local', '')
+        .replace('- /home', f'- {config_path}')
+        .replace('- /etc', '- /pool/dataset')
+        .replace('- /var/log/syslog*', '')
+        + 'encryption_passphrase: "test"\n'
+        + 'zfs:\n'
+        + '    zfs_command: python3 /app/tests/end-to-end/commands/fake_zfs.py\n'
+        + '    mount_command: python3 /app/tests/end-to-end/commands/fake_mount.py\n'
+        + '    umount_command: python3 /app/tests/end-to-end/commands/fake_umount.py'
+    )
+    config_file = open(config_path, 'w')
+    config_file.write(config)
+    config_file.close()
+
+
+def test_zfs_create_and_list():
+    temporary_directory = tempfile.mkdtemp()
+    repository_path = os.path.join(temporary_directory, 'test.borg')
+
+    try:
+        config_path = os.path.join(temporary_directory, 'test.yaml')
+        generate_configuration(config_path, repository_path)
+
+        subprocess.check_call(
+            f'borgmatic -v 2 --config {config_path} repo-create --encryption repokey'.split(' ')
+        )
+
+        # Run a create action to exercise ZFS snapshotting and backup.
+        subprocess.check_call(f'borgmatic --config {config_path} create'.split(' '))
+
+        # List the resulting archive and assert that the snapshotted files are there.
+        output = subprocess.check_output(
+            f'borgmatic --config {config_path} list --archive latest'.split(' ')
+        ).decode(sys.stdout.encoding)
+
+        assert 'pool/dataset/subdir/file.txt' in output
+
+        # Assert that the snapshot has been deleted.
+        assert not subprocess.check_output(
+            f'python3 /app/tests/end-to-end/commands/fake_zfs.py list -H -t snapshot'.split(' ')
+        )
+    finally:
+        shutil.rmtree(temporary_directory)

From 03bbe77dd93fec8e8be93309c6471b3ff4bacfb2 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Thu, 5 Dec 2024 17:35:44 -0800
Subject: [PATCH 25/33] Add an end-to-end test for the Btrfs hook using a fake
 Btrfs binary (#80).

---
 borgmatic/hooks/data_source/btrfs.py      |  3 +-
 scripts/run-full-tests                    |  2 +-
 tests/end-to-end/commands/fake_btrfs.py   | 86 +++++++++++++++++++++++
 tests/end-to-end/commands/fake_findmnt.py | 34 +++++++++
 tests/end-to-end/test_btrfs.py            | 61 ++++++++++++++++
 tests/end-to-end/test_zfs.py              |  2 +-
 6 files changed, 185 insertions(+), 3 deletions(-)
 create mode 100755 tests/end-to-end/commands/fake_btrfs.py
 create mode 100755 tests/end-to-end/commands/fake_findmnt.py
 create mode 100644 tests/end-to-end/test_btrfs.py

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index c99ee04b..54f90a58 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -26,7 +26,8 @@ def get_filesystem_mount_points(findmnt_command):
     findmnt_output = borgmatic.execute.execute_command_and_capture_output(
         tuple(findmnt_command.split(' '))
         + (
-            '-nt',
+            '-n',  # No headings.
+            '-t',  # Filesystem type.
             'btrfs',
         )
     )
diff --git a/scripts/run-full-tests b/scripts/run-full-tests
index d57340e1..8dccdb12 100755
--- a/scripts/run-full-tests
+++ b/scripts/run-full-tests
@@ -25,5 +25,5 @@ python3 -m pip install --no-cache --upgrade pip==24.2 setuptools==72.1.0
 pip3 install --ignore-installed tox==4.11.3
 export COVERAGE_FILE=/tmp/.coverage
 
-tox --workdir /tmp/.tox --sitepackages
+#tox --workdir /tmp/.tox --sitepackages
 tox --workdir /tmp/.tox --sitepackages -e end-to-end
diff --git a/tests/end-to-end/commands/fake_btrfs.py b/tests/end-to-end/commands/fake_btrfs.py
new file mode 100755
index 00000000..ad6c15e4
--- /dev/null
+++ b/tests/end-to-end/commands/fake_btrfs.py
@@ -0,0 +1,86 @@
+import argparse
+import json
+import os
+import shutil
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    global_parser = argparse.ArgumentParser(add_help=False)
+    action_parsers = global_parser.add_subparsers(dest='action')
+
+    subvolume_parser = action_parsers.add_parser('subvolume')
+    subvolume_subparser = subvolume_parser.add_subparsers(dest='subaction')
+
+    list_parser = subvolume_subparser.add_parser('list')
+    list_parser.add_argument('-s', dest='snapshots_only', action='store_true')
+    list_parser.add_argument('subvolume_path')
+
+    snapshot_parser = subvolume_subparser.add_parser('snapshot')
+    snapshot_parser.add_argument('-r', dest='read_only', action='store_true')
+    snapshot_parser.add_argument('subvolume_path')
+    snapshot_parser.add_argument('snapshot_path')
+
+    delete_parser = subvolume_subparser.add_parser('delete')
+    delete_parser.add_argument('snapshot_path')
+
+    return global_parser.parse_args(unparsed_arguments)
+
+
+BUILTIN_SUBVOLUME_LIST_LINES = (
+    '261 gen 29 top level 5 path sub',
+    '262 gen 29 top level 5 path other',
+)
+SUBVOLUME_LIST_LINE_PREFIX = '263 gen 29 top level 5 path '
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_btrfs.json'))
+    except FileNotFoundError:
+        return []
+
+
+def save_snapshots(snapshot_paths):
+    json.dump(snapshot_paths, open('/tmp/fake_btrfs.json', 'w'))
+
+
+def print_subvolume_list(arguments, snapshot_paths):
+    assert arguments.subvolume_path == '/mnt/subvolume'
+
+    if not arguments.snapshots_only:
+        for line in BUILTIN_SUBVOLUME_LIST_LINES:
+            print(line)
+
+    for snapshot_path in snapshot_paths:
+        print(SUBVOLUME_LIST_LINE_PREFIX + snapshot_path[snapshot_path.index('.borgmatic-snapshot-'):])
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+    snapshot_paths = load_snapshots()
+
+    if arguments.subaction == 'list':
+        print_subvolume_list(arguments, snapshot_paths)
+    elif arguments.subaction == 'snapshot':
+        snapshot_paths.append(arguments.snapshot_path)
+        save_snapshots(snapshot_paths)
+
+        subdirectory = os.path.join(arguments.snapshot_path, 'subdir')
+        os.makedirs(subdirectory, mode=0o700, exist_ok=True)
+        test_file = open(os.path.join(subdirectory, 'file.txt'), 'w')
+        test_file.write('contents')
+        test_file.close()
+    elif arguments.subaction == 'delete':
+        subdirectory = os.path.join(arguments.snapshot_path, 'subdir')
+        shutil.rmtree(subdirectory)
+
+        snapshot_paths = [
+            snapshot_path for snapshot_path in snapshot_paths
+            if snapshot_path.endswith('/' + arguments.snapshot_path)
+        ]
+        save_snapshots(snapshot_paths)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_findmnt.py b/tests/end-to-end/commands/fake_findmnt.py
new file mode 100755
index 00000000..ddc6c787
--- /dev/null
+++ b/tests/end-to-end/commands/fake_findmnt.py
@@ -0,0 +1,34 @@
+import argparse
+import json
+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')
+
+    return parser.parse_args(unparsed_arguments)
+
+
+BUILTIN_FILESYSTEM_MOUNT_LINES = (
+    '/mnt/subvolume /dev/loop1 btrfs rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/',
+)
+
+
+def print_filesystem_mounts(arguments):
+    for line in BUILTIN_FILESYSTEM_MOUNT_LINES:
+        print(line)
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+
+    assert not arguments.headings
+    assert arguments.type == 'btrfs'
+
+    print_filesystem_mounts(arguments)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/test_btrfs.py b/tests/end-to-end/test_btrfs.py
new file mode 100644
index 00000000..4886120d
--- /dev/null
+++ b/tests/end-to-end/test_btrfs.py
@@ -0,0 +1,61 @@
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+
+def generate_configuration(config_path, repository_path):
+    '''
+    Generate borgmatic configuration into a file at the config path, and update the defaults so as
+    to work for testing (including injecting the given repository path and tacking on an encryption
+    passphrase).
+    '''
+    subprocess.check_call(f'borgmatic config generate --destination {config_path}'.split(' '))
+    config = (
+        open(config_path)
+        .read()
+        .replace('ssh://user@backupserver/./sourcehostname.borg', repository_path)
+        .replace('- path: /mnt/backup', '')
+        .replace('label: local', '')
+        .replace('- /home', f'- {config_path}')
+        .replace('- /etc', '- /mnt/subvolume/subdir')
+        .replace('- /var/log/syslog*', '')
+        + 'encryption_passphrase: "test"\n'
+        + 'btrfs:\n'
+        + '    btrfs_command: python3 /app/tests/end-to-end/commands/fake_btrfs.py\n'
+        + '    findmnt_command: python3 /app/tests/end-to-end/commands/fake_findmnt.py\n'
+    )
+    config_file = open(config_path, 'w')
+    config_file.write(config)
+    config_file.close()
+
+
+def test_btrfs_create_and_list():
+    temporary_directory = tempfile.mkdtemp()
+    repository_path = os.path.join(temporary_directory, 'test.borg')
+
+    try:
+        config_path = os.path.join(temporary_directory, 'test.yaml')
+        generate_configuration(config_path, repository_path)
+
+        subprocess.check_call(
+            f'borgmatic -v 2 --config {config_path} repo-create --encryption repokey'.split(' ')
+        )
+
+        # Run a create action to exercise Btrfs snapshotting and backup.
+        subprocess.check_call(f'borgmatic --config {config_path} create'.split(' '))
+
+        # List the resulting archive and assert that the snapshotted files are there.
+        output = subprocess.check_output(
+            f'borgmatic --config {config_path} list --archive latest'.split(' ')
+        ).decode(sys.stdout.encoding)
+
+        assert 'mnt/subvolume/subdir/file.txt' in output
+
+        # Assert that the snapshot has been deleted.
+        assert not subprocess.check_output(
+            f'python3 /app/tests/end-to-end/commands/fake_btrfs.py subvolume list -s /mnt/subvolume'.split(' ')
+        )
+    finally:
+        shutil.rmtree(temporary_directory)
diff --git a/tests/end-to-end/test_zfs.py b/tests/end-to-end/test_zfs.py
index 9a173157..7c2cd7f6 100644
--- a/tests/end-to-end/test_zfs.py
+++ b/tests/end-to-end/test_zfs.py
@@ -19,7 +19,7 @@ def generate_configuration(config_path, repository_path):
         .replace('- path: /mnt/backup', '')
         .replace('label: local', '')
         .replace('- /home', f'- {config_path}')
-        .replace('- /etc', '- /pool/dataset')
+        .replace('- /etc', '- /pool/dataset/subdir')
         .replace('- /var/log/syslog*', '')
         + 'encryption_passphrase: "test"\n'
         + 'zfs:\n'

From ec9e1a8223027edb837f9d0be88a32f5ccc5cb39 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Thu, 5 Dec 2024 22:46:50 -0800
Subject: [PATCH 26/33] LVM hook end-to-end tests, not quite working yet (#80).

---
 borgmatic/config/schema.yaml               |  6 +-
 borgmatic/hooks/data_source/lvm.py         | 12 ++--
 borgmatic/hooks/data_source/zfs.py         |  2 +
 scripts/run-full-tests                     |  2 +-
 tests/end-to-end/commands/fake_lsblk.py    | 79 ++++++++++++++++++++++
 tests/end-to-end/commands/fake_lvcreate.py | 43 ++++++++++++
 tests/end-to-end/commands/fake_lvremove.py | 38 +++++++++++
 tests/end-to-end/commands/fake_lvs.py      | 52 ++++++++++++++
 tests/end-to-end/commands/fake_mount.py    |  3 +
 tests/end-to-end/test_lvm.py               | 66 ++++++++++++++++++
 10 files changed, 293 insertions(+), 10 deletions(-)
 create mode 100644 tests/end-to-end/commands/fake_lsblk.py
 create mode 100644 tests/end-to-end/commands/fake_lvcreate.py
 create mode 100644 tests/end-to-end/commands/fake_lvremove.py
 create mode 100644 tests/end-to-end/commands/fake_lvs.py
 create mode 100644 tests/end-to-end/test_lvm.py

diff --git a/borgmatic/config/schema.yaml b/borgmatic/config/schema.yaml
index e0a7faf3..081cc966 100644
--- a/borgmatic/config/schema.yaml
+++ b/borgmatic/config/schema.yaml
@@ -2333,11 +2333,11 @@ properties:
                 description: |
                     Command to use instead of "lvs".
                 example: /usr/local/bin/lvs
-            lsbrk_command:
+            lsblk_command:
                 type: string
                 description: |
-                    Command to use instead of "lsbrk".
-                example: /usr/local/bin/lsbrk
+                    Command to use instead of "lsblk".
+                example: /usr/local/bin/lsblk
             mount_command:
                 type: string
                 description: |
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 48418f43..6937bb85 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -39,10 +39,10 @@ def get_logical_volumes(lsblk_command, source_directories=None):
     '''
     try:
         devices_info = json.loads(
-            subprocess.check_output(
-                (
-                    # Use lsblk instead of lvs here because lvs can't show active mounts.
-                    lsblk_command,
+            borgmatic.execute.execute_command_and_capture_output(
+                # Use lsblk instead of lvs here because lvs can't show active mounts.
+                tuple(lsblk_command.split(' '))
+                + (
                     '--output',
                     'name,path,mountpoint,type',
                     '--json',
@@ -229,7 +229,7 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
     )
 
 
-def delete_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
+def remove_snapshot(lvremove_command, snapshot_device_path):  # pragma: no cover
     '''
     Given an lvremove command to run and the device path of a snapshot, remove it it.
     '''
@@ -362,7 +362,7 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
         logger.debug(f'{log_prefix}: Deleting LVM snapshot {snapshot.name}{dry_run_label}')
 
         if not dry_run:
-            delete_snapshot(lvremove_command, snapshot.device_path)
+            remove_snapshot(lvremove_command, snapshot.device_path)
 
 
 def make_data_source_dump_patterns(
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index d5dac5b0..630f707f 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -144,6 +144,8 @@ def mount_snapshot(mount_command, full_snapshot_name, snapshot_mount_path):  # p
         + (
             '-t',
             'zfs',
+            '-o',
+            'ro',
             full_snapshot_name,
             snapshot_mount_path,
         ),
diff --git a/scripts/run-full-tests b/scripts/run-full-tests
index 8dccdb12..d57340e1 100755
--- a/scripts/run-full-tests
+++ b/scripts/run-full-tests
@@ -25,5 +25,5 @@ python3 -m pip install --no-cache --upgrade pip==24.2 setuptools==72.1.0
 pip3 install --ignore-installed tox==4.11.3
 export COVERAGE_FILE=/tmp/.coverage
 
-#tox --workdir /tmp/.tox --sitepackages
+tox --workdir /tmp/.tox --sitepackages
 tox --workdir /tmp/.tox --sitepackages -e end-to-end
diff --git a/tests/end-to-end/commands/fake_lsblk.py b/tests/end-to-end/commands/fake_lsblk.py
new file mode 100644
index 00000000..b9b585bc
--- /dev/null
+++ b/tests/end-to-end/commands/fake_lsblk.py
@@ -0,0 +1,79 @@
+import argparse
+import json
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+
+    parser.add_argument('--output', required=True)
+    parser.add_argument('--json', action='store_true', required=True)
+    parser.add_argument('--list', action='store_true', required=True)
+
+    return parser.parse_args(unparsed_arguments)
+
+
+BUILTIN_BLOCK_DEVICES = {
+   'blockdevices': [
+      {
+         'name': 'loop0',
+         'path': '/dev/loop0',
+         'mountpoint': None,
+         'type': 'loop'
+      },
+      {
+         'name': 'cryptroot',
+         'path': '/dev/mapper/cryptroot',
+         'mountpoint': '/',
+         'type': 'crypt'
+      },{
+         'name': 'vgroup-lvolume',
+         'path': '/dev/mapper/vgroup-lvolume',
+         'mountpoint': '/mnt/lvolume',
+         'type': 'lvm'
+      },
+      {
+         'name': 'vgroup-lvolume-real',
+         'path': '/dev/mapper/vgroup-lvolume-real',
+         'mountpoint': None,
+         'type': 'lvm'
+      },
+   ]
+}
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_lvm.json'))
+    except FileNotFoundError:
+        return []
+
+
+def print_logical_volumes_json(arguments, snapshots):
+    data = dict(BUILTIN_BLOCK_DEVICES)
+
+    for snapshot in snapshots:
+        data['blockdevices'].extend(
+            {
+               'name': snapshot['lv_name'],
+               'path': snapshot['lv_path'],
+               'mountpoint': None,
+               'type': 'lvm'
+            }
+            for snapshot in snapshots
+        )
+
+    print(json.dumps(data))
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+    snapshots = load_snapshots()
+
+    assert arguments.output == 'name,path,mountpoint,type'
+
+    print_logical_volumes_json(arguments, snapshots)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_lvcreate.py b/tests/end-to-end/commands/fake_lvcreate.py
new file mode 100644
index 00000000..a389b710
--- /dev/null
+++ b/tests/end-to-end/commands/fake_lvcreate.py
@@ -0,0 +1,43 @@
+import argparse
+import json
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+
+    parser.add_argument('--snapshot', action='store_true', required=True)
+    parser.add_argument('--extents')
+    parser.add_argument('--size')
+    parser.add_argument('--name', dest='snapshot_name', required=True)
+    parser.add_argument('logical_volume_device')
+
+    return parser.parse_args(unparsed_arguments)
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_lvm.json'))
+    except FileNotFoundError:
+        return []
+
+
+def save_snapshots(snapshots):
+    json.dump(snapshots, open('/tmp/fake_lvm.json', 'w'))
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+    snapshots = load_snapshots()
+
+    assert arguments.extents or arguments.size
+
+    snapshots.append(
+        {'lv_name': arguments.snapshot_name, 'lv_path': f'/dev/vgroup/{arguments.snapshot_name}'},
+    )
+
+    save_snapshots(snapshots)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_lvremove.py b/tests/end-to-end/commands/fake_lvremove.py
new file mode 100644
index 00000000..552e81bd
--- /dev/null
+++ b/tests/end-to-end/commands/fake_lvremove.py
@@ -0,0 +1,38 @@
+import argparse
+import json
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+
+    parser.add_argument('--force', action='store_true', required=True)
+    parser.add_argument('snapshot_device')
+
+    return parser.parse_args(unparsed_arguments)
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_lvm.json'))
+    except FileNotFoundError:
+        return []
+
+
+def save_snapshots(snapshots):
+    json.dump(snapshots, open('/tmp/fake_lvm.json', 'w'))
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+
+    snapshots = [
+        snapshot for snapshot in load_snapshots()
+        if snapshot['lv_path'] == arguments.snapshot_device
+    ]
+
+    save_snapshots(snapshots)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_lvs.py b/tests/end-to-end/commands/fake_lvs.py
new file mode 100644
index 00000000..7a1e28e5
--- /dev/null
+++ b/tests/end-to-end/commands/fake_lvs.py
@@ -0,0 +1,52 @@
+import argparse
+import json
+import sys
+
+
+def parse_arguments(*unparsed_arguments):
+    parser = argparse.ArgumentParser(add_help=False)
+
+    parser.add_argument('--report-format', required=True)
+    parser.add_argument('--options', required=True)
+    parser.add_argument('--select', required=True)
+
+    return parser.parse_args(unparsed_arguments)
+
+
+def load_snapshots():
+    try:
+        return json.load(open('/tmp/fake_lvm.json'))
+    except FileNotFoundError:
+        return []
+
+
+def print_snapshots_json(arguments, snapshots):
+    assert arguments.report_format == 'json'
+    assert arguments.options == 'lv_name,lv_path'
+    assert arguments.select == 'lv_attr =~ ^s'
+
+    print(
+        json.dumps(
+            {
+                'report': [
+                    {
+                        'lv': snapshots,
+                    }
+                ]
+                ,
+                'log': [
+                ]
+            }
+        )
+    )
+
+
+def main():
+    arguments = parse_arguments(*sys.argv[1:])
+    snapshots = load_snapshots()
+
+    print_snapshots_json(arguments, snapshots)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tests/end-to-end/commands/fake_mount.py b/tests/end-to-end/commands/fake_mount.py
index 21e161c9..b69b85ed 100755
--- a/tests/end-to-end/commands/fake_mount.py
+++ b/tests/end-to-end/commands/fake_mount.py
@@ -6,6 +6,7 @@ import sys
 def parse_arguments(*unparsed_arguments):
     parser = argparse.ArgumentParser(add_help=False)
     parser.add_argument('-t', dest='type')
+    parser.add_argument('-o', dest='options')
     parser.add_argument('snapshot_name')
     parser.add_argument('mount_point')
 
@@ -15,6 +16,8 @@ def parse_arguments(*unparsed_arguments):
 def main():
     arguments = parse_arguments(*sys.argv[1:])
 
+    assert arguments.options == 'ro'
+
     subdirectory = os.path.join(arguments.mount_point, 'subdir')
     os.mkdir(subdirectory)
     test_file = open(os.path.join(subdirectory, 'file.txt'), 'w')
diff --git a/tests/end-to-end/test_lvm.py b/tests/end-to-end/test_lvm.py
new file mode 100644
index 00000000..6ea7e377
--- /dev/null
+++ b/tests/end-to-end/test_lvm.py
@@ -0,0 +1,66 @@
+import json
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+
+def generate_configuration(config_path, repository_path):
+    '''
+    Generate borgmatic configuration into a file at the config path, and update the defaults so as
+    to work for testing (including injecting the given repository path and tacking on an encryption
+    passphrase).
+    '''
+    subprocess.check_call(f'borgmatic config generate --destination {config_path}'.split(' '))
+    config = (
+        open(config_path)
+        .read()
+        .replace('ssh://user@backupserver/./sourcehostname.borg', repository_path)
+        .replace('- path: /mnt/backup', '')
+        .replace('label: local', '')
+        .replace('- /home', f'- {config_path}')
+        .replace('- /etc', '- /mnt/lvolume/subdir')
+        .replace('- /var/log/syslog*', '')
+        + 'encryption_passphrase: "test"\n'
+        + 'lvm:\n'
+        + '    lsblk_command: python3 /app/tests/end-to-end/commands/fake_lsblk.py\n'
+        + '    lvcreate_command: python3 /app/tests/end-to-end/commands/fake_lvcreate.py\n'
+        + '    lvremove_command: python3 /app/tests/end-to-end/commands/fake_lvremove.py\n'
+        + '    lvs_command: python3 /app/tests/end-to-end/commands/fake_lvs.py\n'
+        + '    mount_command: python3 /app/tests/end-to-end/commands/fake_mount.py\n'
+        + '    umount_command: python3 /app/tests/end-to-end/commands/fake_umount.py\n'
+    )
+    config_file = open(config_path, 'w')
+    config_file.write(config)
+    config_file.close()
+
+
+def test_lvm_create_and_list():
+    temporary_directory = tempfile.mkdtemp()
+    repository_path = os.path.join(temporary_directory, 'test.borg')
+
+    try:
+        config_path = os.path.join(temporary_directory, 'test.yaml')
+        generate_configuration(config_path, repository_path)
+
+        subprocess.check_call(
+            f'borgmatic -v 2 --config {config_path} repo-create --encryption repokey'.split(' ')
+        )
+
+        # Run a create action to exercise LVM snapshotting and backup.
+        subprocess.check_call(f'borgmatic --config {config_path} create'.split(' '))
+
+        # List the resulting archive and assert that the snapshotted files are there.
+        output = subprocess.check_output(
+            f'borgmatic --config {config_path} list --archive latest'.split(' ')
+        ).decode(sys.stdout.encoding)
+
+        assert 'mnt/lvolume/subdir/file.txt' in output
+
+        # Assert that the snapshot has been deleted.
+        assert not json.loads(subprocess.check_output(
+            'python3 /app/tests/end-to-end/commands/fake_lvs.py --report-format json --options lv_name,lv_path --select'.split(' ') + ['lv_attr =~ ^s']
+        ))['report'][0]['lv']
+    finally:
+        shutil.rmtree(temporary_directory)

From 140fc248b671761db2e78fe36eabbb3c23cb0f00 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 09:39:24 -0800
Subject: [PATCH 27/33] Fix LVM end-to-end tests (#80).

---
 tests/end-to-end/commands/fake_lvremove.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/end-to-end/commands/fake_lvremove.py b/tests/end-to-end/commands/fake_lvremove.py
index 552e81bd..ece5dbea 100644
--- a/tests/end-to-end/commands/fake_lvremove.py
+++ b/tests/end-to-end/commands/fake_lvremove.py
@@ -28,7 +28,7 @@ def main():
 
     snapshots = [
         snapshot for snapshot in load_snapshots()
-        if snapshot['lv_path'] == arguments.snapshot_device
+        if snapshot['lv_path'] != arguments.snapshot_device
     ]
 
     save_snapshots(snapshots)

From 7f2e38d0616644b88ace9aa5eea4860640256386 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 09:40:32 -0800
Subject: [PATCH 28/33] Fix file permissions (#80).

---
 tests/end-to-end/commands/fake_btrfs.py   | 0
 tests/end-to-end/commands/fake_findmnt.py | 0
 tests/end-to-end/commands/fake_mount.py   | 0
 tests/end-to-end/commands/fake_umount.py  | 0
 tests/end-to-end/commands/fake_zfs.py     | 0
 5 files changed, 0 insertions(+), 0 deletions(-)
 mode change 100755 => 100644 tests/end-to-end/commands/fake_btrfs.py
 mode change 100755 => 100644 tests/end-to-end/commands/fake_findmnt.py
 mode change 100755 => 100644 tests/end-to-end/commands/fake_mount.py
 mode change 100755 => 100644 tests/end-to-end/commands/fake_umount.py
 mode change 100755 => 100644 tests/end-to-end/commands/fake_zfs.py

diff --git a/tests/end-to-end/commands/fake_btrfs.py b/tests/end-to-end/commands/fake_btrfs.py
old mode 100755
new mode 100644
diff --git a/tests/end-to-end/commands/fake_findmnt.py b/tests/end-to-end/commands/fake_findmnt.py
old mode 100755
new mode 100644
diff --git a/tests/end-to-end/commands/fake_mount.py b/tests/end-to-end/commands/fake_mount.py
old mode 100755
new mode 100644
diff --git a/tests/end-to-end/commands/fake_umount.py b/tests/end-to-end/commands/fake_umount.py
old mode 100755
new mode 100644
diff --git a/tests/end-to-end/commands/fake_zfs.py b/tests/end-to-end/commands/fake_zfs.py
old mode 100755
new mode 100644

From b999d2dc4d18f68eb9048f6b7c5fbe9c375d7824 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 10:27:47 -0800
Subject: [PATCH 29/33] Add some missing test coverage (#80).

---
 borgmatic/hooks/data_source/btrfs.py          |  2 +-
 borgmatic/hooks/data_source/lvm.py            |  6 +-
 borgmatic/hooks/data_source/zfs.py            |  6 +-
 tests/unit/hooks/data_source/test_snapshot.py | 26 +++++++
 tests/unit/hooks/data_source/test_zfs.py      | 70 +++++++++++++++++++
 5 files changed, 103 insertions(+), 7 deletions(-)
 create mode 100644 tests/unit/hooks/data_source/test_snapshot.py

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 54f90a58..986f25dd 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -146,7 +146,7 @@ def make_snapshot_exclude_path(subvolume_path):  # pragma: no cover
     )
 
 
-def make_borg_source_directory_path(subvolume_path, source_directory):
+def make_borg_source_directory_path(subvolume_path, source_directory):  # pragma: no cover
     '''
     Given the path to a subvolume and a source directory inside it, make a corresponding path for
     the source directory within a snapshot path intended for giving to Borg.
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 6937bb85..ed8db471 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -330,9 +330,9 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not dry_run:
                 shutil.rmtree(snapshot_mount_path, ignore_errors=True)
 
-            # If the delete was successful, that means there's nothing to unmount.
-            if not os.path.isdir(snapshot_mount_path):
-                continue
+                # If the delete was successful, that means there's nothing to unmount.
+                if not os.path.isdir(snapshot_mount_path):
+                    continue
 
             logger.debug(
                 f'{log_prefix}: Unmounting LVM snapshot at {snapshot_mount_path}{dry_run_label}'
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 630f707f..bc44316c 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -332,9 +332,9 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
             if not dry_run:
                 shutil.rmtree(snapshot_mount_path, ignore_errors=True)
 
-            # If the delete was successful, that means there's nothing to unmount.
-            if not os.path.isdir(snapshot_mount_path):
-                continue
+                # If the delete was successful, that means there's nothing to unmount.
+                if not os.path.isdir(snapshot_mount_path):
+                    continue
 
             logger.debug(
                 f'{log_prefix}: Unmounting ZFS snapshot at {snapshot_mount_path}{dry_run_label}'
diff --git a/tests/unit/hooks/data_source/test_snapshot.py b/tests/unit/hooks/data_source/test_snapshot.py
new file mode 100644
index 00000000..196269c5
--- /dev/null
+++ b/tests/unit/hooks/data_source/test_snapshot.py
@@ -0,0 +1,26 @@
+from borgmatic.hooks.data_source import snapshot as module
+
+
+def test_get_contained_directories_without_candidates_returns_empty():
+    assert module.get_contained_directories('/mnt', {}) == ()
+
+
+def test_get_contained_directories_with_self_candidate_returns_self():
+    candidates = {'/foo', '/mnt', '/bar'}
+
+    assert module.get_contained_directories('/mnt', candidates) == ('/mnt',)
+    assert candidates == {'/foo', '/bar'}
+
+
+def test_get_contained_directories_with_child_candidate_returns_child():
+    candidates = {'/foo', '/mnt/subdir', '/bar'}
+
+    assert module.get_contained_directories('/mnt', candidates) == ('/mnt/subdir',)
+    assert candidates == {'/foo', '/bar'}
+
+
+def test_get_contained_directories_with_grandchild_candidate_returns_child():
+    candidates = {'/foo', '/mnt/sub/dir', '/bar'}
+
+    assert module.get_contained_directories('/mnt', candidates) == ('/mnt/sub/dir',)
+    assert candidates == {'/foo', '/bar'}
diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py
index 7323cabd..ab1daa6e 100644
--- a/tests/unit/hooks/data_source/test_zfs.py
+++ b/tests/unit/hooks/data_source/test_zfs.py
@@ -216,6 +216,46 @@ def test_dump_data_sources_with_dry_run_skips_commands_and_does_not_touch_source
     assert source_directories == ['/mnt/dataset']
 
 
+def test_dump_data_sources_ignores_mismatch_between_source_directories_and_contained_source_directories():
+    flexmock(module).should_receive('get_datasets_to_backup').and_return(
+        (
+            flexmock(
+                name='dataset',
+                mount_point='/mnt/dataset',
+                contained_source_directories=('/mnt/dataset/subdir',),
+            )
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    full_snapshot_name = 'dataset@borgmatic-1234'
+    flexmock(module).should_receive('snapshot_dataset').with_args(
+        'zfs',
+        full_snapshot_name,
+    ).once()
+    snapshot_mount_path = '/run/borgmatic/zfs_snapshots/./mnt/dataset'
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount',
+        full_snapshot_name,
+        module.os.path.normpath(snapshot_mount_path),
+    ).once()
+    source_directories = ['/hmm']
+
+    assert (
+        module.dump_data_sources(
+            hook_config={},
+            config={'source_directories': '/mnt/dataset', 'zfs': {}},
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == ['/hmm', os.path.join(snapshot_mount_path, 'subdir')]
+
+
 def test_get_all_snapshots_parses_list_output():
     flexmock(module.borgmatic.execute).should_receive(
         'execute_command_and_capture_output'
@@ -418,6 +458,36 @@ 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_after_rmtree_succeeds():
+    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])
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/zfs_snapshots'
+    ).and_return(True)
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/zfs_snapshots/mnt/dataset'
+    ).and_return(True).and_return(False)
+    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': {}},
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
 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.borgmatic.config.paths).should_receive(

From f2d93b85b41d38484843858b08841a8145588cd0 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 13:59:38 -0800
Subject: [PATCH 30/33] Lots of LVM unit tests + code formatting (#80).

---
 borgmatic/hooks/data_source/lvm.py         |   9 +-
 borgmatic/hooks/data_source/zfs.py         |   5 +-
 tests/end-to-end/commands/fake_btrfs.py    |   8 +-
 tests/end-to-end/commands/fake_lsblk.py    |  49 +-
 tests/end-to-end/commands/fake_lvremove.py |   3 +-
 tests/end-to-end/commands/fake_lvs.py      |   6 +-
 tests/end-to-end/commands/fake_zfs.py      |  17 +-
 tests/end-to-end/test_btrfs.py             |   4 +-
 tests/end-to-end/test_lvm.py               |  11 +-
 tests/unit/hooks/data_source/test_btrfs.py |  35 +-
 tests/unit/hooks/data_source/test_lvm.py   | 617 +++++++++++++++++++++
 tests/unit/hooks/data_source/test_zfs.py   |   2 +-
 12 files changed, 695 insertions(+), 71 deletions(-)
 create mode 100644 tests/unit/hooks/data_source/test_lvm.py

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index ed8db471..6a5d0759 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -78,7 +78,7 @@ def snapshot_logical_volume(
     snapshot_name,
     logical_volume_device,
     snapshot_size,
-):  # pragma: no cover
+):
     '''
     Given an lvcreate command to run, a snapshot name, the path to the logical volume device to
     snapshot, and a snapshot size string, create a new LVM snapshot.
@@ -221,10 +221,7 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
     Given a umount command to run and the mount path of a snapshot, unmount it.
     '''
     borgmatic.execute.execute_command(
-        tuple(umount_command.split(' '))
-        + (
-            snapshot_mount_path,
-        ),
+        tuple(umount_command.split(' ')) + (snapshot_mount_path,),
         output_log_level=logging.DEBUG,
     )
 
@@ -279,6 +276,8 @@ def get_snapshots(lvs_command, snapshot_name=None):
             for snapshot in snapshot_info['report'][0]['lv']
             if snapshot_name is None or snapshot['lv_name'] == snapshot_name
         )
+    except IndexError:
+        raise ValueError(f'Invalid {lvs_command} output: Missing report data')
     except KeyError as error:
         raise ValueError(f'Invalid {lvs_command} output: Missing key "{error}"')
 
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index bc44316c..5903e9a4 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -239,10 +239,7 @@ def unmount_snapshot(umount_command, snapshot_mount_path):  # pragma: no cover
     Given a umount command to run and the mount path of a snapshot, unmount it.
     '''
     borgmatic.execute.execute_command(
-        tuple(umount_command.split(' '))
-        + (
-            snapshot_mount_path,
-        ),
+        tuple(umount_command.split(' ')) + (snapshot_mount_path,),
         output_log_level=logging.DEBUG,
     )
 
diff --git a/tests/end-to-end/commands/fake_btrfs.py b/tests/end-to-end/commands/fake_btrfs.py
index ad6c15e4..ee969597 100644
--- a/tests/end-to-end/commands/fake_btrfs.py
+++ b/tests/end-to-end/commands/fake_btrfs.py
@@ -53,7 +53,10 @@ def print_subvolume_list(arguments, snapshot_paths):
             print(line)
 
     for snapshot_path in snapshot_paths:
-        print(SUBVOLUME_LIST_LINE_PREFIX + snapshot_path[snapshot_path.index('.borgmatic-snapshot-'):])
+        print(
+            SUBVOLUME_LIST_LINE_PREFIX
+            + snapshot_path[snapshot_path.index('.borgmatic-snapshot-') :]
+        )
 
 
 def main():
@@ -76,7 +79,8 @@ def main():
         shutil.rmtree(subdirectory)
 
         snapshot_paths = [
-            snapshot_path for snapshot_path in snapshot_paths
+            snapshot_path
+            for snapshot_path in snapshot_paths
             if snapshot_path.endswith('/' + arguments.snapshot_path)
         ]
         save_snapshots(snapshot_paths)
diff --git a/tests/end-to-end/commands/fake_lsblk.py b/tests/end-to-end/commands/fake_lsblk.py
index b9b585bc..4ad9c552 100644
--- a/tests/end-to-end/commands/fake_lsblk.py
+++ b/tests/end-to-end/commands/fake_lsblk.py
@@ -14,31 +14,22 @@ def parse_arguments(*unparsed_arguments):
 
 
 BUILTIN_BLOCK_DEVICES = {
-   'blockdevices': [
-      {
-         'name': 'loop0',
-         'path': '/dev/loop0',
-         'mountpoint': None,
-         'type': 'loop'
-      },
-      {
-         'name': 'cryptroot',
-         'path': '/dev/mapper/cryptroot',
-         'mountpoint': '/',
-         'type': 'crypt'
-      },{
-         'name': 'vgroup-lvolume',
-         'path': '/dev/mapper/vgroup-lvolume',
-         'mountpoint': '/mnt/lvolume',
-         'type': 'lvm'
-      },
-      {
-         'name': 'vgroup-lvolume-real',
-         'path': '/dev/mapper/vgroup-lvolume-real',
-         'mountpoint': None,
-         'type': 'lvm'
-      },
-   ]
+    'blockdevices': [
+        {'name': 'loop0', 'path': '/dev/loop0', 'mountpoint': None, 'type': 'loop'},
+        {'name': 'cryptroot', 'path': '/dev/mapper/cryptroot', 'mountpoint': '/', 'type': 'crypt'},
+        {
+            'name': 'vgroup-lvolume',
+            'path': '/dev/mapper/vgroup-lvolume',
+            'mountpoint': '/mnt/lvolume',
+            'type': 'lvm',
+        },
+        {
+            'name': 'vgroup-lvolume-real',
+            'path': '/dev/mapper/vgroup-lvolume-real',
+            'mountpoint': None,
+            'type': 'lvm',
+        },
+    ]
 }
 
 
@@ -55,10 +46,10 @@ def print_logical_volumes_json(arguments, snapshots):
     for snapshot in snapshots:
         data['blockdevices'].extend(
             {
-               'name': snapshot['lv_name'],
-               'path': snapshot['lv_path'],
-               'mountpoint': None,
-               'type': 'lvm'
+                'name': snapshot['lv_name'],
+                'path': snapshot['lv_path'],
+                'mountpoint': None,
+                'type': 'lvm',
             }
             for snapshot in snapshots
         )
diff --git a/tests/end-to-end/commands/fake_lvremove.py b/tests/end-to-end/commands/fake_lvremove.py
index ece5dbea..e35fde9f 100644
--- a/tests/end-to-end/commands/fake_lvremove.py
+++ b/tests/end-to-end/commands/fake_lvremove.py
@@ -27,7 +27,8 @@ def main():
     arguments = parse_arguments(*sys.argv[1:])
 
     snapshots = [
-        snapshot for snapshot in load_snapshots()
+        snapshot
+        for snapshot in load_snapshots()
         if snapshot['lv_path'] != arguments.snapshot_device
     ]
 
diff --git a/tests/end-to-end/commands/fake_lvs.py b/tests/end-to-end/commands/fake_lvs.py
index 7a1e28e5..79f3bd97 100644
--- a/tests/end-to-end/commands/fake_lvs.py
+++ b/tests/end-to-end/commands/fake_lvs.py
@@ -32,10 +32,8 @@ def print_snapshots_json(arguments, snapshots):
                     {
                         'lv': snapshots,
                     }
-                ]
-                ,
-                'log': [
-                ]
+                ],
+                'log': [],
             }
         )
     )
diff --git a/tests/end-to-end/commands/fake_zfs.py b/tests/end-to-end/commands/fake_zfs.py
index 88c4d1e3..61f8ae5e 100644
--- a/tests/end-to-end/commands/fake_zfs.py
+++ b/tests/end-to-end/commands/fake_zfs.py
@@ -39,9 +39,6 @@ BUILTIN_DATASETS = (
 )
 
 
-
-
-
 def load_snapshots():
     try:
         return json.load(open('/tmp/fake_zfs.json'))
@@ -56,14 +53,9 @@ def save_snapshots(snapshots):
 def print_dataset_list(arguments, datasets, snapshots):
     properties = arguments.properties.split(',')
     data = (
-        (
-            tuple(property_name.upper() for property_name in properties),
-        )
-        if arguments.header else ()
+        (tuple(property_name.upper() for property_name in properties),) if arguments.header else ()
     ) + tuple(
-        tuple(
-            dataset.get(property_name, '-') for property_name in properties
-        )
+        tuple(dataset.get(property_name, '-') for property_name in properties)
         for dataset in (snapshots if arguments.type == 'snapshot' else datasets)
     )
 
@@ -92,10 +84,7 @@ def main():
         )
         save_snapshots(snapshots)
     elif arguments.action == 'destroy':
-        snapshots = [
-            snapshot for snapshot in snapshots
-            if snapshot['name'] != arguments.name
-        ]
+        snapshots = [snapshot for snapshot in snapshots if snapshot['name'] != arguments.name]
         save_snapshots(snapshots)
 
 
diff --git a/tests/end-to-end/test_btrfs.py b/tests/end-to-end/test_btrfs.py
index 4886120d..2c410774 100644
--- a/tests/end-to-end/test_btrfs.py
+++ b/tests/end-to-end/test_btrfs.py
@@ -55,7 +55,9 @@ def test_btrfs_create_and_list():
 
         # Assert that the snapshot has been deleted.
         assert not subprocess.check_output(
-            f'python3 /app/tests/end-to-end/commands/fake_btrfs.py subvolume list -s /mnt/subvolume'.split(' ')
+            f'python3 /app/tests/end-to-end/commands/fake_btrfs.py subvolume list -s /mnt/subvolume'.split(
+                ' '
+            )
         )
     finally:
         shutil.rmtree(temporary_directory)
diff --git a/tests/end-to-end/test_lvm.py b/tests/end-to-end/test_lvm.py
index 6ea7e377..06f02ea2 100644
--- a/tests/end-to-end/test_lvm.py
+++ b/tests/end-to-end/test_lvm.py
@@ -59,8 +59,13 @@ def test_lvm_create_and_list():
         assert 'mnt/lvolume/subdir/file.txt' in output
 
         # Assert that the snapshot has been deleted.
-        assert not json.loads(subprocess.check_output(
-            'python3 /app/tests/end-to-end/commands/fake_lvs.py --report-format json --options lv_name,lv_path --select'.split(' ') + ['lv_attr =~ ^s']
-        ))['report'][0]['lv']
+        assert not json.loads(
+            subprocess.check_output(
+                'python3 /app/tests/end-to-end/commands/fake_lvs.py --report-format json --options lv_name,lv_path --select'.split(
+                    ' '
+                )
+                + ['lv_attr =~ ^s']
+            )
+        )['report'][0]['lv']
     finally:
         shutil.rmtree(temporary_directory)
diff --git a/tests/unit/hooks/data_source/test_btrfs.py b/tests/unit/hooks/data_source/test_btrfs.py
index b6a9834a..00e4d186 100644
--- a/tests/unit/hooks/data_source/test_btrfs.py
+++ b/tests/unit/hooks/data_source/test_btrfs.py
@@ -21,7 +21,11 @@ def test_get_subvolumes_for_filesystem_parses_subvolume_list_output():
         'ID 270 gen 107 top level 5 path subvol1\nID 272 gen 74 top level 5 path subvol2\n'
     )
 
-    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == ('/mnt', '/mnt/subvol1', '/mnt/subvol2')
+    assert module.get_subvolumes_for_filesystem('btrfs', '/mnt') == (
+        '/mnt',
+        '/mnt/subvol1',
+        '/mnt/subvol2',
+    )
 
 
 def test_get_subvolumes_for_filesystem_skips_empty_subvolume_paths():
@@ -202,7 +206,9 @@ def test_dump_data_sources_uses_custom_findmnt_command_in_commands():
     config = {'btrfs': {'findmnt_command': '/usr/local/bin/findmnt'}}
     flexmock(module).should_receive('get_subvolumes').with_args(
         'btrfs', '/usr/local/bin/findmnt', source_directories
-    ).and_return((module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),)).once()
+    ).and_return(
+        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),)
+    ).once()
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
     )
@@ -301,7 +307,10 @@ def test_dump_data_sources_snapshots_adds_to_existing_exclude_patterns():
     source_directories = ['/foo', '/mnt/subvol1']
     config = {'btrfs': {}, 'exclude_patterns': ['/bar']}
     flexmock(module).should_receive('get_subvolumes').and_return(
-        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
     )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/mnt/subvol1'
@@ -359,7 +368,10 @@ def test_dump_data_sources_snapshots_adds_to_existing_exclude_patterns():
 def test_remove_data_source_dumps_deletes_snapshots():
     config = {'btrfs': {}}
     flexmock(module).should_receive('get_subvolumes').and_return(
-        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
     )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
@@ -482,7 +494,10 @@ def test_remove_data_source_dumps_with_get_subvolumes_called_process_error_bails
 def test_remove_data_source_dumps_with_dry_run_skips_deletes():
     config = {'btrfs': {}}
     flexmock(module).should_receive('get_subvolumes').and_return(
-        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
     )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
@@ -562,7 +577,10 @@ def test_remove_data_source_dumps_without_subvolumes_skips_deletes():
 def test_remove_data_source_without_snapshots_skips_deletes():
     config = {'btrfs': {}}
     flexmock(module).should_receive('get_subvolumes').and_return(
-        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
     )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
@@ -603,7 +621,10 @@ def test_remove_data_source_without_snapshots_skips_deletes():
 def test_remove_data_source_dumps_with_delete_snapshot_file_not_found_error_bails():
     config = {'btrfs': {}}
     flexmock(module).should_receive('get_subvolumes').and_return(
-        (module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)), module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)))
+        (
+            module.Subvolume('/mnt/subvol1', contained_source_directories=('/mnt/subvol1',)),
+            module.Subvolume('/mnt/subvol2', contained_source_directories=('/mnt/subvol2',)),
+        )
     )
     flexmock(module).should_receive('make_snapshot_path').with_args('/mnt/subvol1').and_return(
         '/mnt/subvol1/.borgmatic-1234/./mnt/subvol1'
diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py
new file mode 100644
index 00000000..7dadbe36
--- /dev/null
+++ b/tests/unit/hooks/data_source/test_lvm.py
@@ -0,0 +1,617 @@
+from flexmock import flexmock
+import pytest
+
+from borgmatic.hooks.data_source import lvm as module
+
+
+def test_get_logical_volumes_filters_by_source_directories():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return(
+        '''
+        {
+            "blockdevices": [
+                {
+                   "name": "vgroup-notmounted",
+                   "path": "/dev/mapper/vgroup-notmounted",
+                   "mountpoint": null,
+                   "type": "lvm"
+                }, {
+                   "name": "vgroup-lvolume",
+                   "path": "/dev/mapper/vgroup-lvolume",
+                   "mountpoint": "/mnt/lvolume",
+                   "type": "lvm"
+                }, {
+                   "name": "vgroup-other",
+                   "path": "/dev/mapper/vgroup-other",
+                   "mountpoint": "/mnt/other",
+                   "type": "lvm"
+                }, {
+                   "name": "vgroup-notlvm",
+                   "path": "/dev/mapper/vgroup-notlvm",
+                   "mountpoint": "/mnt/notlvm",
+                   "type": "notlvm"
+                }
+            ]
+        }
+        '''
+    )
+    contained = {'/mnt/lvolume', '/mnt/lvolume/subdir'}
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args(None, contained).never()
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/mnt/lvolume', contained).and_return(('/mnt/lvolume', '/mnt/lvolume/subdir'))
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/mnt/other', contained).and_return(())
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).with_args('/mnt/notlvm', contained).never()
+
+    assert module.get_logical_volumes(
+        'lsblk', source_directories=('/mnt/lvolume', '/mnt/lvolume/subdir')
+    ) == (
+        module.Logical_volume(
+            'vgroup-lvolume',
+            '/dev/mapper/vgroup-lvolume',
+            '/mnt/lvolume',
+            ('/mnt/lvolume', '/mnt/lvolume/subdir'),
+        ),
+    )
+
+
+def test_get_logical_volumes_with_invalid_lsblk_json_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return('{')
+
+    contained = {'/mnt/lvolume', '/mnt/lvolume/subdir'}
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).never()
+
+    with pytest.raises(ValueError):
+        module.get_logical_volumes(
+            'lsblk', source_directories=('/mnt/lvolume', '/mnt/lvolume/subdir')
+        )
+
+
+def test_get_logical_volumes_with_lsblk_json_missing_keys_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return('{"block_devices": [{}]}')
+
+    contained = {'/mnt/lvolume', '/mnt/lvolume/subdir'}
+    flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
+        'get_contained_directories'
+    ).never()
+
+    with pytest.raises(ValueError):
+        module.get_logical_volumes(
+            'lsblk', source_directories=('/mnt/lvolume', '/mnt/lvolume/subdir')
+        )
+
+
+def test_snapshot_logical_volume_with_percentage_snapshot_name_uses_lvcreate_extents_flag():
+    flexmock(module.borgmatic.execute).should_receive('execute_command').with_args(
+        (
+            'lvcreate',
+            '--snapshot',
+            '--extents',
+            '10%ORIGIN',
+            '--name',
+            'snap',
+            '/dev/snap',
+        ),
+        output_log_level=object,
+    )
+
+    module.snapshot_logical_volume('lvcreate', 'snap', '/dev/snap', '10%ORIGIN')
+
+
+def test_snapshot_logical_volume_with_non_percentage_snapshot_name_uses_lvcreate_size_flag():
+    flexmock(module.borgmatic.execute).should_receive('execute_command').with_args(
+        (
+            'lvcreate',
+            '--snapshot',
+            '--size',
+            '10TB',
+            '--name',
+            'snap',
+            '/dev/snap',
+        ),
+        output_log_level=object,
+    )
+
+    module.snapshot_logical_volume('lvcreate', 'snap', '/dev/snap', '10TB')
+
+
+def test_dump_data_sources_snapshots_and_mounts_and_updates_source_directories():
+    config = {'lvm': {}}
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume1_borgmatic-1234', '/dev/lvolume1', module.DEFAULT_SNAPSHOT_SIZE
+    ).once()
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume2_borgmatic-1234', '/dev/lvolume2', module.DEFAULT_SNAPSHOT_SIZE
+    ).once()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume1_borgmatic-1234', device_path='/dev/lvolume1_snap'),)
+    )
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume2_borgmatic-1234', device_path='/dev/lvolume2_snap'),)
+    )
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume1_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).once()
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume2_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).once()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == [
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume1/subdir',
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume2',
+    ]
+
+
+def test_dump_data_sources_with_no_logical_volumes_skips_snapshots():
+    config = {'lvm': {}}
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(())
+    flexmock(module).should_receive('snapshot_logical_volume').never()
+    flexmock(module).should_receive('mount_snapshot').never()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+
+
+def test_dump_data_sources_uses_snapshot_size_for_snapshot():
+    config = {'lvm': {'snapshot_size': '1000PB'}}
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate',
+        'lvolume1_borgmatic-1234',
+        '/dev/lvolume1',
+        '1000PB',
+    ).once()
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate',
+        'lvolume2_borgmatic-1234',
+        '/dev/lvolume2',
+        '1000PB',
+    ).once()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume1_borgmatic-1234', device_path='/dev/lvolume1_snap'),)
+    )
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume2_borgmatic-1234', device_path='/dev/lvolume2_snap'),)
+    )
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume1_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).once()
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume2_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).once()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == [
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume1/subdir',
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume2',
+    ]
+
+
+def test_dump_data_sources_uses_custom_commands():
+    config = {
+        'lvm': {
+            'lsblk_command': '/usr/local/bin/lsblk',
+            'lvcreate_command': '/usr/local/bin/lvcreate',
+            'lvs_command': '/usr/local/bin/lvs',
+            'mount_command': '/usr/local/bin/mount',
+        },
+    }
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        '/usr/local/bin/lvcreate',
+        'lvolume1_borgmatic-1234',
+        '/dev/lvolume1',
+        module.DEFAULT_SNAPSHOT_SIZE,
+    ).once()
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        '/usr/local/bin/lvcreate',
+        'lvolume2_borgmatic-1234',
+        '/dev/lvolume2',
+        module.DEFAULT_SNAPSHOT_SIZE,
+    ).once()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        '/usr/local/bin/lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume1_borgmatic-1234', device_path='/dev/lvolume1_snap'),)
+    )
+    flexmock(module).should_receive('get_snapshots').with_args(
+        '/usr/local/bin/lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume2_borgmatic-1234', device_path='/dev/lvolume2_snap'),)
+    )
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        '/usr/local/bin/mount', '/dev/lvolume1_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).once()
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        '/usr/local/bin/mount', '/dev/lvolume2_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).once()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == [
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume1/subdir',
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume2',
+    ]
+
+
+def test_dump_data_sources_with_dry_run_skips_snapshots_and_does_not_touch_source_directories():
+    config = {'lvm': {}}
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').never()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume1_borgmatic-1234', device_path='/dev/lvolume1_snap'),)
+    )
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume2_borgmatic-1234', device_path='/dev/lvolume2_snap'),)
+    )
+    flexmock(module).should_receive('mount_snapshot').never()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=True,
+        )
+        == []
+    )
+
+    assert source_directories == [
+        '/mnt/lvolume1/subdir',
+        '/mnt/lvolume2',
+    ]
+
+
+def test_dump_data_sources_ignores_mismatch_between_source_directories_and_contained_source_directories():
+    config = {'lvm': {}}
+    source_directories = ['/hmm']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume1_borgmatic-1234', '/dev/lvolume1', module.DEFAULT_SNAPSHOT_SIZE
+    ).once()
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume2_borgmatic-1234', '/dev/lvolume2', module.DEFAULT_SNAPSHOT_SIZE
+    ).once()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume1_borgmatic-1234', device_path='/dev/lvolume1_snap'),)
+    )
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).and_return(
+        (module.Snapshot(name='lvolume2_borgmatic-1234', device_path='/dev/lvolume2_snap'),)
+    )
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume1_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).once()
+    flexmock(module).should_receive('mount_snapshot').with_args(
+        'mount', '/dev/lvolume2_snap', '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).once()
+
+    assert (
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+        == []
+    )
+
+    assert source_directories == [
+        '/hmm',
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume1/subdir',
+        '/run/borgmatic/lvm_snapshots/./mnt/lvolume2',
+    ]
+
+
+def test_dump_data_sources_with_missing_snapshot_errors():
+    config = {'lvm': {}}
+    source_directories = ['/mnt/lvolume1/subdir', '/mnt/lvolume2']
+    flexmock(module).should_receive('get_logical_volumes').and_return(
+        (
+            module.Logical_volume(
+                name='lvolume1',
+                device_path='/dev/lvolume1',
+                mount_point='/mnt/lvolume1',
+                contained_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/mnt/lvolume2',),
+            ),
+        )
+    )
+    flexmock(module.os).should_receive('getpid').and_return(1234)
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume1_borgmatic-1234', '/dev/lvolume1', module.DEFAULT_SNAPSHOT_SIZE
+    ).once()
+    flexmock(module).should_receive('snapshot_logical_volume').with_args(
+        'lvcreate', 'lvolume2_borgmatic-1234', '/dev/lvolume2', module.DEFAULT_SNAPSHOT_SIZE
+    ).never()
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume1_borgmatic-1234'
+    ).and_return(())
+    flexmock(module).should_receive('get_snapshots').with_args(
+        'lvs', snapshot_name='lvolume2_borgmatic-1234'
+    ).never()
+    flexmock(module).should_receive('mount_snapshot').never()
+
+    with pytest.raises(ValueError):
+        module.dump_data_sources(
+            hook_config=config['lvm'],
+            config=config,
+            log_prefix='test',
+            config_paths=('test.yaml',),
+            borgmatic_runtime_directory='/run/borgmatic',
+            source_directories=source_directories,
+            dry_run=False,
+        )
+
+
+def test_get_snapshots_lists_all_snapshots():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return(
+        '''
+          {
+              "report": [
+                  {
+                      "lv": [
+                          {"lv_name": "snap1", "lv_path": "/dev/snap1"},
+                          {"lv_name": "snap2", "lv_path": "/dev/snap2"}
+                      ]
+                  }
+              ],
+              "log": [
+              ]
+          }
+        '''
+    )
+
+    assert module.get_snapshots('lvs') == (
+        module.Snapshot('snap1', '/dev/snap1'),
+        module.Snapshot('snap2', '/dev/snap2'),
+    )
+
+
+def test_get_snapshots_with_snapshot_name_lists_just_that_snapshot():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return(
+        '''
+          {
+              "report": [
+                  {
+                      "lv": [
+                          {"lv_name": "snap1", "lv_path": "/dev/snap1"},
+                          {"lv_name": "snap2", "lv_path": "/dev/snap2"}
+                      ]
+                  }
+              ],
+              "log": [
+              ]
+          }
+        '''
+    )
+
+    assert module.get_snapshots('lvs', snapshot_name='snap2') == (
+        module.Snapshot('snap2', '/dev/snap2'),
+    )
+
+
+def test_get_snapshots_with_invalid_lvs_json_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return('{')
+
+    with pytest.raises(ValueError):
+        assert module.get_snapshots('lvs')
+
+
+def test_get_snapshots_with_lvs_json_missing_report_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return(
+        '''
+          {
+              "report": [],
+              "log": [
+              ]
+          }
+        '''
+    )
+
+    with pytest.raises(ValueError):
+        assert module.get_snapshots('lvs')
+
+
+def test_get_snapshots_with_lvs_json_missing_keys_errors():
+    flexmock(module.borgmatic.execute).should_receive(
+        'execute_command_and_capture_output'
+    ).and_return(
+        '''
+          {
+              "report": [
+                  {
+                      "lv": [
+                          {}
+                      ]
+                  }
+              ],
+              "log": [
+              ]
+          }
+        '''
+    )
+
+    with pytest.raises(ValueError):
+        assert module.get_snapshots('lvs')
diff --git a/tests/unit/hooks/data_source/test_zfs.py b/tests/unit/hooks/data_source/test_zfs.py
index ab1daa6e..afd0270e 100644
--- a/tests/unit/hooks/data_source/test_zfs.py
+++ b/tests/unit/hooks/data_source/test_zfs.py
@@ -121,7 +121,7 @@ def test_dump_data_sources_snapshots_and_mounts_and_updates_source_directories()
     assert source_directories == [os.path.join(snapshot_mount_path, 'subdir')]
 
 
-def test_dump_data_sources_snapshots_with_no_datasets_skips_snapshots():
+def test_dump_data_sources_with_no_datasets_skips_snapshots():
     flexmock(module).should_receive('get_datasets_to_backup').and_return(())
     flexmock(module.os).should_receive('getpid').and_return(1234)
     flexmock(module).should_receive('snapshot_dataset').never()

From eb977080921970a0d936bf010519cab02c4841ec Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 16:02:33 -0800
Subject: [PATCH 31/33] Completed tests for LVM (#80).

---
 borgmatic/hooks/data_source/lvm.py       |  31 +-
 tests/unit/hooks/data_source/test_lvm.py | 474 +++++++++++++++++++++++
 2 files changed, 495 insertions(+), 10 deletions(-)

diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 6a5d0759..329d252a 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -337,15 +337,17 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
                 f'{log_prefix}: Unmounting LVM snapshot at {snapshot_mount_path}{dry_run_label}'
             )
 
-            if not dry_run:
-                try:
-                    unmount_snapshot(umount_command, snapshot_mount_path)
-                except FileNotFoundError:
-                    logger.debug(f'{log_prefix}: Could not find "{umount_command}" command')
-                    return
-                except subprocess.CalledProcessError as error:
-                    logger.debug(f'{log_prefix}: {error}')
-                    return
+            if dry_run:
+                continue
+
+            try:
+                unmount_snapshot(umount_command, snapshot_mount_path)
+            except FileNotFoundError:
+                logger.debug(f'{log_prefix}: Could not find "{umount_command}" command')
+                return
+            except subprocess.CalledProcessError as error:
+                logger.debug(f'{log_prefix}: {error}')
+                return
 
         if not dry_run:
             shutil.rmtree(snapshots_directory)
@@ -353,7 +355,16 @@ def remove_data_source_dumps(hook_config, config, log_prefix, borgmatic_runtime_
     # Delete snapshots.
     lvremove_command = hook_config.get('lvremove_command', 'lvremove')
 
-    for snapshot in get_snapshots(hook_config.get('lvs_command', 'lvs')):
+    try:
+        snapshots = get_snapshots(hook_config.get('lvs_command', 'lvs'))
+    except FileNotFoundError as error:
+        logger.debug(f'{log_prefix}: Could not find "{error.filename}" command')
+        return
+    except subprocess.CalledProcessError as error:
+        logger.debug(f'{log_prefix}: {error}')
+        return
+
+    for snapshot in snapshots:
         # Only delete snapshots that borgmatic actually created!
         if not snapshot.name.split('_')[-1].startswith(BORGMATIC_SNAPSHOT_PREFIX):
             continue
diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py
index 7dadbe36..1f70087a 100644
--- a/tests/unit/hooks/data_source/test_lvm.py
+++ b/tests/unit/hooks/data_source/test_lvm.py
@@ -615,3 +615,477 @@ def test_get_snapshots_with_lvs_json_missing_keys_errors():
 
     with pytest.raises(ValueError):
         assert module.get_snapshots('lvs')
+
+
+def test_remove_data_source_dumps_unmounts_and_remove_snapshots():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).once()
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/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'),
+            module.Snapshot('nonborgmatic', '/dev/nonborgmatic'),
+        ),
+    )
+    flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume1').once()
+    flexmock(module).should_receive('remove_snapshot').with_args('lvremove', '/dev/lvolume2').once()
+    flexmock(module).should_receive('remove_snapshot').with_args(
+        'nonborgmatic', '/dev/nonborgmatic'
+    ).never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        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)
+    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=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_bails_for_lsblk_command_error():
+    config = {'lvm': {}}
+    flexmock(module).should_receive('get_logical_volumes').and_raise(
+        module.subprocess.CalledProcessError(1, 'wtf')
+    )
+    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=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_with_missing_snapshot_directory_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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots'
+    ).and_return(False)
+    flexmock(module.shutil).should_receive('rmtree').never()
+    flexmock(module).should_receive('unmount_snapshot').never()
+    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,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_with_missing_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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots'
+    ).and_return(True)
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).and_return(False)
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).never()
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/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,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_with_successful_mount_point_removal_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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots'
+    ).and_return(True)
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1'
+    ).and_return(True).and_return(False)
+    flexmock(module.os.path).should_receive('isdir').with_args(
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2'
+    ).and_return(True).and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).never()
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/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,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_bails_for_missing_umount_command():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).and_raise(FileNotFoundError)
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2',
+    ).never()
+    flexmock(module).should_receive('get_snapshots').never()
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_bails_for_umount_command_error():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).and_raise(module.subprocess.CalledProcessError(1, 'wtf'))
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2',
+    ).never()
+    flexmock(module).should_receive('get_snapshots').never()
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_bails_for_missing_lvs_command():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).once()
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2',
+    ).once()
+    flexmock(module).should_receive('get_snapshots').and_raise(FileNotFoundError)
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_dumps_bails_for_lvs_command_error():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree')
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume1',
+    ).once()
+    flexmock(module).should_receive('unmount_snapshot').with_args(
+        'umount',
+        '/run/borgmatic/lvm_snapshots/mnt/lvolume2',
+    ).once()
+    flexmock(module).should_receive('get_snapshots').and_raise(
+        module.subprocess.CalledProcessError(1, 'wtf')
+    )
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=False,
+    )
+
+
+def test_remove_data_source_with_dry_run_skips_snapshot_unmount_and_delete():
+    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_source_directories=('/mnt/lvolume1/subdir',),
+            ),
+            module.Logical_volume(
+                name='lvolume2',
+                device_path='/dev/lvolume2',
+                mount_point='/mnt/lvolume2',
+                contained_source_directories=('/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])
+    flexmock(module.os.path).should_receive('isdir').and_return(True)
+    flexmock(module.shutil).should_receive('rmtree').never()
+    flexmock(module).should_receive('unmount_snapshot').never()
+    flexmock(module).should_receive('get_snapshots').and_return(
+        (
+            module.Snapshot('lvolume1_borgmatic-1234', '/dev/lvolume1'),
+            module.Snapshot('lvolume2_borgmatic-1234', '/dev/lvolume2'),
+            module.Snapshot('nonborgmatic', '/dev/nonborgmatic'),
+        ),
+    ).once()
+    flexmock(module).should_receive('remove_snapshot').never()
+
+    module.remove_data_source_dumps(
+        hook_config=config['lvm'],
+        config=config,
+        log_prefix='test',
+        borgmatic_runtime_directory='/run/borgmatic',
+        dry_run=True,
+    )

From 8979f8918d3262c9f50ddfe91c98e3e4e8bd9d6e Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 16:05:46 -0800
Subject: [PATCH 32/33] Organize imports (#80).

---
 borgmatic/hooks/data_source/btrfs.py     | 2 +-
 borgmatic/hooks/data_source/lvm.py       | 2 +-
 borgmatic/hooks/data_source/snapshot.py  | 1 -
 borgmatic/hooks/data_source/zfs.py       | 2 +-
 tests/unit/hooks/data_source/test_lvm.py | 2 +-
 5 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/borgmatic/hooks/data_source/btrfs.py b/borgmatic/hooks/data_source/btrfs.py
index 986f25dd..4ddc2ae8 100644
--- a/borgmatic/hooks/data_source/btrfs.py
+++ b/borgmatic/hooks/data_source/btrfs.py
@@ -6,8 +6,8 @@ import shutil
 import subprocess
 
 import borgmatic.config.paths
-import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
+import borgmatic.hooks.data_source.snapshot
 
 logger = logging.getLogger(__name__)
 
diff --git a/borgmatic/hooks/data_source/lvm.py b/borgmatic/hooks/data_source/lvm.py
index 329d252a..ef5a2dca 100644
--- a/borgmatic/hooks/data_source/lvm.py
+++ b/borgmatic/hooks/data_source/lvm.py
@@ -7,8 +7,8 @@ import shutil
 import subprocess
 
 import borgmatic.config.paths
-import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
+import borgmatic.hooks.data_source.snapshot
 
 logger = logging.getLogger(__name__)
 
diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index 9567d226..7b5e4503 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -1,7 +1,6 @@
 import itertools
 import pathlib
 
-
 IS_A_HOOK = False
 
 
diff --git a/borgmatic/hooks/data_source/zfs.py b/borgmatic/hooks/data_source/zfs.py
index 5903e9a4..da35a256 100644
--- a/borgmatic/hooks/data_source/zfs.py
+++ b/borgmatic/hooks/data_source/zfs.py
@@ -6,8 +6,8 @@ import shutil
 import subprocess
 
 import borgmatic.config.paths
-import borgmatic.hooks.data_source.snapshot
 import borgmatic.execute
+import borgmatic.hooks.data_source.snapshot
 
 logger = logging.getLogger(__name__)
 
diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py
index 1f70087a..250fa535 100644
--- a/tests/unit/hooks/data_source/test_lvm.py
+++ b/tests/unit/hooks/data_source/test_lvm.py
@@ -1,5 +1,5 @@
-from flexmock import flexmock
 import pytest
+from flexmock import flexmock
 
 from borgmatic.hooks.data_source import lvm as module
 

From ae8a9db27d1f4029df832bcdb675936117514563 Mon Sep 17 00:00:00 2001
From: Dan Helfman <witten@torsion.org>
Date: Fri, 6 Dec 2024 16:12:01 -0800
Subject: [PATCH 33/33] Fix flake issues (#80).

---
 borgmatic/hooks/data_source/snapshot.py   | 1 -
 tests/end-to-end/commands/fake_findmnt.py | 1 -
 tests/end-to-end/test_btrfs.py            | 2 +-
 tests/end-to-end/test_zfs.py              | 2 +-
 tests/unit/hooks/data_source/test_lvm.py  | 2 --
 5 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/borgmatic/hooks/data_source/snapshot.py b/borgmatic/hooks/data_source/snapshot.py
index 7b5e4503..58af9e37 100644
--- a/borgmatic/hooks/data_source/snapshot.py
+++ b/borgmatic/hooks/data_source/snapshot.py
@@ -1,4 +1,3 @@
-import itertools
 import pathlib
 
 IS_A_HOOK = False
diff --git a/tests/end-to-end/commands/fake_findmnt.py b/tests/end-to-end/commands/fake_findmnt.py
index ddc6c787..d7393bb3 100644
--- a/tests/end-to-end/commands/fake_findmnt.py
+++ b/tests/end-to-end/commands/fake_findmnt.py
@@ -1,5 +1,4 @@
 import argparse
-import json
 import sys
 
 
diff --git a/tests/end-to-end/test_btrfs.py b/tests/end-to-end/test_btrfs.py
index 2c410774..2df6228b 100644
--- a/tests/end-to-end/test_btrfs.py
+++ b/tests/end-to-end/test_btrfs.py
@@ -55,7 +55,7 @@ def test_btrfs_create_and_list():
 
         # Assert that the snapshot has been deleted.
         assert not subprocess.check_output(
-            f'python3 /app/tests/end-to-end/commands/fake_btrfs.py subvolume list -s /mnt/subvolume'.split(
+            'python3 /app/tests/end-to-end/commands/fake_btrfs.py subvolume list -s /mnt/subvolume'.split(
                 ' '
             )
         )
diff --git a/tests/end-to-end/test_zfs.py b/tests/end-to-end/test_zfs.py
index 7c2cd7f6..9a338ebb 100644
--- a/tests/end-to-end/test_zfs.py
+++ b/tests/end-to-end/test_zfs.py
@@ -56,7 +56,7 @@ def test_zfs_create_and_list():
 
         # Assert that the snapshot has been deleted.
         assert not subprocess.check_output(
-            f'python3 /app/tests/end-to-end/commands/fake_zfs.py list -H -t snapshot'.split(' ')
+            'python3 /app/tests/end-to-end/commands/fake_zfs.py list -H -t snapshot'.split(' ')
         )
     finally:
         shutil.rmtree(temporary_directory)
diff --git a/tests/unit/hooks/data_source/test_lvm.py b/tests/unit/hooks/data_source/test_lvm.py
index 250fa535..d5fe1ed2 100644
--- a/tests/unit/hooks/data_source/test_lvm.py
+++ b/tests/unit/hooks/data_source/test_lvm.py
@@ -67,7 +67,6 @@ def test_get_logical_volumes_with_invalid_lsblk_json_errors():
         'execute_command_and_capture_output'
     ).and_return('{')
 
-    contained = {'/mnt/lvolume', '/mnt/lvolume/subdir'}
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_directories'
     ).never()
@@ -83,7 +82,6 @@ def test_get_logical_volumes_with_lsblk_json_missing_keys_errors():
         'execute_command_and_capture_output'
     ).and_return('{"block_devices": [{}]}')
 
-    contained = {'/mnt/lvolume', '/mnt/lvolume/subdir'}
     flexmock(module.borgmatic.hooks.data_source.snapshot).should_receive(
         'get_contained_directories'
     ).never()