From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 44EE41FF16F for ; Tue, 22 Jul 2025 12:15:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 56BB5364C9; Tue, 22 Jul 2025 12:17:05 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Tue, 22 Jul 2025 12:10:40 +0200 Message-ID: <20250722101106.526438-25-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.2 In-Reply-To: <20250722101106.526438-1-c.ebner@proxmox.com> References: <20250722101106.526438-1-c.ebner@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753179083370 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.045 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pbs-devel] [PATCH proxmox-backup v11 20/46] datastore: prune groups/snapshots from s3 object store backend X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" When pruning a backup group or a backup snapshot for a datastore with S3 object store backend, remove the associated objects by removing them based on the prefix. In order to exclude protected contents, add a filtering based on the presence of the protected marker. Signed-off-by: Christian Ebner Reviewed-by: Lukas Wagner Reviewed-by: Hannes Laimer --- changes since version 10: - no changes pbs-datastore/src/backup_info.rs | 49 ++++++++++++++++++++++++++++++-- pbs-datastore/src/datastore.rs | 46 ++++++++++++++++++++++++++---- src/api2/admin/datastore.rs | 24 ++++++++++------ 3 files changed, 102 insertions(+), 17 deletions(-) diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs index 26f03a0ae..4b10b6435 100644 --- a/pbs-datastore/src/backup_info.rs +++ b/pbs-datastore/src/backup_info.rs @@ -9,6 +9,7 @@ use std::time::Duration; use anyhow::{bail, format_err, Context, Error}; use const_format::concatcp; +use proxmox_s3_client::S3PathPrefix; use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions}; use proxmox_systemd::escape_unit; @@ -18,7 +19,9 @@ use pbs_api_types::{ }; use pbs_config::{open_backup_lockfile, BackupLockGuard}; +use crate::datastore::{GROUP_NOTES_FILE_NAME, GROUP_OWNER_FILE_NAME}; use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME}; +use crate::s3::S3_CONTENT_PREFIX; use crate::{DataBlob, DataStore, DatastoreBackend}; pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks"; @@ -218,7 +221,7 @@ impl BackupGroup { /// /// Returns `BackupGroupDeleteStats`, containing the number of deleted snapshots /// and number of protected snaphsots, which therefore were not removed. - pub fn destroy(&self) -> Result { + pub fn destroy(&self, backend: &DatastoreBackend) -> Result { let _guard = self .lock() .with_context(|| format!("while destroying group '{self:?}'"))?; @@ -232,10 +235,30 @@ impl BackupGroup { delete_stats.increment_protected_snapshots(); continue; } - snap.destroy(false)?; + // 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)?; delete_stats.increment_removed_snapshots(); } + if let DatastoreBackend::S3(s3_client) = backend { + let path = self.relative_group_path(); + let group_prefix = path + .to_str() + .ok_or_else(|| format_err!("invalid group path prefix"))?; + let prefix = format!("{S3_CONTENT_PREFIX}/{group_prefix}"); + let delete_objects_error = proxmox_async::runtime::block_on( + s3_client.delete_objects_by_prefix_with_suffix_filter( + &S3PathPrefix::Some(prefix), + PROTECTED_MARKER_FILENAME, + &[GROUP_OWNER_FILE_NAME, GROUP_NOTES_FILE_NAME], + ), + )?; + if delete_objects_error { + bail!("deleting objects failed"); + } + } + // Note: make sure the old locking mechanism isn't used as `remove_dir_all` is not safe in // that case if delete_stats.all_removed() && !*OLD_LOCKING { @@ -588,7 +611,7 @@ impl BackupDir { /// Destroy the whole snapshot, bails if it's protected /// /// Setting `force` to true skips locking and thus ignores if the backup is currently in use. - pub fn destroy(&self, force: bool) -> Result<(), Error> { + pub fn destroy(&self, force: bool, backend: &DatastoreBackend) -> Result<(), Error> { let (_guard, _manifest_guard); if !force { _guard = self @@ -601,6 +624,20 @@ impl BackupDir { bail!("cannot remove protected snapshot"); // use special error type? } + if let DatastoreBackend::S3(s3_client) = backend { + let path = self.relative_path(); + let snapshot_prefix = path + .to_str() + .ok_or_else(|| format_err!("invalid snapshot path"))?; + let prefix = format!("{S3_CONTENT_PREFIX}/{snapshot_prefix}"); + let delete_objects_error = proxmox_async::runtime::block_on( + s3_client.delete_objects_by_prefix(&S3PathPrefix::Some(prefix)), + )?; + if delete_objects_error { + bail!("deleting objects failed"); + } + } + let full_path = self.full_path(); log::info!("removing backup snapshot {:?}", full_path); std::fs::remove_dir_all(&full_path).map_err(|err| { @@ -630,6 +667,12 @@ impl BackupDir { // do to rectify the situation. if guard.is_ok() && group.list_backups()?.is_empty() && !*OLD_LOCKING { group.remove_group_dir()?; + if let DatastoreBackend::S3(s3_client) = backend { + let object_key = + super::s3::object_key_from_path(&group.relative_group_path(), "owner") + .context("invalid owner file object key")?; + proxmox_async::runtime::block_on(s3_client.delete_object(object_key))?; + } } else if let Err(err) = guard { log::debug!("{err:#}"); } diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs index 63dc91cb8..4d5ed95f9 100644 --- a/pbs-datastore/src/datastore.rs +++ b/pbs-datastore/src/datastore.rs @@ -12,7 +12,7 @@ use pbs_tools::lru_cache::LruCache; use tracing::{info, warn}; use proxmox_human_byte::HumanByte; -use proxmox_s3_client::{S3Client, S3ClientConfig, S3ClientOptions}; +use proxmox_s3_client::{S3Client, S3ClientConfig, S3ClientOptions, S3PathPrefix}; use proxmox_schema::ApiType; use proxmox_sys::error::SysError; @@ -39,13 +39,17 @@ use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter}; use crate::fixed_index::{FixedIndexReader, FixedIndexWriter}; use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive}; use crate::index::IndexFile; +use crate::s3::S3_CONTENT_PREFIX; use crate::task_tracking::{self, update_active_operations}; use crate::DataBlob; static DATASTORE_MAP: LazyLock>>> = LazyLock::new(|| Mutex::new(HashMap::new())); -const GROUP_NOTES_FILE_NAME: &str = "notes"; +/// Filename to store backup group notes +pub const GROUP_NOTES_FILE_NAME: &str = "notes"; +/// Filename to store backup group owner +pub const GROUP_OWNER_FILE_NAME: &str = "owner"; const NAMESPACE_MARKER_FILENAME: &str = ".namespace"; /// checks if auth_id is owner, or, if owner is a token, if @@ -654,7 +658,9 @@ impl DataStore { let mut stats = BackupGroupDeleteStats::default(); for group in self.iter_backup_groups(ns.to_owned())? { - let delete_stats = group?.destroy()?; + let group = group?; + let backend = self.backend()?; + let delete_stats = group.destroy(&backend)?; stats.add(&delete_stats); removed_all_groups = removed_all_groups && delete_stats.all_removed(); } @@ -688,6 +694,8 @@ impl DataStore { let store = self.name(); let mut removed_all_requested = true; let mut stats = BackupGroupDeleteStats::default(); + let backend = self.backend()?; + if delete_groups { log::info!("removing whole namespace recursively below {store}:/{ns}",); for ns in self.recursive_iter_backup_ns(ns.to_owned())? { @@ -695,6 +703,24 @@ impl DataStore { stats.add(&delete_stats); removed_all_requested = removed_all_requested && removed_ns_groups; } + + if let DatastoreBackend::S3(s3_client) = &backend { + let ns_dir = ns.path(); + let ns_prefix = ns_dir + .to_str() + .ok_or_else(|| format_err!("invalid namespace path prefix"))?; + let prefix = format!("{S3_CONTENT_PREFIX}/{ns_prefix}"); + let delete_objects_error = proxmox_async::runtime::block_on( + s3_client.delete_objects_by_prefix_with_suffix_filter( + &S3PathPrefix::Some(prefix), + PROTECTED_MARKER_FILENAME, + &[GROUP_OWNER_FILE_NAME, GROUP_NOTES_FILE_NAME], + ), + )?; + if delete_objects_error { + bail!("deleting objects failed"); + } + } } else { log::info!("pruning empty namespace recursively below {store}:/{ns}"); } @@ -730,6 +756,14 @@ impl DataStore { log::warn!("failed to remove namespace {ns} - {err}") } } + if let DatastoreBackend::S3(s3_client) = &backend { + // Only remove the namespace marker, if it was empty, + // than this is the same as the namespace being removed. + let object_key = + crate::s3::object_key_from_path(&ns.path(), NAMESPACE_MARKER_FILENAME) + .context("invalid namespace marker object key")?; + proxmox_async::runtime::block_on(s3_client.delete_object(object_key))?; + } } } @@ -747,7 +781,7 @@ impl DataStore { ) -> Result { let backup_group = self.backup_group(ns.clone(), backup_group.clone()); - backup_group.destroy() + backup_group.destroy(&self.backend()?) } /// Remove a backup directory including all content @@ -759,7 +793,7 @@ impl DataStore { ) -> Result<(), Error> { let backup_dir = self.backup_dir(ns.clone(), backup_dir.clone())?; - backup_dir.destroy(force) + backup_dir.destroy(force, &self.backend()?) } /// Returns the time of the last successful backup @@ -787,7 +821,7 @@ impl DataStore { ns: &BackupNamespace, group: &pbs_api_types::BackupGroup, ) -> PathBuf { - self.group_path(ns, group).join("owner") + self.group_path(ns, group).join(GROUP_OWNER_FILE_NAME) } /// Returns the backup owner. diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 3e1610936..5f8394d5d 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -422,7 +422,7 @@ pub async fn delete_snapshot( let snapshot = datastore.backup_dir(ns, backup_dir)?; - snapshot.destroy(false)?; + snapshot.destroy(false, &datastore.backend()?)?; Ok(Value::Null) }) @@ -1088,13 +1088,21 @@ pub fn prune( }); if !keep { - if let Err(err) = backup_dir.destroy(false) { - warn!( - "failed to remove dir {:?}: {}", - backup_dir.relative_path(), - err, - ); - } + match datastore.backend() { + Ok(backend) => { + if let Err(err) = backup_dir.destroy(false, &backend) { + warn!( + "failed to remove dir {:?}: {}", + backup_dir.relative_path(), + err, + ); + } + } + Err(err) => warn!( + "failed to remove dir {:?}: {err}", + backup_dir.relative_path() + ), + }; } } prune_result -- 2.47.2 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel