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 7AD681FF17A for ; Fri, 18 Jul 2025 10:37:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 718F416CE3; Fri, 18 Jul 2025 10:38:32 +0200 (CEST) Message-ID: Date: Fri, 18 Jul 2025 10:38:28 +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-24-c.ebner@proxmox.com> Content-Language: de-AT, en-US From: Lukas Wagner In-Reply-To: <20250715125332.954494-24-c.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1752827905656 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.020 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 14/45] api: reader: fetch chunks based on datastore 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" One comment inline, but nothing prohibitive of a R-b: Reviewed-by: Lukas Wagner On 2025-07-15 14:53, Christian Ebner wrote: > Read the chunk based on the datastores backend, reading from local > filesystem or fetching from S3 object store. > > Signed-off-by: Christian Ebner > --- > changes since version 7: > - no changes > > src/api2/reader/environment.rs | 12 ++++++---- > src/api2/reader/mod.rs | 41 +++++++++++++++++++++++----------- > 2 files changed, 36 insertions(+), 17 deletions(-) > > diff --git a/src/api2/reader/environment.rs b/src/api2/reader/environment.rs > index 3b2f06f43..8924352b0 100644 > --- a/src/api2/reader/environment.rs > +++ b/src/api2/reader/environment.rs > @@ -1,13 +1,14 @@ > use std::collections::HashSet; > use std::sync::{Arc, RwLock}; > > +use anyhow::Error; > use serde_json::{json, Value}; > > use proxmox_router::{RpcEnvironment, RpcEnvironmentType}; > > use pbs_api_types::Authid; > use pbs_datastore::backup_info::BackupDir; > -use pbs_datastore::DataStore; > +use pbs_datastore::{DataStore, DatastoreBackend}; > use proxmox_rest_server::formatter::*; > use proxmox_rest_server::WorkerTask; > use tracing::info; > @@ -23,6 +24,7 @@ pub struct ReaderEnvironment { > pub worker: Arc, > pub datastore: Arc, > pub backup_dir: BackupDir, > + pub backend: DatastoreBackend, > allowed_chunks: Arc>>, > } > > @@ -33,8 +35,9 @@ impl ReaderEnvironment { > worker: Arc, > datastore: Arc, > backup_dir: BackupDir, > - ) -> Self { > - Self { > + ) -> Result { > + let backend = datastore.backend()?; > + Ok(Self { > result_attributes: json!({}), > env_type, > auth_id, > @@ -43,8 +46,9 @@ impl ReaderEnvironment { > debug: tracing::enabled!(tracing::Level::DEBUG), > formatter: JSON_FORMATTER, > backup_dir, > + backend, > allowed_chunks: Arc::new(RwLock::new(HashSet::new())), > - } > + }) > } > > pub fn log>(&self, msg: S) { > diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs > index a77216043..997d9ca77 100644 > --- a/src/api2/reader/mod.rs > +++ b/src/api2/reader/mod.rs > @@ -3,6 +3,7 @@ > use anyhow::{bail, format_err, Context, Error}; > use futures::*; > use hex::FromHex; > +use http_body_util::BodyExt; > use hyper::body::Incoming; > use hyper::header::{self, HeaderValue, CONNECTION, UPGRADE}; > use hyper::http::request::Parts; > @@ -27,8 +28,9 @@ use pbs_api_types::{ > }; > use pbs_config::CachedUserInfo; > use pbs_datastore::index::IndexFile; > -use pbs_datastore::{DataStore, PROXMOX_BACKUP_READER_PROTOCOL_ID_V1}; > +use pbs_datastore::{DataStore, DatastoreBackend, PROXMOX_BACKUP_READER_PROTOCOL_ID_V1}; > use pbs_tools::json::required_string_param; > +use proxmox_s3_client::S3Client; > > use crate::api2::backup::optional_ns_param; > use crate::api2::helpers; > @@ -162,7 +164,7 @@ fn upgrade_to_backup_reader_protocol( > worker.clone(), > datastore, > backup_dir, > - ); > + )?; > > env.debug = debug; > > @@ -323,17 +325,10 @@ fn download_chunk( > )); > } > > - let (path, _) = env.datastore.chunk_path(&digest); > - let path2 = path.clone(); > - > - env.debug(format!("download chunk {:?}", path)); > - > - let data = > - proxmox_async::runtime::block_in_place(|| std::fs::read(path)).map_err(move |err| { > - http_err!(BAD_REQUEST, "reading file {:?} failed: {}", path2, err) > - })?; > - > - let body = Body::from(data); > + let body = match &env.backend { > + DatastoreBackend::Filesystem => load_from_filesystem(env, &digest)?, > + DatastoreBackend::S3(s3_client) => fetch_from_object_store(s3_client, &digest).await?, > + }; > > // fixme: set other headers ? > Ok(Response::builder() > @@ -345,6 +340,26 @@ fn download_chunk( > .boxed() > } > > +async fn fetch_from_object_store(s3_client: &S3Client, digest: &[u8; 32]) -> Result { > + let object_key = pbs_datastore::s3::object_key_from_digest(digest)?; > + if let Some(response) = s3_client.get_object(object_key).await? { ^ Do we maybe want some kind of retry-logic for retrieving objects as well? Disregard in case you implement it in a later patch, I'm reviewing this series patch by patch. > + let data = response.content.collect().await?.to_bytes(); > + return Ok(Body::from(data)); > + } > + bail!("cannot find chunk with digest {}", hex::encode(digest)); > +} > + > +fn load_from_filesystem(env: &ReaderEnvironment, digest: &[u8; 32]) -> Result { > + let (path, _) = env.datastore.chunk_path(digest); > + let path2 = path.clone(); > + > + env.debug(format!("download chunk {path:?}")); > + > + let data = proxmox_async::runtime::block_in_place(|| std::fs::read(path)) > + .map_err(move |err| http_err!(BAD_REQUEST, "reading file {path2:?} failed: {err}"))?; > + Ok(Body::from(data)) > +} > + > /* this is too slow > fn download_chunk_old( > _parts: Parts, -- - Lukas _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel