[PATCH proxmox-backup v2 0/5] fix #7606: verify: s3: handle transient fetch errors and add retry

[PATCH proxmox-backup v2 0/5] fix #7606: verify: s3: handle transient fetch errors and add retry

[Christian Ebner]

Transient errors during fetching of object from S3 backend currently
lead to snapshots being marked as verify failed. Subsequent runs of
verification on the same snapshot might succeed without issues, since
chunks are only flagged as bad if the chunk blob object could not be
decoded or content verification failed.

To fix this, discriminate between transient errors and permanent
errors and account for these separately. For snapshots encountering
only transient errors, but no permanent errors during verification,
do not set or strip the verify okay state, but always keep a
pre-existing verification failed state. This is to never allow reuse
of a snapshot as reference for backup/sync jobs, when the state
was bad before and could not be verified again. For the other case
there is not much to lose, if it was okay or not verified before
falling back to not verified reflects the current snapshot state.

Link to the bugtracker issue:
https://bugzilla.proxmox.com/show_bug.cgi?id=7606

Changes since v1:
- fix off-by-one error in retry count check, only up to 2 retries are
  being performed.
- refactor manifest verify state parsing error handling, it cannot
  propagate an error as that is interpreted as task abort. try to
  update the manifest's verify state instead and log the parsing
  issue.
- early stop verification on transient errors for datastores backed
  by S3, as this causes additional requests, costs and runtime at no
  benefit.
- improve on logging and code style in general


proxmox-backup:

Christian Ebner (5):
  verify: drop useless try_block! in snapshot verification
  datastore: snapshot: propagate errors in manifest update callback
  fix #7606: verify: s3: handle transient chunk load errors
  verify: s3: retry on transient response body collection errors
  verify: s3: early exit on chunk fetch or verification errors

 pbs-datastore/src/backup_info.rs |   4 +-
 pbs-datastore/src/datastore.rs   |   1 +
 src/backup/verify.rs             | 321 ++++++++++++++++++++++---------
 3 files changed, 230 insertions(+), 96 deletions(-)


Summary over all repositories:
  3 files changed, 230 insertions(+), 96 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox-backup v2 1/5] verify: drop useless try_block! in snapshot verification

[Christian Ebner]

