backup limiter should remove the last chain when reached

tests/test_job.py

@@ -314,63 +314,83 @@ def test_limit_backups(self):
         self.job._limit_backups()
 
         # Then
-        # Only the oldest incremental backup should be removed.
+        # Only one full should remain as we deleted the oldest tree
         backups_full_new = self.job._backup_db.get_backups(backup_type='full')
-        self.assertEqual(backups_full, backups_full_new)
+        self.assertEqual(backups_full[1:], backups_full_new)
 
         backups_inc_new = self.job._backup_db.get_backups(backup_type='inc')
         self.assertEqual(backups_inc[1:], backups_inc_new)
 
         # When
-        self.job._max_backups = 2
+        self.job._max_backups = 1
         self.job._limit_backups()
+        # This config would force create a new full and remove the old chain
 
         # Then
-        # The oldest full backup should be removed.
         backups_full_new = self.job._backup_db.get_backups(backup_type='full')
-        self.assertEqual(backups_full[1:], backups_full_new)
+        self.assertEqual(1, len(backups_full_new))
 
+        # No inc should remain
         backups_inc_new = self.job._backup_db.get_backups(backup_type='inc')
-        self.assertEqual(backups_inc[1:], backups_inc_new)
+        self.assertEqual(0, len(backups_inc_new))
 
-    def test_limit_backups_one_full(self):
-        """ Test the backup limiter when there's only one full backup. """
+
+    def test_limit_backups_remove_last_chain(self):
+        """ Test the backup limiter with 3 chains, deleting the oldest """
 
         # Given
-        for _ in range(3):
+        self.job._max_incremental_backups_per_full = 1
+
+        for _ in range(6):
             self.job.start()
 
         backups_full = self.job._backup_db.get_backups(backup_type='full')
-        self.assertEqual(1, len(backups_full))
+        self.assertEqual(3, len(backups_full))
 
         backups_inc = self.job._backup_db.get_backups(backup_type='inc')
-        self.assertEqual(2, len(backups_inc))
+        self.assertEqual(3, len(backups_inc))
 
         # When
-        self.job._max_backups = 2
+        self.job._max_backups = 5
         self.job._limit_backups()
 
         # Then
-        # Only the oldest incremental backup should be removed.
         backups_full_new = self.job._backup_db.get_backups(backup_type='full')
-        self.assertEqual(backups_full, backups_full_new)
+        self.assertEqual(backups_full[1:], backups_full_new)
 
         backups_inc_new = self.job._backup_db.get_backups(backup_type='inc')
-        # Can only nuke the last backup as the others have dependants
-        self.assertEqual(backups_inc[:1], backups_inc_new)
+        self.assertEqual(backups_inc[1:], backups_inc_new)
+
+
+
+    def test_limit_backups_one_full(self):
+        """ Test the backup limiter when there's only one full backup. """
+
+        # Given
+        for _ in range(3):
+            self.job.start()
+
+        backups_full = self.job._backup_db.get_backups(backup_type='full')
+        self.assertEqual(1, len(backups_full))
+
+        backups_inc = self.job._backup_db.get_backups(backup_type='inc')
+        self.assertEqual(2, len(backups_inc))
 
         # When
-        self.job._max_backups = 1
+        self.job._max_backups = 2
         self.job._limit_backups()
 
         # Then
-        # Only the full backup should remain.
+        # Since there is only one full and we reached the limit, 
+        # a new full is created and the old chain deleted
         backups_full_new = self.job._backup_db.get_backups(backup_type='full')
-        self.assertEqual(backups_full, backups_full_new)
+        self.assertEqual(1, len(backups_full_new))
 
         backups_inc_new = self.job._backup_db.get_backups(backup_type='inc')
+        # No incremental remains
         self.assertEqual(0, len(backups_inc_new))
 
+
     def test_limit_backups_all_full(self):
         """ Test the backup limiter when it's only full backups. """
 

zfs_uploader/job.py

@@ -560,23 +560,26 @@ def _limit_backups(self):
             self._logger.info(f'filesystem={self._filesystem} '
                               'msg="Backup limit achieved."')
 
-        deleted_count = 0
-        for backup in backups:
-            backup_time = backup.backup_time
-            s3_key = backup.s3_key
-
-            dependants = any([True if b.dependency == backup_time
-                              else False for b in backups])
-            if dependants:
-                self._logger.info(f's3_key={s3_key} '
-                                  'msg="Backup has dependants. Not '
-                                  'deleting."')
-            else:
-                self._delete_backup(backup)
-                deleted_count += 1
+            # Check how many full backups we have. If only one, we
+            # need to force a new one before deleting the oldest chain
+            full_count = sum(1 for b in backups if b.backup_type == 'full')
+            if full_count <= 1:
+                self._backup_full()
 
-            if len(backups) - deleted_count == self._max_backups:
-                break
+            # Re fetch the backups to make sure we get them all
+            backups = self._backup_db.get_backups()
+
+            # Backups will have the most recent last, so we just need
+            # to drop them in order until we reach the second full
+            delete_list = []
+            for i, backup in enumerate(backups):
+                if i != 0 and backup.backup_type == 'full':
+                    break
+                delete_list.append(backup)
+            delete_list.reverse()
+
+            for backup in delete_list:
+                self._delete_backup(backup)
 
 
 class TransferCallback: