all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy
@ 2025-07-28 12:39 Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 1/7] tree wide: fix useless borrow warnings Christian Ebner
                   ` (8 more replies)
  0 siblings, 9 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:39 UTC (permalink / raw)
  To: pbs-devel

This patch series makes sure the datastore contents are also removed
from the s3 backend if the datastore is destroyed with the remove all
data flag being set.

The first 6 patches fix clippy warnings and refactor the s3 client from
datastore config generation into a dedicated helper, since the last
patch requires that as well and this would be the third ocurence thereof.

The last patch finally does the object store cleanup if the datastore is
backed by s3 and the data should be removed. This simply clears all
contents starting with the store prefix from the bucket.

Christian Ebner (7):
  tree wide: fix useless borrow warnings
  client: backup writer: elide lifetime which can be auto inferred
  api: tape: fix clippy warning on map iteration
  datastore: fix clippy warning checking file extension
  datastore: fix clippy warning to use ? on option
  datastore: add helper to get s3 client from datastore config
  datastore: delete all objects on datastore destroy with remove data

 pbs-client/src/backup_writer.rs    |   4 +-
 pbs-config/src/datastore.rs        |   2 +-
 pbs-datastore/src/datastore.rs     |  64 +++++++++++++---
 src/api2/config/datastore.rs       | 119 +++++++++--------------------
 src/api2/config/tape_backup_job.rs |   2 +-
 src/api2/tape/restore.rs           |   2 +-
 6 files changed, 97 insertions(+), 96 deletions(-)

-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 1/7] tree wide: fix useless borrow warnings
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 2/7] client: backup writer: elide lifetime which can be auto inferred Christian Ebner
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

Fix the useless borrows reported by `cargo clippy`.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-config/src/datastore.rs        | 2 +-
 pbs-datastore/src/datastore.rs     | 6 +++---
 src/api2/config/tape_backup_job.rs | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index 5a5553dbc..4aecc0d3c 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -117,7 +117,7 @@ pub fn complete_calendar_event(_arg: &str, _param: &HashMap<String, String>) ->
 /// Returns the datastore backend type from it's name
 pub fn datastore_backend_type(store: &str) -> Result<pbs_api_types::DatastoreBackendType, Error> {
     let (config, _) = config()?;
-    let store_config: DataStoreConfig = config.lookup("datastore", &store)?;
+    let store_config: DataStoreConfig = config.lookup("datastore", store)?;
 
     let backend_config: pbs_api_types::DatastoreBackendConfig = serde_json::from_value(
         pbs_api_types::DatastoreBackendConfig::API_SCHEMA
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e8be576f7..e3c0589a4 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1780,7 +1780,7 @@ impl DataStore {
         delete_list: &mut Vec<S3ObjectKey>,
         gc_status: &mut GarbageCollectionStatus,
     ) -> Result<bool, Error> {
-        let (chunk_path, digest) = match self.chunk_path_from_object_key(&object_key) {
+        let (chunk_path, digest) = match self.chunk_path_from_object_key(object_key) {
             Some(path) => path,
             None => return Ok(false),
         };
@@ -2218,7 +2218,7 @@ impl DataStore {
                 .await?
                 .context("failed to set maintenance mode")?;
 
-                let tmp_base = proxmox_sys::fs::make_tmp_dir(&self.base_path(), None)
+                let tmp_base = proxmox_sys::fs::make_tmp_dir(self.base_path(), None)
                     .context("failed to create temporary content folder in {store_base}")?;
 
                 if let Err(err) = async {
@@ -2377,7 +2377,7 @@ impl DataStore {
             .with_context(|| format!("failed to refresh {:?}", self.base_path()))?;
         }
 
-        std::fs::remove_dir_all(&tmp_base)
+        std::fs::remove_dir_all(tmp_base)
             .with_context(|| format!("failed to cleanup temporary content in {tmp_base:?}"))?;
 
         Ok(())
diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index 786acde06..38dbe9747 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -185,7 +185,7 @@ pub fn update_tape_backup_job(
     digest: Option<String>,
 ) -> Result<(), Error> {
     if let Some(store) = &update.setup.store {
-        assert_datastore_type(&store)?;
+        assert_datastore_type(store)?;
     }
     let _lock = pbs_config::tape_job::lock()?;
 
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 2/7] client: backup writer: elide lifetime which can be auto inferred
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 1/7] tree wide: fix useless borrow warnings Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 3/7] api: tape: fix clippy warning on map iteration Christian Ebner
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

Fix the issue as reported by a `cargo clippy` run.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/backup_writer.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index 9f62baba3..03202eac5 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -91,9 +91,9 @@ impl BackupWriter {
     }
 
     #[allow(clippy::too_many_arguments)]
-    pub async fn start<'a>(
+    pub async fn start(
         client: &HttpClient,
-        writer_options: BackupWriterOptions<'a>,
+        writer_options: BackupWriterOptions<'_>,
     ) -> Result<Arc<BackupWriter>, Error> {
         let mut param = json!({
             "backup-type": writer_options.backup.ty(),
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 3/7] api: tape: fix clippy warning on map iteration
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 1/7] tree wide: fix useless borrow warnings Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 2/7] client: backup writer: elide lifetime which can be auto inferred Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 4/7] datastore: fix clippy warning checking file extension Christian Ebner
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

The loop only iterates over the values of the map, so use
the `values` iterator instead of iterating key value pairs.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/tape/restore.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 9e55cae26..8b6979017 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -354,7 +354,7 @@ pub fn restore(
         bail!("no datastores given");
     }
 
-    for (_, (target, _)) in &used_datastores {
+    for (target, _) in used_datastores.values() {
         assert_datastore_type(target.name())?;
     }
 
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 4/7] datastore: fix clippy warning checking file extension
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (2 preceding siblings ...)
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 3/7] api: tape: fix clippy warning on map iteration Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 5/7] datastore: fix clippy warning to use ? on option Christian Ebner
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

Follow the suggestion and check the file extension instead of relying
on the path to str conversion.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e3c0589a4..3b91a3583 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1798,7 +1798,10 @@ impl DataStore {
         };
         let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
 
-        let bad = chunk_path.as_path().ends_with(".bad");
+        let bad = chunk_path
+            .as_path()
+            .extension()
+            .is_some_and(|ext| ext == "bad");
 
         if atime < min_atime {
             if let Some(cache) = self.cache() {
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 5/7] datastore: fix clippy warning to use ? on option
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (3 preceding siblings ...)
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 4/7] datastore: fix clippy warning checking file extension Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 6/7] datastore: add helper to get s3 client from datastore config Christian Ebner
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

As cargo clippy suggests, simple unwrapping of an Option value with
early return if None can be achieved using the question mark syntax.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 3b91a3583..a6849ecf4 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1835,11 +1835,8 @@ impl DataStore {
     // Check and generate a chunk path from given object key
     fn chunk_path_from_object_key(&self, object_key: &S3ObjectKey) -> Option<(PathBuf, [u8; 32])> {
         // Check object is actually a chunk
-        let digest = match Path::new::<str>(object_key).file_name() {
-            Some(file_name) => file_name,
-            // should never be the case as objects will have a filename
-            None => return None,
-        };
+        // file_name() should always be Some, as objects will have a filename
+        let digest = Path::new::<str>(object_key).file_name()?;
         let bytes = digest.as_bytes();
         if bytes.len() != 64 && bytes.len() != 64 + ".0.bad".len() {
             return None;
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 6/7] datastore: add helper to get s3 client from datastore config
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (4 preceding siblings ...)
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 5/7] datastore: fix clippy warning to use ? on option Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 7/7] datastore: delete all objects on datastore destroy with remove data Christian Ebner
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

Refactors the currently duplicate code to get the s3 client based on
a datastore config into a dedicated helper associated function of the
datastore. This can then further be used to get the s3 client for
datastore deletion when all data are requested to be removed.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs |  34 ++++++++++
 src/api2/config/datastore.rs   | 119 ++++++++++-----------------------
 2 files changed, 71 insertions(+), 82 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index a6849ecf4..3bc3aab0d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2382,4 +2382,38 @@ impl DataStore {
 
         Ok(())
     }
+
+    pub fn s3_client_and_backend_from_datastore_config(
+        datastore_config: &DataStoreConfig,
+    ) -> Result<(DatastoreBackendType, Option<S3Client>), Error> {
+        let backend_config: DatastoreBackendConfig =
+            datastore_config.backend.as_deref().unwrap_or("").parse()?;
+        let backend_type = backend_config.ty.unwrap_or_default();
+
+        if backend_type != DatastoreBackendType::S3 {
+            return Ok((backend_type, None));
+        }
+
+        let s3_client_id = backend_config
+            .client
+            .as_ref()
+            .ok_or_else(|| format_err!("missing required client"))?;
+        let bucket = backend_config
+            .bucket
+            .clone()
+            .ok_or_else(|| format_err!("missing required bucket"))?;
+        let (config, _config_digest) =
+            pbs_config::s3::config().context("failed to get s3 config")?;
+        let client_config: S3ClientConf = config
+            .lookup(S3_CFG_TYPE_ID, s3_client_id)
+            .with_context(|| format!("no '{s3_client_id}' in config"))?;
+        let options = S3ClientOptions::from_config(
+            client_config.config,
+            client_config.secret_key,
+            bucket,
+            datastore_config.name.to_owned(),
+        );
+        let s3_client = S3Client::new(options).context("failed to create s3 client")?;
+        Ok((backend_type, Some(s3_client)))
+    }
 }
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index d24323152..1b56fd276 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -2,25 +2,23 @@ use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
 use ::serde::{Deserialize, Serialize};
-use anyhow::{bail, format_err, Context, Error};
+use anyhow::{bail, Context, Error};
 use hex::FromHex;
 use http_body_util::BodyExt;
 use serde_json::Value;
 use tracing::{info, warn};
 
 use proxmox_router::{http_bail, Permission, Router, RpcEnvironment, RpcEnvironmentType};
-use proxmox_s3_client::{S3Client, S3ClientConf, S3ClientOptions};
 use proxmox_schema::{api, param_bail, ApiType};
 use proxmox_section_config::SectionConfigData;
 use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
-    Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreBackendConfig, DatastoreBackendType,
-    DatastoreNotify, DatastoreTuning, KeepOptions, MaintenanceMode, Operation, PruneJobConfig,
-    PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_MODIFY, PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
+    Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreBackendType, DatastoreNotify,
+    DatastoreTuning, KeepOptions, MaintenanceMode, Operation, PruneJobConfig, PruneJobOptions,
+    DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY,
+    PRIV_SYS_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
 };
-use pbs_config::s3::S3_CFG_TYPE_ID;
 use pbs_config::BackupLockGuard;
 use pbs_datastore::chunk_store::ChunkStore;
 
@@ -34,7 +32,7 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
 use crate::api2::config::verify::delete_verification_job;
 use pbs_config::CachedUserInfo;
 
-use pbs_datastore::{get_datastore_mount_status, DatastoreBackend};
+use pbs_datastore::{get_datastore_mount_status, DataStore, DatastoreBackend};
 use proxmox_rest_server::WorkerTask;
 use proxmox_s3_client::S3ObjectKey;
 
@@ -131,54 +129,34 @@ pub(crate) fn do_create_datastore(
             .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
     )?;
 
-    let backend_config: DatastoreBackendConfig =
-        datastore.backend.as_deref().unwrap_or("").parse()?;
-    let backend_type = backend_config.ty.unwrap_or_default();
-    let backend_s3_client = if backend_type == DatastoreBackendType::S3 {
-        let s3_client_id = backend_config
-            .client
-            .as_ref()
-            .ok_or_else(|| format_err!("missing required client"))?;
-        let bucket = backend_config
-            .bucket
-            .clone()
-            .ok_or_else(|| format_err!("missing required bucket"))?;
-        let (config, _config_digest) =
-            pbs_config::s3::config().context("failed to get s3 config")?;
-        let config: S3ClientConf = config
-            .lookup(S3_CFG_TYPE_ID, s3_client_id)
-            .with_context(|| format!("no '{s3_client_id}' in config"))?;
-        let options = S3ClientOptions::from_config(
-            config.config,
-            config.secret_key,
-            bucket,
-            datastore.name.to_owned(),
-        );
-        let s3_client = S3Client::new(options).context("failed to create s3 client")?;
-
-        if !overwrite_in_use {
-            let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
-                .context("failed to generate s3 object key")?;
-            if let Some(response) =
-                proxmox_async::runtime::block_on(s3_client.get_object(object_key.clone()))
-                    .context("failed to get in-use marker from bucket")?
-            {
-                let content = proxmox_async::runtime::block_on(response.content.collect())
-                    .unwrap_or_default();
-                let content = String::from_utf8(content.to_bytes().to_vec()).unwrap_or_default();
-                let in_use: InUseContent = serde_json::from_str(&content).unwrap_or_default();
-                if let Some(hostname) = in_use.hostname {
-                    bail!("Bucket already contains datastore in use by host {hostname}");
-                } else {
-                    bail!("Bucket already contains datastore in use");
+    let (backend_type, backend_s3_client) =
+        match DataStore::s3_client_and_backend_from_datastore_config(&datastore)? {
+            (backend_type, Some(s3_client)) => {
+                if !overwrite_in_use {
+                    let object_key = S3ObjectKey::try_from(S3_DATASTORE_IN_USE_MARKER)
+                        .context("failed to generate s3 object key")?;
+                    if let Some(response) =
+                        proxmox_async::runtime::block_on(s3_client.get_object(object_key.clone()))
+                            .context("failed to get in-use marker from bucket")?
+                    {
+                        let content = proxmox_async::runtime::block_on(response.content.collect())
+                            .unwrap_or_default();
+                        let content =
+                            String::from_utf8(content.to_bytes().to_vec()).unwrap_or_default();
+                        let in_use: InUseContent =
+                            serde_json::from_str(&content).unwrap_or_default();
+                        if let Some(hostname) = in_use.hostname {
+                            bail!("Bucket already contains datastore in use by host {hostname}");
+                        } else {
+                            bail!("Bucket already contains datastore in use");
+                        }
+                    }
                 }
-            }
-        }
 
-        Some(Arc::new(s3_client))
-    } else {
-        None
-    };
+                (backend_type, Some(Arc::new(s3_client)))
+            }
+            (backend_type, None) => (backend_type, None),
+        };
 
     let unmount_guard = if datastore.backing_device.is_some() {
         do_mount_device(datastore.clone())?;
@@ -359,34 +337,11 @@ pub fn create_datastore(
 
     let store_name = config.name.to_string();
 
-    let backend_config: DatastoreBackendConfig = config.backend.as_deref().unwrap_or("").parse()?;
-    match backend_config.ty.unwrap_or_default() {
-        DatastoreBackendType::Filesystem => (),
-        DatastoreBackendType::S3 => {
-            let s3_client_id = backend_config
-                .client
-                .as_ref()
-                .ok_or_else(|| format_err!("missing required client"))?;
-            let bucket = backend_config
-                .bucket
-                .clone()
-                .ok_or_else(|| format_err!("missing required bucket"))?;
-            let (config, _config_digest) =
-                pbs_config::s3::config().context("failed to get s3 config")?;
-            let config: S3ClientConf = config
-                .lookup(S3_CFG_TYPE_ID, s3_client_id)
-                .with_context(|| format!("no '{s3_client_id}' in config"))?;
-            let options = S3ClientOptions::from_config(
-                config.config,
-                config.secret_key,
-                bucket,
-                store_name.clone(),
-            );
-            let s3_client = S3Client::new(options).context("failed to create s3 client")?;
-            // Fine to block since this runs in worker task
-            proxmox_async::runtime::block_on(s3_client.head_bucket())
-                .context("failed to access bucket")?;
-        }
+    if let (_backend, Some(s3_client)) =
+        DataStore::s3_client_and_backend_from_datastore_config(&config)?
+    {
+        proxmox_async::runtime::block_on(s3_client.head_bucket())
+            .context("failed to access bucket")?;
     }
 
     WorkerTask::new_thread(
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 7/7] datastore: delete all objects on datastore destroy with remove data
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (5 preceding siblings ...)
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 6/7] datastore: add helper to get s3 client from datastore config Christian Ebner
@ 2025-07-28 12:40 ` Christian Ebner
  2025-07-29 13:53 ` [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Lukas Wagner
  2025-07-30  7:59 ` [pbs-devel] superseded: " Christian Ebner
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-28 12:40 UTC (permalink / raw)
  To: pbs-devel