Not needed and makes the code more concise as well as better readable
because of the reduced nesting level.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/backup/verify.rs | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 43bee6a36..18fb1ca4c 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -441,14 +441,12 @@ impl VerifyWorker {
 
         let mut verify_result = VerifyState::Ok;
         for info in manifest.files() {
-            let result = proxmox_lang::try_block!({
-                info!("  check {}", info.filename);
-                match info.filename.archive_type() {
-                    ArchiveType::FixedIndex => self.verify_fixed_index(backup_dir, info),
-                    ArchiveType::DynamicIndex => self.verify_dynamic_index(backup_dir, info),
-                    ArchiveType::Blob => Self::verify_blob(backup_dir, info),
-                }
-            });
+            info!("  check {}", info.filename);
+            let result = match info.filename.archive_type() {
+                ArchiveType::FixedIndex => self.verify_fixed_index(backup_dir, info),
+                ArchiveType::DynamicIndex => self.verify_dynamic_index(backup_dir, info),
+                ArchiveType::Blob => Self::verify_blob(backup_dir, info),
+            };
 
             self.worker.check_abort()?;
             self.worker.fail_on_shutdown()?;
-- 
2.47.3

applied: [PATCH proxmox-backup v2 1/5] verify: drop useless try_block! in snapshot verification

[Thomas Lamprecht]

Am 10.11.26 um 16:37 schrieb Christian Ebner:
> Not needed and makes the code more concise as well as better readable
> because of the reduced nesting level.
> 
> No functional changes.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/backup/verify.rs | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
>

applied this one already, thanks!

[PATCH proxmox-backup v2 2/5] datastore: snapshot: propagate errors in manifest update callback

[Christian Ebner]

Allow propagating potential errors from the manifest update callback
to the caller.

In preparation for propagating verify state encoding errors.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 4 ++--
 pbs-datastore/src/datastore.rs   | 1 +
 src/backup/verify.rs             | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index be4ec8b3e..d6d1f07f4 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -1001,12 +1001,12 @@ impl BackupDir {
     pub fn update_manifest(
         &self,
         backend: &DatastoreBackend,
-        update_fn: impl FnOnce(&mut BackupManifest),
+        update_fn: impl FnOnce(&mut BackupManifest) -> Result<(), Error>,
     ) -> Result<(), Error> {
         let _guard = self.lock_manifest()?;
         let (mut manifest, _) = self.load_manifest()?;
 
-        update_fn(&mut manifest);
+        update_fn(&mut manifest)?;
 
         // keep existing signature
         let blob = manifest.to_data_blob(None)?;
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e6766a3b9..0f3b9b711 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -3453,6 +3453,7 @@ impl DataStore {
         snapshot
             .update_manifest(&backend, |manifest| {
                 manifest.unprotected["notes"] = notes.into();
+                Ok(())
             })
             .map_err(|err| format_err!("unable to update manifest blob - {err}"))?;
         Ok(())
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 18fb1ca4c..4d497c219 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -470,6 +470,7 @@ impl VerifyWorker {
             let verify_state = serde_json::to_value(verify_state)?;
             backup_dir.update_manifest(&self.datastore.backend()?, |manifest| {
                 manifest.unprotected["verify_state"] = verify_state;
+                Ok(())
             })
         } {
             info!("verify {datastore_name}:{backup_dir_name} - manifest update error: {err}");
-- 
2.47.3

[PATCH proxmox-backup v2 3/5] fix #7606: verify: s3: handle transient chunk load errors

[Christian Ebner]

Fetching an object for verification might fail when sending the
request to the S3 API or while receiving the response. Both are
currently handled as hard errors, leading to the manifest state being
set to verify failed. This however does not reflect the actual state,
as a subsequent verification can work just fine.

Therefore, distinguish and propagate these error kinds by enum
variants, adapting return types and call sites accordingly.

Handle the variants respectively when setting/updating the verify
state for the manifest: if only transient errors, but no permanent
errors occur, strip any verify okay state since then it is not
possible to determine if the snapshot is still fine. That is best
represented by not having a verify state, as consequence sync jobs
with verified-only filtering will then exclude such snapshots as
well.

Independent of the error variant, always keep a previously set verify
error state in order to avoid allowing backups to reuse this snapshot
as reference. This covers both the new state as well as the manifest
state prior to verification.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7606
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/backup/verify.rs | 190 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 143 insertions(+), 47 deletions(-)

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 4d497c219..670a899ca 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -4,7 +4,7 @@ use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
 
-use anyhow::{Error, bail};
+use anyhow::{Error, format_err};
 use http_body_util::BodyExt;
 use tracing::{error, info, warn};
 
@@ -39,6 +39,7 @@ struct IndexVerifyState {
     read_bytes: AtomicU64,
     decoded_bytes: AtomicU64,
     errors: AtomicUsize,
+    fetch_errors: AtomicUsize,
     datastore: Arc<DataStore>,
     corrupt_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
     verified_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
@@ -55,6 +56,7 @@ impl IndexVerifyState {
             read_bytes: AtomicU64::new(0),
             decoded_bytes: AtomicU64::new(0),
             errors: AtomicUsize::new(0),
+            fetch_errors: AtomicUsize::new(0),
             datastore: Arc::clone(datastore),
             corrupt_chunks: Arc::clone(corrupt_chunks),
             verified_chunks: Arc::clone(verified_chunks),
@@ -63,6 +65,11 @@ impl IndexVerifyState {
     }
 }
 
+enum VerificationError {
+    Transient(Error),
+    Permanent(Error),
+}
+
 impl VerifyWorker {
     /// Creates a new VerifyWorker for a given task worker and datastore.
     pub fn new(
@@ -95,27 +102,38 @@ impl VerifyWorker {
         })
     }
 
-    fn verify_blob(backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> {
-        let blob = backup_dir.load_blob(info.filename.as_ref())?;
+    fn verify_blob(backup_dir: &BackupDir, info: &FileInfo) -> Result<(), VerificationError> {
+        let blob = backup_dir
+            .load_blob(info.filename.as_ref())
+            .map_err(VerificationError::Permanent)?;
 
         let raw_size = blob.raw_size();
         if raw_size != info.size {
-            bail!("wrong size ({} != {})", info.size, raw_size);
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong size ({} != {})",
+                info.size,
+                raw_size
+            )));
         }
 
         let csum = openssl::sha::sha256(blob.raw_data());
         if csum != info.csum {
-            bail!("wrong index checksum");
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong index checksum"
+            )));
         }
 
-        match blob.crypt_mode()? {
+        match blob.crypt_mode().map_err(VerificationError::Permanent)? {
             CryptMode::Encrypt => Ok(()),
             CryptMode::None => {
                 // digest already verified above
-                blob.decode(None, None)?;
+                blob.decode(None, None)
+                    .map_err(VerificationError::Permanent)?;
                 Ok(())
             }
-            CryptMode::SignOnly => bail!("Invalid CryptMode for blob"),
+            CryptMode::SignOnly => Err(VerificationError::Permanent(format_err!(
+                "Invalid CryptMode for blob"
+            ))),
         }
     }
 
@@ -123,7 +141,7 @@ impl VerifyWorker {
         &self,
         index: Box<dyn IndexFile + Send>,
         crypt_mode: CryptMode,
-    ) -> Result<(), Error> {
+    ) -> Result<(), VerificationError> {
         let verify_state = Arc::new(IndexVerifyState::new(
             &self.datastore,
             &self.corrupt_chunks,
@@ -190,7 +208,8 @@ impl VerifyWorker {
 
         let chunk_list = self
             .datastore
-            .get_chunks_in_order(&*index, skip_chunk, check_abort)?;
+            .get_chunks_in_order(&*index, skip_chunk, check_abort)
+            .map_err(VerificationError::Transient)?;
 
         let reader_pool = ParallelHandler::new("read chunks", self.read_threads, {
             let decoder_pool = decoder_pool.channel();
@@ -207,8 +226,12 @@ impl VerifyWorker {
             }
         });
         for (pos, _) in chunk_list {
-            self.worker.check_abort()?;
-            self.worker.fail_on_shutdown()?;
+            self.worker
+                .check_abort()
+                .map_err(VerificationError::Transient)?;
+            self.worker
+                .fail_on_shutdown()
+                .map_err(VerificationError::Transient)?;
 
             let info = index.chunk_info(pos).unwrap();
 
@@ -217,11 +240,17 @@ impl VerifyWorker {
                 continue; // already verified or marked corrupt
             }
 
-            reader_pool.send(info)?;
+            reader_pool
+                .send(info)
+                .map_err(|err| VerificationError::Transient(err.into()))?;
         }
 
-        reader_pool.complete()?;
-        decoder_pool.complete()?;
+        reader_pool
+            .complete()
+            .map_err(|err| VerificationError::Transient(err.into()))?;
+        decoder_pool
+            .complete()
+            .map_err(|err| VerificationError::Transient(err.into()))?;
 
         let elapsed = verify_state.start_time.elapsed().as_secs_f64();
 
@@ -235,13 +264,22 @@ impl VerifyWorker {
         let decode_speed = decoded_bytes_mib / elapsed;
 
         let error_count = verify_state.errors.load(Ordering::SeqCst);
+        let fetch_error_count = verify_state.fetch_errors.load(Ordering::SeqCst);
 
         info!(
-            "  verified {read_bytes_mib:.2}/{decoded_bytes_mib:.2} MiB in {elapsed:.2} seconds, speed {read_speed:.2}/{decode_speed:.2} MiB/s ({error_count} errors)"
+            "  verified {read_bytes_mib:.2}/{decoded_bytes_mib:.2} MiB in {elapsed:.2} seconds, speed {read_speed:.2}/{decode_speed:.2} MiB/s ({error_count} errors, {fetch_error_count} fetch errors)"
         );
 
-        if verify_state.errors.load(Ordering::SeqCst) > 0 {
-            bail!("chunks could not be verified");
+        if error_count > 0 {
+            return Err(VerificationError::Permanent(format_err!(
+                "chunks could not be verified"
+            )));
+        }
+
+        if fetch_error_count > 0 {
+            return Err(VerificationError::Transient(format_err!(
+                "chunks could not be fetched"
+            )));
         }
 
         Ok(())
@@ -297,7 +335,7 @@ impl VerifyWorker {
                                 }
                             }
                             Err(err) => {
-                                verify_state.errors.fetch_add(1, Ordering::SeqCst);
+                                verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
                                 error!("can't verify chunk, load failed - {err}");
                             }
                         }
@@ -311,7 +349,7 @@ impl VerifyWorker {
                         ),
                     ),
                     Err(err) => {
-                        verify_state.errors.fetch_add(1, Ordering::SeqCst);
+                        verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
                         error!("can't verify chunk, load failed - {err}");
                     }
                 }
@@ -333,37 +371,63 @@ impl VerifyWorker {
         }
     }
 
-    fn verify_fixed_index(&self, backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> {
+    fn verify_fixed_index(
+        &self,
+        backup_dir: &BackupDir,
+        info: &FileInfo,
+    ) -> Result<(), VerificationError> {
         let mut path = backup_dir.relative_path();
         path.push(info.filename.as_ref());
 
-        let index = self.datastore.open_fixed_reader(&path)?;
+        let index = self
+            .datastore
+            .open_fixed_reader(&path)
+            .map_err(VerificationError::Permanent)?;
 
         let (csum, size) = index.compute_csum();
         if size != info.size {
-            bail!("wrong size ({} != {})", info.size, size);
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong size ({} != {})",
+                info.size,
+                size
+            )));
         }
 
         if csum != info.csum {
-            bail!("wrong index checksum");
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong index checksum"
+            )));
         }
 
         self.verify_index_chunks(Box::new(index), info.chunk_crypt_mode())
     }
 
-    fn verify_dynamic_index(&self, backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> {
+    fn verify_dynamic_index(
+        &self,
+        backup_dir: &BackupDir,
+        info: &FileInfo,
+    ) -> Result<(), VerificationError> {
         let mut path = backup_dir.relative_path();
         path.push(info.filename.as_ref());
 
-        let index = self.datastore.open_dynamic_reader(&path)?;
+        let index = self
+            .datastore
+            .open_dynamic_reader(&path)
+            .map_err(VerificationError::Permanent)?;
 
         let (csum, size) = index.compute_csum();
         if size != info.size {
-            bail!("wrong size ({} != {})", info.size, size);
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong size ({} != {})",
+                info.size,
+                size
+            )));
         }
 
         if csum != info.csum {
-            bail!("wrong index checksum");
+            return Err(VerificationError::Permanent(format_err!(
+                "wrong index checksum"
+            )));
         }
 
         self.verify_index_chunks(Box::new(index), info.chunk_crypt_mode())
@@ -439,7 +503,7 @@ impl VerifyWorker {
 
         let mut error_count = 0;
 
-        let mut verify_result = VerifyState::Ok;
+        let mut verify_result = Some(VerifyState::Ok);
         for info in manifest.files() {
             info!("  check {}", info.filename);
             let result = match info.filename.archive_type() {
@@ -451,28 +515,60 @@ impl VerifyWorker {
             self.worker.check_abort()?;
             self.worker.fail_on_shutdown()?;
 
-            if let Err(err) = result {
-                info!(
-                    "verify {datastore_name}:{backup_dir_name}/{file_name} failed: {err}",
-                    file_name = info.filename,
-                );
-                error_count += 1;
-                verify_result = VerifyState::Failed;
+            match result {
+                Ok(_) => (),
+                Err(VerificationError::Permanent(err)) => {
+                    error_count += 1;
+                    verify_result = Some(VerifyState::Failed);
+
+                    info!(
+                        "verify {datastore_name}:{backup_dir_name}/{file_name} failed with permanent error: {err}",
+                        file_name = info.filename,
+                    );
+                }
+                Err(VerificationError::Transient(err)) => {
+                    error_count += 1;
+                    match manifest.verify_state() {
+                        Ok(Some(SnapshotVerifyState {
+                            state: VerifyState::Failed,
+                            ..
+                        })) => {
+                            // keep failed state, to never allow chunk reuse for subsequent backup
+                            verify_result = Some(VerifyState::Failed);
+                        }
+                        Err(err) => {
+                            info!(
+                                "failed to parse verify state for {datastore_name}:{backup_dir_name}: {err}"
+                            );
+                            // set to failed since verify state parsing failed
+                            verify_result = Some(VerifyState::Failed);
+                        }
+                        _ => {
+                            // strip any VerifyState::Ok when updating the manifest
+                            if let Some(VerifyState::Ok) = verify_result {
+                                verify_result = None;
+                            }
+                        }
+                    };
+
+                    info!(
+                        "verify {datastore_name}:{backup_dir_name}/{file_name} failed with transient error: {err}",
+                        file_name = info.filename,
+                    );
+                }
             }
         }
 
-        let verify_state = SnapshotVerifyState {
-            state: verify_result,
-            upid,
-        };
-
-        if let Err(err) = {
-            let verify_state = serde_json::to_value(verify_state)?;
-            backup_dir.update_manifest(&self.datastore.backend()?, |manifest| {
+        let verify_state = verify_result.map(|state| SnapshotVerifyState { state, upid });
+        if let Err(err) = backup_dir.update_manifest(&self.datastore.backend()?, |manifest| {
+            if let Some(verify_state) = verify_state {
+                let verify_state = serde_json::to_value(verify_state)?;
                 manifest.unprotected["verify_state"] = verify_state;
-                Ok(())
-            })
-        } {
+            } else if let Some(unprotected) = manifest.unprotected.as_object_mut() {
+                unprotected.remove("verify_state");
+            }
+            Ok(())
+        }) {
             info!("verify {datastore_name}:{backup_dir_name} - manifest update error: {err}");
             return Ok(false);
         }
-- 
2.47.3

[PATCH proxmox-backup v2 4/5] verify: s3: retry on transient response body collection errors

[Christian Ebner]

If collecting the response body for the S3 object fetching requests
fail, retry with exponential backoff time and only consider this
as transient fetch error on retry exhaustion. Sending the requests
is already retried by the s3-client, so not to be repeated.

This avoids that an otherwise long running verification which already
downloaded a lot of chunks needs to be redone due to transient
connection issues.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/backup/verify.rs | 102 ++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 39 deletions(-)

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 670a899ca..edfb52a5d 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -2,7 +2,7 @@ use pbs_config::BackupLockGuard;
 use std::collections::HashSet;
 use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
-use std::time::Instant;
+use std::time::{Duration, Instant};
 
 use anyhow::{Error, format_err};
 use http_body_util::BodyExt;
@@ -309,48 +309,72 @@ impl VerifyWorker {
             },
             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)) => {
-                        match proxmox_async::runtime::block_on(response.content.collect()) {
-                            Ok(raw_chunk) => {
-                                match DataBlob::from_raw(raw_chunk.to_bytes().to_vec()) {
-                                    Ok(chunk) => {
-                                        let size = info.size();
-                                        verify_state
-                                            .read_bytes
-                                            .fetch_add(chunk.raw_size(), Ordering::SeqCst);
-                                        decoder_pool.send((chunk, info.digest, size))?;
-                                        verify_state
-                                            .decoded_bytes
-                                            .fetch_add(size, Ordering::SeqCst);
-                                    }
-                                    Err(err) => Self::add_corrupt_chunk(
-                                        verify_state,
-                                        info.digest,
-                                        &format!(
-                                            "can't verify chunk with digest {} - {err}",
-                                            hex::encode(info.digest)
+
+                for retry in 0..3 {
+                    match proxmox_async::runtime::block_on(s3_client.get_object(object_key.clone()))
+                    {
+                        Ok(Some(response)) => {
+                            match proxmox_async::runtime::block_on(response.content.collect()) {
+                                Ok(raw_chunk) => {
+                                    match DataBlob::from_raw(raw_chunk.to_bytes().to_vec()) {
+                                        Ok(chunk) => {
+                                            let size = info.size();
+                                            verify_state
+                                                .read_bytes
+                                                .fetch_add(chunk.raw_size(), Ordering::SeqCst);
+                                            decoder_pool.send((chunk, info.digest, size))?;
+                                            verify_state
+                                                .decoded_bytes
+                                                .fetch_add(size, Ordering::SeqCst);
+                                        }
+                                        Err(err) => Self::add_corrupt_chunk(
+                                            verify_state,
+                                            info.digest,
+                                            &format!(
+                                                "can't verify chunk with digest {} - {err}",
+                                                hex::encode(info.digest)
+                                            ),
                                         ),
-                                    ),
+                                    }
+                                    break;
+                                }
+                                Err(err) => {
+                                    if retry < 2 {
+                                        warn!(
+                                            "retry reading chunk with digest {}, load failed - {err}",
+                                            hex::encode(info.digest),
+                                        );
+                                        let backoff = Duration::from_secs(3_u64.pow(retry));
+                                        std::thread::sleep(backoff);
+                                    } else {
+                                        verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
+                                        error!(
+                                            "can't verify chunk with digest {}, load failed - {err}",
+                                            hex::encode(info.digest),
+                                        );
+                                    }
                                 }
-                            }
-                            Err(err) => {
-                                verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
-                                error!("can't verify chunk, load failed - {err}");
                             }
                         }
-                    }
-                    Ok(None) => Self::add_corrupt_chunk(
-                        verify_state,
-                        info.digest,
-                        &format!(
-                            "can't verify missing chunk with digest {}",
-                            hex::encode(info.digest)
-                        ),
-                    ),
-                    Err(err) => {
-                        verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
-                        error!("can't verify chunk, load failed - {err}");
+                        Ok(None) => {
+                            Self::add_corrupt_chunk(
+                                verify_state,
+                                info.digest,
+                                &format!(
+                                    "can't verify missing chunk with digest {}",
+                                    hex::encode(info.digest)
+                                ),
+                            );
+                            break;
+                        }
+                        Err(err) => {
+                            verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst);
+                            error!(
+                                "can't verify chunk with digest {}, load failed - {err}",
+                                hex::encode(info.digest),
+                            );
+                            break;
+                        }
                     }
                 }
             }
-- 
2.47.3

[PATCH proxmox-backup v2 5/5] verify: s3: early exit on chunk fetch or verification errors

[Christian Ebner]

If verification fails due to a transient error on S3 backed
datastores, it is better to skip any further verification on the
same snapshot to save useless API requests and runtime, the final
verification state is already known anyway. This further avoids
blocking for a very long time if the S3 API endpoint is not
reachable.

For permanent error continue, since any bad chunk is best marked
as such and must then be uploaded again, so it is best to check all
of them.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/backup/verify.rs | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index edfb52a5d..15293403b 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -12,9 +12,9 @@ use proxmox_parallel_handler::{ParallelHandler, SendHandle};
 use proxmox_worker_task::WorkerTaskContext;
 
 use pbs_api_types::{
-    ArchiveType, Authid, BackupNamespace, BackupType, CryptMode, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_VERIFY, SnapshotVerifyState, UPID, VerifyState, print_ns_and_snapshot,
-    print_store_and_ns,
+    ArchiveType, Authid, BackupNamespace, BackupType, CryptMode, DatastoreBackendType,
+    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_VERIFY, SnapshotVerifyState, UPID, VerifyState,
+    print_ns_and_snapshot, print_store_and_ns,
 };
 use pbs_datastore::backup_info::{BackupDir, BackupGroup, BackupInfo};
 use pbs_datastore::index::{ChunkReadInfo, IndexFile};
@@ -233,6 +233,15 @@ impl VerifyWorker {
                 .fail_on_shutdown()
                 .map_err(VerificationError::Transient)?;
 
+            if self.datastore.backend_type() == DatastoreBackendType::S3
+                && verify_state.fetch_errors.load(Ordering::SeqCst) > 0
+            {
+                // early abort on transient errors for S3, avoiding useless requests and
+                // protect against stalling if S3 endpoint cannot be reached.
+                warn!("verification failed because of transient errors");
+                break;
+            }
+
             let info = index.chunk_info(pos).unwrap();
 
             // we must always recheck this here, the parallel worker below alter it!
@@ -579,6 +588,11 @@ impl VerifyWorker {
                         "verify {datastore_name}:{backup_dir_name}/{file_name} failed with transient error: {err}",
                         file_name = info.filename,
                     );
+
+                    if self.datastore.backend_type() == DatastoreBackendType::S3 {
+                        warn!("early aborting verification for {datastore_name}:{backup_dir_name}");
+                        break;
+                    }
                 }
             }
         }
-- 
2.47.3