[pbs-devel] [PATCH proxmox-backup v6 19/37] datastore: prune groups/snapshots from s3 object store backend

From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH proxmox-backup v6 19/37] datastore: prune groups/snapshots from s3 object store backend
Date: Tue,  8 Jul 2025 19:00:56 +0200	[thread overview]
Message-ID: <20250708170114.1556057-29-c.ebner@proxmox.com> (raw)
In-Reply-To: <20250708170114.1556057-1-c.ebner@proxmox.com>

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 <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 49 ++++++++++++++++++++++++++++++--
 pbs-datastore/src/datastore.rs   | 43 ++++++++++++++++++++++++----
 src/api2/admin/datastore.rs      | 24 ++++++++++------
 3 files changed, 100 insertions(+), 16 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 112d82658..2e811f32a 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;
 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<BackupGroupDeleteStats, Error> {
+    pub fn destroy(&self, backend: &DatastoreBackend) -> Result<BackupGroupDeleteStats, Error> {
         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,
+                    &["owner", 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 099c65ce2..265624229 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -12,7 +12,9 @@ use pbs_tools::lru_cache::LruCache;
 use tracing::{info, warn};
 
 use proxmox_human_byte::HumanByte;
-use proxmox_s3_client::{S3Client, S3ClientConfig, S3ClientOptions, S3ClientSecretsConfig};
+use proxmox_s3_client::{
+    S3Client, S3ClientConfig, S3ClientOptions, S3ClientSecretsConfig, S3PathPrefix,
+};
 use proxmox_schema::ApiType;
 
 use proxmox_sys::error::SysError;
@@ -37,13 +39,14 @@ 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<Mutex<HashMap<String, Arc<DataStoreImpl>>>> =
     LazyLock::new(|| Mutex::new(HashMap::new()));
 
-const GROUP_NOTES_FILE_NAME: &str = "notes";
+pub const GROUP_NOTES_FILE_NAME: &str = "notes";
 const NAMESPACE_MARKER_FILENAME: &str = ".namespace";
 
 /// checks if auth_id is owner, or, if owner is a token, if
@@ -654,7 +657,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 +693,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 +702,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,
+                        &["owner", GROUP_NOTES_FILE_NAME],
+                    ),
+                )?;
+                if delete_objects_error {
+                    bail!("deleting objects failed");
+                }
+            }
         } else {
             log::info!("pruning empty namespace recursively below {store}:/{ns}");
         }
@@ -730,6 +755,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 +780,7 @@ impl DataStore {
     ) -> Result<BackupGroupDeleteStats, Error> {
         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 +792,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
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 070711a1c..f4539e6f1 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

