[pbs-devel] [PATCH proxmox-backup v8 11/45] api: backup: conditionally upload manifest to s3 object store backend
Christian Ebner
c.ebner at proxmox.com
Fri Jul 18 11:33:33 CEST 2025
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 at 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 at 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;
>> })
>> } {
>
More information about the pbs-devel
mailing list