Conversation
Implement a new function to enforce backup retention by deleting older backups on the SMB share when the number exceeds the specified limit. The maximum number of backups is now configurable via the SambaBackupCloudConfiguration class. This change ensures that only the most recent backups are retained, improving storage management. Additionally, integrate the retention enforcement into the upload_backup function, handling potential errors during the cleanup process gracefully.
Enhance the Nextcloud backup module by introducing functions to list existing backups and enforce retention policies. The new `_list_backups` function retrieves backups based on a naming prefix, while `_enforce_retention` ensures that only a specified number of recent backups are retained. The `NextcloudBackupCloudConfiguration` class is updated to include a `max_backups` parameter for configuration. These changes improve backup management and storage efficiency.
benderl
changed the title
benderl
changed the title
|
Samba und Nextcloud |
There was a problem hiding this comment.
Pull request overview
Adds a configurable retention limit for cloud backups so older backup generations can be cleaned up automatically (addresses #2020; UI change tracked in openwb-ui-settings#899).
Changes:
- Add
max_backupsto Samba and Nextcloud backup cloud configuration objects. - Implement post-upload retention enforcement for Samba (SMB) by listing/deleting older backups.
- Implement post-upload retention enforcement for Nextcloud via WebDAV PROPFIND + DELETE.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/modules/backup_clouds/samba/config.py | Adds max_backups configuration field for Samba backups. |
| packages/modules/backup_clouds/samba/backup_cloud.py | Adds SMB retention cleanup logic and invokes it after upload. |
| packages/modules/backup_clouds/nextcloud/config.py | Adds max_backups configuration field for Nextcloud backups. |
| packages/modules/backup_clouds/nextcloud/backup_cloud.py | Adds WebDAV-based retention cleanup logic and adjusts upload URL construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Präfix aus aktuellem Backup ableiten (alles vor dem ersten Punkt) | ||
| # Beispiel: openwb-backup-2026-02-10.tar.gz -> openwb-backup-2026-02-10 | ||
| base_name = backup_filename.split("/")[-1] | ||
| base_prefix = base_name.split(".")[0] | ||
|
|
There was a problem hiding this comment.
Die Retention-Filterlogik nutzt base_prefix = base_name.split(".")[0] und filtert anschließend mit startswith(base_prefix). Bei den realen Backup-Dateinamen (z.B. 2026-03-20_02-15-00_2.1.0.openwb-backup) ist der Teil vor dem ersten Punkt timestamp-/versionspezifisch, sodass praktisch nur das gerade hochgeladene Backup gematcht wird und alte Backups nie gelöscht werden. Bitte stattdessen nach einem stabilen Muster filtern (z.B. alle Dateien, die auf .openwb-backup bzw. .openwb-backup.gpg enden) und dann nach Dateiname sortieren/limitieren.
| # Hier bleiben wir beim bisherigen Verhalten (/public.php/webdav/<filename>), | ||
| # d.h. base_path ist leer und backup_filename enthält ggf. Unterordner. | ||
| base_path = "" |
There was a problem hiding this comment.
Im else-Zweig (Benutzer/Passwort konfiguriert) wird base_path = "" gesetzt, aber der Kommentar sagt, dass beim bisherigen Verhalten (/public.php/webdav/<filename>) geblieben werden soll. Mit base_path="" wird aktuell jedoch nach f"{upload_url}/{backup_filename}" hochgeladen bzw. per PROPFIND/DELETE auf upload_url + "/" gearbeitet, was für eine normale Nextcloud-Installation sehr wahrscheinlich falsche Endpunkte sind. Bitte entweder base_path auch hier auf /public.php/webdav setzen (wie vorher) oder den korrekten DAV-Pfad für User-Accounts (/remote.php/dav/files/<user>/...) sauber implementieren.
| # Hier bleiben wir beim bisherigen Verhalten (/public.php/webdav/<filename>), | |
| # d.h. base_path ist leer und backup_filename enthält ggf. Unterordner. | |
| base_path = "" | |
| # Hier bleiben wir jedoch beim bisherigen Verhalten und nutzen ebenfalls | |
| # den öffentlichen WebDAV-Pfad (/public.php/webdav/<filename>). | |
| base_path = "/public.php/webdav" |
| # Basispräfix der aktuellen Backup-Datei (z.B. "openwb-backup-2026-02-10" aus "openwb-backup-2026-02-10.tar.gz") | ||
| base_name = os.path.basename(backup_filename) | ||
| base_prefix = base_name.split(".")[0] | ||
|
|
||
| # Alle Einträge im Backup-Verzeichnis holen | ||
| entries = conn.listPath(config.smb_share, dir_path) | ||
|
|
||
| # Nur relevante Backup-Dateien herausfiltern | ||
| backup_files = [ | ||
| e for e in entries | ||
| if not e.isDirectory | ||
| and e.filename not in (".", "..") | ||
| and e.filename.startswith(base_prefix) | ||
| ] |
There was a problem hiding this comment.
Die Retention-Filterlogik nutzt base_prefix = base_name.split(".")[0] und filtert anschließend mit startswith(base_prefix). Die von runs/backup.sh generierten Dateinamen enthalten vor der Endung bereits weitere Punkte (Version), z.B. 2026-03-20_02-15-00_2.1.0.openwb-backup. Dadurch ist base_prefix timestamp-/versionspezifisch und matcht praktisch nur das aktuelle Backup – alte Dateien werden nicht gefunden und folglich nie gelöscht. Bitte stattdessen nach einem stabilen Muster filtern (z.B. Endung .openwb-backup/.openwb-backup.gpg) und dann das Limit anwenden.
| # Nach Änderungszeit sortieren (neueste zuerst) | ||
| backup_files.sort(key=lambda e: e.last_write_time, reverse=True) | ||
|
|
There was a problem hiding this comment.
Im Issue #2020 wird gefordert, die zu löschenden Backups nach Dateiname zu sortieren (weil der Dateiname die zeitliche Reihenfolge abbildet). Hier wird stattdessen nach last_write_time sortiert. Das kann abweichen (z.B. wenn Dateien kopiert/verschoben wurden oder der Server-Timestamp unzuverlässig ist) und führt dann zu unerwarteten Löschungen. Bitte auf Sortierung nach Dateiname umstellen (oder klar begründen/konfigurierbar machen), damit das Verhalten dem Feature-Request entspricht.
|
Bitte die Kommentare vom Copilot-Review beachten. |
Update the backup retention mechanisms in both Nextcloud and Samba modules to use suffix-based filtering for backup filenames. The changes ensure that only files ending with ".openwb-backup" or ".openwb-backup.gpg" are considered for retention, improving accuracy in backup management. Additionally, comments have been enhanced for clarity regarding the implementation details and expected filename patterns.
Issue: #2020
UI: openWB/openwb-ui-settings#899