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 A1ADD1FF17A for ; Fri, 18 Jul 2025 13:44:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 29D7C1C46F; Fri, 18 Jul 2025 13:45:34 +0200 (CEST) Message-ID: <4f257205-f98f-4cfe-94b7-adf6383dd1d9@proxmox.com> Date: Fri, 18 Jul 2025 13:45:30 +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-27-c.ebner@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1752839127727 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.044 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 Subject: Re: [pbs-devel] [PATCH proxmox-backup v8 17/45] verify: implement chunk verification for stores with s3 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:56 AM, Lukas Wagner wrote: > On 2025-07-15 14:53, Christian Ebner wrote: >> For datastores backed by an S3 compatible object store, rather than >> reading the chunks to be verified from the local filesystem, fetch >> them via the s3 client from the configured bucket. >> >> Signed-off-by: Christian Ebner >> --- >> changes since version 7: >> - no changes >> >> src/backup/verify.rs | 89 ++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 77 insertions(+), 12 deletions(-) >> >> diff --git a/src/backup/verify.rs b/src/backup/verify.rs >> index dea10f618..3a4a1d0d5 100644 >> --- a/src/backup/verify.rs >> +++ b/src/backup/verify.rs >> @@ -5,6 +5,7 @@ use std::sync::{Arc, Mutex}; >> use std::time::Instant; >> >> use anyhow::{bail, Error}; >> +use http_body_util::BodyExt; >> use tracing::{error, info, warn}; >> >> use proxmox_worker_task::WorkerTaskContext; >> @@ -89,6 +90,38 @@ impl VerifyWorker { >> } >> } >> >> + if let Ok(DatastoreBackend::S3(s3_client)) = datastore.backend() { >> + let suffix = format!(".{}.bad", counter); >> + let target_key = >> + match pbs_datastore::s3::object_key_from_digest_with_suffix(digest, &suffix) { >> + Ok(target_key) => target_key, >> + Err(err) => { >> + info!("could not generate target key for corrupted chunk {path:?} - {err}"); >> + return; >> + } >> + }; >> + let object_key = match pbs_datastore::s3::object_key_from_digest(digest) { >> + Ok(object_key) => object_key, >> + Err(err) => { >> + info!("could not generate object key for corrupted chunk {path:?} - {err}"); >> + return; >> + } >> + }; >> + if proxmox_async::runtime::block_on( >> + s3_client.copy_object(object_key.clone(), target_key), >> + ) >> + .is_ok() >> + { >> + if proxmox_async::runtime::block_on(s3_client.delete_object(object_key)).is_err() { >> + info!("failed to delete corrupt chunk on s3 backend: {digest_str}"); >> + } >> + } else { >> + info!("failed to copy corrupt chunk on s3 backend: {digest_str}"); >> + } >> + } else { >> + info!("failed to get s3 backend while trying to rename bad chunk: {digest_str}"); >> + } >> + >> match std::fs::rename(&path, &new_path) { >> Ok(_) => { >> info!("corrupted chunk renamed to {:?}", &new_path); >> @@ -189,18 +222,50 @@ impl VerifyWorker { >> continue; // already verified or marked corrupt >> } >> >> - match self.datastore.load_chunk(&info.digest) { >> - Err(err) => { >> - self.corrupt_chunks.lock().unwrap().insert(info.digest); >> - error!("can't verify chunk, load failed - {err}"); >> - errors.fetch_add(1, Ordering::SeqCst); >> - Self::rename_corrupted_chunk(self.datastore.clone(), &info.digest); >> - } >> - Ok(chunk) => { >> - let size = info.size(); >> - read_bytes += chunk.raw_size(); >> - decoder_pool.send((chunk, info.digest, size))?; >> - decoded_bytes += size; >> + match &self.backend { > > The whole method becomes uncomfortably large, maybe move the entire match &self.backend into a new method? Okay, took me a bit since all the rquired interdependencies here, but now this is all placed in a verify_chunk_by_backend() > >> + DatastoreBackend::Filesystem => match self.datastore.load_chunk(&info.digest) { >> + Err(err) => { >> + self.corrupt_chunks.lock().unwrap().insert(info.digest); > > Maybe add a new method self.add_corrupt_chunk > > fn add_corrupt_chunk(&mut self, chunk: ...) { > // Panic on poisoned mutex > let mut chunks = self.corrupt_chunks.lock().unwrap(); > > chunks.insert(chunk); > } > > or the like and this moved into a dedicated helper as suggested, but with the error counting and message included. > >> + error!("can't verify chunk, load failed - {err}"); >> + errors.fetch_add(1, Ordering::SeqCst); >> + Self::rename_corrupted_chunk(self.datastore.clone(), &info.digest); >> + } >> + Ok(chunk) => { >> + let size = info.size(); >> + read_bytes += chunk.raw_size(); >> + decoder_pool.send((chunk, info.digest, size))?; >> + decoded_bytes += size; >> + } >> + }, >> + DatastoreBackend::S3(s3_client) => { >> + let object_key = pbs_datastore::s3::object_key_from_digest(&info.digest)?; >> + match proxmox_async::runtime::block_on(s3_client.get_object(object_key)) { >> + Ok(Some(response)) => { >> + let bytes = >> + proxmox_async::runtime::block_on(response.content.collect())? >> + .to_bytes(); >> + let chunk = DataBlob::from_raw(bytes.to_vec())?; >> + let size = info.size(); >> + read_bytes += chunk.raw_size(); >> + decoder_pool.send((chunk, info.digest, size))?; >> + decoded_bytes += size; >> + } >> + Ok(None) => { >> + self.corrupt_chunks.lock().unwrap().insert(info.digest); >> + error!( >> + "can't verify missing chunk with digest {}", >> + hex::encode(info.digest) >> + ); >> + errors.fetch_add(1, Ordering::SeqCst); >> + Self::rename_corrupted_chunk(self.datastore.clone(), &info.digest); >> + } >> + Err(err) => { >> + self.corrupt_chunks.lock().unwrap().insert(info.digest); >> + error!("can't verify chunk, load failed - {err}"); >> + errors.fetch_add(1, Ordering::SeqCst); >> + Self::rename_corrupted_chunk(self.datastore.clone(), &info.digest); >> + } >> + } >> } >> } >> } > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel