From 21b3cb588e46e1263e693a923c56c3c9d7c6c303 Mon Sep 17 00:00:00 2001 From: "Dr. Matthias Ratajczak" Date: Wed, 4 May 2022 11:26:39 +0200 Subject: [PATCH] fix overzealous snapshot removal bug --- test_zfs_backup.py | 15 ++++++++++++++- zfs_backup.py | 16 ++++++++++------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/test_zfs_backup.py b/test_zfs_backup.py index d075998..f7ed358 100644 --- a/test_zfs_backup.py +++ b/test_zfs_backup.py @@ -207,7 +207,20 @@ class TestDataset: counter = 0 ds._clean_old_snapshots_for_interval("daily", MAX_COUNT) - assert counter == MAX_COUNT + assert counter <= MAX_COUNT + + @pytest.mark.parametrize("max_count", [10, 40]) + def test__get_oldest_snapshots_exceeding_max_count(self, max_count): + snapshots = [ + snap for snap in gen_snapshots_for_tank_ROOT() if "daily" in str(snap) + ] + ORIGINAL_COUNT = 31 + assert len(snapshots) == ORIGINAL_COUNT + expected_result_len = ORIGINAL_COUNT - max_count + if expected_result_len < 0: + expected_result_len = 0 + result = Dataset._get_oldest_snapshots_exceeding_max_count(snapshots, max_count) + assert len(result) == expected_result_len def gen_snapshots_for_tank_ROOT() -> list[Snapshot]: diff --git a/zfs_backup.py b/zfs_backup.py index 8554e9f..35ace08 100755 --- a/zfs_backup.py +++ b/zfs_backup.py @@ -205,6 +205,7 @@ class ZFSPath: """Return the path in the same manner as `zfs`.""" path = "/".join(self._elements) if self._is_snapshot: + assert self._snapshot_name is not None return "@".join([path, self._snapshot_name]) return path @@ -722,22 +723,25 @@ class Dataset: for interval, number in config.keep_snapshots_per_interval.items(): self._clean_old_snapshots_for_interval(interval, number) - def _clean_old_snapshots_for_interval( - self, interval: str, max_count: int - ) -> None: # TODO I assume the regex matches across different datasets, which it shouldn't + def _clean_old_snapshots_for_interval(self, interval: str, max_count: int) -> None: regex = _get_regex_matching_snapshots_with_tags([config.snapshot_tag, interval]) snapshots = [ snapshot for snapshot in self.snapshots if snapshot.matches_regex(regex) ] - old_snapshots = self._get_n_oldest_snapshots(snapshots, max_count) + old_snapshots = self._get_oldest_snapshots_exceeding_max_count( + snapshots, max_count + ) for snapshot in old_snapshots: snapshot.destroy() @staticmethod - def _get_n_oldest_snapshots( - snapshots: list[Snapshot], count: int + def _get_oldest_snapshots_exceeding_max_count( + snapshots: list[Snapshot], max_count: int ) -> list[Snapshot]: snapshots.sort() + count = len(snapshots) - max_count + if count < 0: + count = 0 return snapshots[:count]