From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E16231FF17A for ; Fri, 18 Jul 2025 11:32:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 90BD318C59; Fri, 18 Jul 2025 11:34:07 +0200 (CEST) Message-ID: <6eb1c36b-37f3-492b-9edd-1aaf121bf88b@proxmox.com> Date: Fri, 18 Jul 2025 11:33:33 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Lukas Wagner , Proxmox Backup Server development discussion References: <20250715125332.954494-1-c.ebner@proxmox.com> <20250715125332.954494-21-c.ebner@proxmox.com> <8a18aa0e-3a16-4c63-bf76-a8d198173ba2@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <8a18aa0e-3a16-4c63-bf76-a8d198173ba2@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1752831211357 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v8 11/45] api: backup: conditionally upload manifest to s3 object store backend X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > > 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 >> --- >> 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