From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 621DA1FF0AF for ; Fri, 06 Nov 2026 18:24:32 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 50A7521713; Fri, 06 Nov 2026 18:23:39 +0100 (CET) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v2 36/43] group: destroy: s3: skip snapshots with active retentions Date: Fri, 6 Nov 2026 18:22:28 +0100 Message-ID: <20261106172235.890542-37-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261106172235.890542-1-c.ebner@proxmox.com> References: <20261106172235.890542-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793985780418 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.449 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: GEU3QQ3VCF5RQQPYH5K4CZDWLG56LWLX X-Message-ID-Hash: GEU3QQ3VCF5RQQPYH5K4CZDWLG56LWLX X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Make sure that snapshots which have an active retention are not removed prematurely from the S3 backend when doing a group cleanup. The group cleanup on S3 backed datastores is optimized to reduce the number of API calls, protected snapshots can be filtered out from the to-be deleted object based on the presence of the protected file/object marker. This cannot be done in the same way for snapshots with retention, as this information is stored on the manifest itself instead. Therefore, first check if there are retained snapshots, the group lock guarding against changes to snapshot manifests, and only if none present, use the optimized cleanup. If there are however retained snapshots, fallback to do a per-snapshot cleanup instead. Signed-off-by: Christian Ebner --- pbs-datastore/src/backup_info.rs | 59 ++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index 13dd52c3a..bd102a233 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -236,18 +236,48 @@ impl BackupGroup { log::info!("removing backup group {:?}", path); let mut delete_stats = BackupGroupDeleteStats::default(); + + let mut to_remove = Vec::new(); + let mut has_retained = false; for snap in self.iter_snapshots()? { let snap = snap?; if snap.is_protected() { delete_stats.increment_protected_snapshots(); continue; } - // also for S3 cleanup local only, the actual S3 objects will be removed below, - // reducing the number of required API calls. - snap.destroy(false, &DatastoreBackend::Filesystem)?; + if let Some(retain_until) = snap.active_retention_lock()? { + info!( + "skip removal of snapshot {:?}, retained until {}", + snap.relative_path(), + proxmox_time::epoch_to_rfc3339(retain_until)?, + ); + // accounted as protected, which also keeps the group itself + delete_stats.increment_protected_snapshots(); + has_retained = true; + continue; + } + to_remove.push(snap); + } + + // Use optimized cleanup for S3 when there are no retained snapshots. + // In that case, groups can be cleaned up by API calls with prefix/suffix + // filtering since the protected markers are present as objects. + let snapshot_backend = if has_retained { + backend + } else { + &DatastoreBackend::Filesystem + }; + + for snapshot in to_remove { + snapshot.destroy(false, snapshot_backend)?; delete_stats.increment_removed_snapshots(); } + if has_retained { + // if there are retained snapshots, the group itself cannot be cleaned up. + return Ok(delete_stats); + } + if let DatastoreBackend::S3(s3_client) = backend { let path = self.relative_group_path(); let group_prefix = path @@ -924,13 +954,11 @@ impl BackupDir { bail!("cannot remove protected snapshot"); // use special error type? } - if let Some((manifest, _size)) = self.load_manifest_optionally(false)? { - if let Some(retain_until) = manifest.active_retention_lock()? { - bail!( - "snapshot protected by retention until {}", - proxmox_time::epoch_to_rfc3339(retain_until)? - ); - } + if let Some(retain_until) = self.active_retention_lock()? { + bail!( + "snapshot protected by retention until {}", + proxmox_time::epoch_to_rfc3339(retain_until)? + ); } if let DatastoreBackend::S3(s3_client) = backend { @@ -1012,6 +1040,17 @@ impl BackupDir { crate::SnapshotReader::new_do(self.clone()) } + /// Returns the retention timestamp if the snapshot is still locked by it. + /// + /// Snapshots without manifest, e.g. of ongoing backups, are never locked. Errors if the + /// manifest or its retention timestamp cannot be parsed. + pub fn active_retention_lock(&self) -> Result, Error> { + match self.load_manifest_optionally(false)? { + Some((manifest, _size)) => manifest.active_retention_lock(), + None => Ok(None), + } + } + /// Load the manifest without a lock. Must not be written back. /// /// A missing manifest file for the snapshot is treated as error. -- 2.47.3