If the user requested to delete a datastore, including all of the data,
also remove the objects from the s3 backend, if any.

This is limited to deleting objects within the datastore path prefix
within the bucket.

Reported-by: Lukas Wagner <l.wagner@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 3bc3aab0d..3e53a670b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -2144,6 +2144,18 @@ impl DataStore {
                 }
             }
 
+            if let (_backend, Some(s3_client)) =
+                Self::s3_client_and_backend_from_datastore_config(&datastore_config)?
+            {
+                // Delete all objects within the datastore prefix
+                let prefix = S3PathPrefix::Some(String::default());
+                let delete_objects_error =
+                    proxmox_async::runtime::block_on(s3_client.delete_objects_by_prefix(&prefix))?;
+                if delete_objects_error {
+                    bail!("deleting objects failed");
+                }
+            }
+
             // chunks get removed last and only if the backups were successfully deleted
             if ok {
                 remove(".chunks", &mut ok);
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (6 preceding siblings ...)
  2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 7/7] datastore: delete all objects on datastore destroy with remove data Christian Ebner
@ 2025-07-29 13:53 ` Lukas Wagner
  2025-07-29 16:54   ` Christian Ebner
  2025-07-30  7:59 ` [pbs-devel] superseded: " Christian Ebner
  8 siblings, 1 reply; 11+ messages in thread
From: Lukas Wagner @ 2025-07-29 13:53 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Christian Ebner

On Mon Jul 28, 2025 at 2:39 PM CEST, Christian Ebner wrote:
> This patch series makes sure the datastore contents are also removed
> from the s3 backend if the datastore is destroyed with the remove all
> data flag being set.
>
> The first 6 patches fix clippy warnings and refactor the s3 client from
> datastore config generation into a dedicated helper, since the last
> patch requires that as well and this would be the third ocurence thereof.
>
> The last patch finally does the object store cleanup if the datastore is
> backed by s3 and the data should be removed. This simply clears all
> contents starting with the store prefix from the bucket.
>

Looks good to me! Also quickly tested this with my minio server.
Also made sure that it does not delete anything else in the bucket.

Only thing that maybe could be improved is the wording on the checkbox
in the "Delete datastore" confirmation dialog. Right now it is:

	[ ] Destroy all data (dangerous!)

Maybe something like this could be more clear:

	[ ] Remove data from S3 bucket and local cache (dangerous!)

But this could also done as a followup, since this is just cosmetic.

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Lukas Wagner <l.wagner@proxmox.com>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy
  2025-07-29 13:53 ` [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Lukas Wagner
@ 2025-07-29 16:54   ` Christian Ebner
  0 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-29 16:54 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox Backup Server development discussion

On 7/29/25 3:53 PM, Lukas Wagner wrote:
> On Mon Jul 28, 2025 at 2:39 PM CEST, Christian Ebner wrote:
>> This patch series makes sure the datastore contents are also removed
>> from the s3 backend if the datastore is destroyed with the remove all
>> data flag being set.
>>
>> The first 6 patches fix clippy warnings and refactor the s3 client from
>> datastore config generation into a dedicated helper, since the last
>> patch requires that as well and this would be the third ocurence thereof.
>>
>> The last patch finally does the object store cleanup if the datastore is
>> backed by s3 and the data should be removed. This simply clears all
>> contents starting with the store prefix from the bucket.
>>
> 
> Looks good to me! Also quickly tested this with my minio server.
> Also made sure that it does not delete anything else in the bucket.
> 
> Only thing that maybe could be improved is the wording on the checkbox
> in the "Delete datastore" confirmation dialog. Right now it is:
> 
> 	[ ] Destroy all data (dangerous!)
> 
> Maybe something like this could be more clear:
> 
> 	[ ] Remove data from S3 bucket and local cache (dangerous!)
> 
> But this could also done as a followup, since this is just cosmetic.

Sent the request followup which can be found at:
https://lore.proxmox.com/pbs-devel/20250729165249.855831-1-c.ebner@proxmox.com/T/



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pbs-devel] superseded: [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy
  2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
                   ` (7 preceding siblings ...)
  2025-07-29 13:53 ` [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Lukas Wagner
@ 2025-07-30  7:59 ` Christian Ebner
  8 siblings, 0 replies; 11+ messages in thread
From: Christian Ebner @ 2025-07-30  7:59 UTC (permalink / raw)
  To: pbs-devel

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20250730075750.36014-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-07-30  7:57 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-28 12:39 [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 1/7] tree wide: fix useless borrow warnings Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 2/7] client: backup writer: elide lifetime which can be auto inferred Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 3/7] api: tape: fix clippy warning on map iteration Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 4/7] datastore: fix clippy warning checking file extension Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 5/7] datastore: fix clippy warning to use ? on option Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 6/7] datastore: add helper to get s3 client from datastore config Christian Ebner
2025-07-28 12:40 ` [pbs-devel] [PATCH proxmox-backup 7/7] datastore: delete all objects on datastore destroy with remove data Christian Ebner
2025-07-29 13:53 ` [pbs-devel] [PATCH proxmox-backup 0/7] remove objects from s3 backend on datastore destroy Lukas Wagner
2025-07-29 16:54   ` Christian Ebner
2025-07-30  7:59 ` [pbs-devel] superseded: " Christian Ebner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal