[pbs-devel] [PATCH proxmox-backup v8 11/45] api: backup: conditionally upload manifest to s3 object store backend
Lukas Wagner
l.wagner at proxmox.com
Fri Jul 18 10:26:58 CEST 2025
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
> + 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
> +
> 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;
> })
> } {
--
- Lukas
More information about the pbs-devel
mailing list