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 ABAB81FF17A for ; Fri, 18 Jul 2025 10:26:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9CB5B16616; Fri, 18 Jul 2025 10:27:32 +0200 (CEST) Message-ID: <8a18aa0e-3a16-4c63-bf76-a8d198173ba2@proxmox.com> Date: Fri, 18 Jul 2025 10:26:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Christian Ebner References: <20250715125332.954494-1-c.ebner@proxmox.com> <20250715125332.954494-21-c.ebner@proxmox.com> Content-Language: de-AT, en-US From: Lukas Wagner In-Reply-To: <20250715125332.954494-21-c.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1752827216360 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.018 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [environment.rs, datastore.rs, verify.rs] 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > + 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 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel