Re: [pbs-devel] [PATCH proxmox-backup v8 11/45] api: backup: conditionally upload manifest to s3 object store backend

[Christian Ebner <c.ebner@proxmox.com>]

On 7/18/25 10:26 AM, Lukas Wagner wrote:
> Two minor suggestions, but nothing that would prohibit my R-b:
> 
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> 
> On  2025-07-15 14:52, Christian Ebner wrote:
>> Reupload the manifest to the S3 object store backend on manifest
>> updates, if s3 is configured as backend.
>> This also triggers the initial manifest upload when finishing backup
>> snapshot in the backup api call handler.
>> Updates also the locally cached version for fast and efficient
>> listing of contents without the need to perform expensive (as in
>> monetary cost and IO latency) requests.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 7:
>> - no changes
>>
>>   pbs-datastore/Cargo.toml         |  3 +++
>>   pbs-datastore/src/backup_info.rs | 12 +++++++++++-
>>   src/api2/admin/datastore.rs      | 14 ++++++++++++--
>>   src/api2/backup/environment.rs   | 16 ++++++++--------
>>   src/backup/verify.rs             |  2 +-
>>   5 files changed, 35 insertions(+), 12 deletions(-)
>>
>> diff --git a/pbs-datastore/Cargo.toml b/pbs-datastore/Cargo.toml
>> index c42eff165..7e56dbd31 100644
>> --- a/pbs-datastore/Cargo.toml
>> +++ b/pbs-datastore/Cargo.toml
>> @@ -13,6 +13,7 @@ crc32fast.workspace = true
>>   endian_trait.workspace = true
>>   futures.workspace = true
>>   hex = { workspace = true, features = [ "serde" ] }
>> +hyper.workspace = true
>>   libc.workspace = true
>>   log.workspace = true
>>   nix.workspace = true
>> @@ -29,8 +30,10 @@ zstd-safe.workspace = true
>>   pathpatterns.workspace = true
>>   pxar.workspace = true
>>   
>> +proxmox-async.workspace = true
>>   proxmox-base64.workspace = true
>>   proxmox-borrow.workspace = true
>> +proxmox-http.workspace = true
>>   proxmox-human-byte.workspace = true
>>   proxmox-io.workspace = true
>>   proxmox-lang.workspace=true
>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
>> index e3ecd437f..46e5b61f0 100644
>> --- a/pbs-datastore/src/backup_info.rs
>> +++ b/pbs-datastore/src/backup_info.rs
>> @@ -19,7 +19,7 @@ use pbs_api_types::{
>>   use pbs_config::{open_backup_lockfile, BackupLockGuard};
>>   
>>   use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
>> -use crate::{DataBlob, DataStore};
>> +use crate::{DataBlob, DataStore, DatastoreBackend};
>>   
>>   pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
>>   const PROTECTED_MARKER_FILENAME: &str = ".protected";
>> @@ -666,6 +666,7 @@ impl BackupDir {
>>       /// only use this method - anything else may break locking guarantees.
>>       pub fn update_manifest(
>>           &self,
>> +        backend: &DatastoreBackend,
>>           update_fn: impl FnOnce(&mut BackupManifest),
>>       ) -> Result<(), Error> {
>>           let _guard = self.lock_manifest()?;
>> @@ -678,6 +679,15 @@ impl BackupDir {
>>           let blob = DataBlob::encode(manifest.as_bytes(), None, true)?;
>>           let raw_data = blob.raw_data();
>>   
>> +        if let DatastoreBackend::S3(s3_client) = backend {
>> +            let object_key =
>> +                super::s3::object_key_from_path(&self.relative_path(), MANIFEST_BLOB_NAME.as_ref())
>> +                    .context("invalid manifest object key")?;
>> +            let data = hyper::body::Bytes::copy_from_slice(raw_data);
>> +            proxmox_async::runtime::block_on(s3_client.upload_with_retry(object_key, data, true))
>> +                .context("failed to update manifest on s3 backend")?;
>> +        }
>> +
>>           let mut path = self.full_path();
>>           path.push(MANIFEST_BLOB_NAME.as_ref());
>>   
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index e24bc1c1b..02666afda 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -65,7 +65,7 @@ use pbs_datastore::manifest::BackupManifest;
>>   use pbs_datastore::prune::compute_prune_info;
>>   use pbs_datastore::{
>>       check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, BackupGroup,
>> -    DataStore, LocalChunkReader, StoreProgress,
>> +    DataStore, DatastoreBackend, LocalChunkReader, StoreProgress,
>>   };
>>   use pbs_tools::json::required_string_param;
>>   use proxmox_rest_server::{formatter, WorkerTask};
>> @@ -2086,6 +2086,16 @@ pub fn set_group_notes(
>>           &backup_group,
>>       )?;
>>   
>> +    if let DatastoreBackend::S3(s3_client) = datastore.backend()? {
>> +        let mut path = ns.path();
>> +        path.push(format!("{backup_group}"));
> 
> You can just use .to_string() here, reads a bit nicer

Indeed, adapted this ...

>> +        let object_key = pbs_datastore::s3::object_key_from_path(&path, "notes")
>> +            .context("invalid owner file object key")?;
>> +        let data = hyper::body::Bytes::copy_from_slice(notes.as_bytes());
>> +        let _is_duplicate =
>> +            proxmox_async::runtime::block_on(s3_client.upload_with_retry(object_key, data, true))
>> +                .context("failed to set notes on s3 backend")?;
>> +    }
>>       let notes_path = datastore.group_notes_path(&ns, &backup_group);
>>       replace_file(notes_path, notes.as_bytes(), CreateOptions::new(), false)?;
>>   
>> @@ -2188,7 +2198,7 @@ pub fn set_notes(
>>       let backup_dir = datastore.backup_dir(ns, backup_dir)?;
>>   
>>       backup_dir
>> -        .update_manifest(|manifest| {
>> +        .update_manifest(&datastore.backend()?, |manifest| {
>>               manifest.unprotected["notes"] = notes.into();
>>           })
>>           .map_err(|err| format_err!("unable to update manifest blob - {}", err))?;
>> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
>> index 9ad13aeb3..0017b347d 100644
>> --- a/src/api2/backup/environment.rs
>> +++ b/src/api2/backup/environment.rs
>> @@ -646,14 +646,6 @@ impl BackupEnvironment {
>>               bail!("backup does not contain valid files (file count == 0)");
>>           }
>>   
>> -        // check for valid manifest and store stats
>> -        let stats = serde_json::to_value(state.backup_stat)?;
>> -        self.backup_dir
>> -            .update_manifest(|manifest| {
>> -                manifest.unprotected["chunk_upload_stats"] = stats;
>> -            })
>> -            .map_err(|err| format_err!("unable to update manifest blob - {}", err))?;
>> -
>>           if let Some(base) = &self.last_backup {
>>               let path = base.backup_dir.full_path();
>>               if !path.exists() {
>> @@ -664,6 +656,14 @@ impl BackupEnvironment {
>>               }
>>           }
>>   
>> +        // check for valid manifest and store stats
>> +        let stats = serde_json::to_value(state.backup_stat)?;
>> +        self.backup_dir
>> +            .update_manifest(&self.backend, |manifest| {
>> +                manifest.unprotected["chunk_upload_stats"] = stats;
>> +            })
>> +            .map_err(|err| format_err!("unable to update manifest blob - {}", err))?;
> 
> nit: you can inline the `err` variable here

... and inlined the error as requested

>> +
>>           self.datastore.try_ensure_sync_level()?;
>>   
>>           // marks the backup as successful
>> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
>> index 0b954ae23..9344033d8 100644
>> --- a/src/backup/verify.rs
>> +++ b/src/backup/verify.rs
>> @@ -359,7 +359,7 @@ impl VerifyWorker {
>>   
>>           if let Err(err) = {
>>               let verify_state = serde_json::to_value(verify_state)?;
>> -            backup_dir.update_manifest(|manifest| {
>> +            backup_dir.update_manifest(&self.datastore.backend()?, |manifest| {
>>                   manifest.unprotected["verify_state"] = verify_state;
>>               })
>>           } {
> 



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