Merge pull request #27 from SomethingGeneric/copilot/fix-58c124cb-0c2e-45f7-9482-1d5949b7ddb5

Implement default snapshot retention limit of 5 snapshots to prevent unbounded growth

Author: SomethingGeneric

README.md

@@ -198,8 +198,9 @@ vmid = 200
 - Use `max_snapshots` to set a maximum number of automated snapshots per VM
 - When set, miniupdate will keep only the N newest snapshots and delete older ones
 - Useful for VMs on capacity-limited storage backends (e.g., small SSDs)
-- Takes precedence over the global `snapshot_retention_days` setting
-- If not set, the global time-based retention policy applies
+- Takes precedence over the default snapshot limit
+- **If not set, miniupdate defaults to keeping the 5 newest snapshots** to prevent unbounded growth
+- Time-based retention (`snapshot_retention_days`) provides additional cleanup after the count limit
 
 ### inventory.yml (Ansible Format)
 
@@ -363,7 +364,8 @@ The `update` command provides a complete automated update workflow with Proxmox
 - `ping_timeout`: How long to wait for host availability (default: 2 minutes)
 - `snapshot_name_prefix`: Prefix for automated snapshots
 - `cleanup_snapshots`: Remove old snapshots after successful updates
-- `snapshot_retention_days`: Keep snapshots for N days
+- `snapshot_retention_days`: Time-based cleanup for snapshots older than N days
+- **Default snapshot limit**: Keeps 5 newest snapshots per VM (override with `max_snapshots` in vm_mapping.toml)
 
 ## Email Reports
 

config.toml.example

@@ -36,5 +36,7 @@ ping_timeout = 120
 ping_interval = 5
 snapshot_name_prefix = "pre-update"
 cleanup_snapshots = true
-snapshot_retention_days = 7
-opt_out_hosts = []
+snapshot_retention_days = 7  # Time-based retention (applies after default snapshot count limit)
+opt_out_hosts = []  # Hosts to skip automated updates (check-only)
+# Note: By default, miniupdate keeps the 5 newest snapshots per VM to prevent unbounded growth
+# Override this per-VM using max_snapshots in vm_mapping.toml

miniupdate/update_automator.py

@@ -524,18 +524,26 @@ def _cleanup_old_snapshots(self, vm_mapping: VMMapping):
                     logger.info(f"Per-host snapshot quota: keeping {vm_mapping.max_snapshots} newest snapshots "
                                f"for VM {vm_mapping.vmid}, deleting {len(snapshots_to_delete)} older ones")
             else:
-                # Fall back to time-based retention policy
-                retention_days = self.update_config.get('snapshot_retention_days', 7)
-                cutoff_time = datetime.now() - timedelta(days=retention_days)
-                
-                snapshots_to_delete = [
-                    snap for snap in automated_snapshots 
-                    if snap['time'] < cutoff_time
-                ]
+                # Use default snapshot count limit to prevent unbounded growth
+                default_max_snapshots = 5
+                if len(automated_snapshots) > default_max_snapshots:
+                    snapshots_to_delete = automated_snapshots[default_max_snapshots:]
+                    logger.info(f"Default snapshot quota: keeping {default_max_snapshots} newest snapshots "
+                               f"for VM {vm_mapping.vmid}, deleting {len(snapshots_to_delete)} older ones")
 
-                if snapshots_to_delete:
-                    logger.info(f"Time-based retention: deleting {len(snapshots_to_delete)} snapshots "
-                               f"older than {retention_days} days for VM {vm_mapping.vmid}")
+                # Also apply time-based retention if configured (for additional cleanup)
+                if not snapshots_to_delete:
+                    retention_days = self.update_config.get('snapshot_retention_days', 7)
+                    cutoff_time = datetime.now() - timedelta(days=retention_days)
+                    
+                    snapshots_to_delete = [
+                        snap for snap in automated_snapshots 
+                        if snap['time'] < cutoff_time
+                    ]
+                    
+                    if snapshots_to_delete:
+                        logger.info(f"Time-based retention: deleting {len(snapshots_to_delete)} snapshots "
+                                   f"older than {retention_days} days for VM {vm_mapping.vmid}")
 
             # Delete the snapshots
             for snap in snapshots_to_delete: