[pbs-devel] [PATCH proxmox-backup 3/3] fix #2988: allow verification after finishing a snapshot

[Stefan Reiter <s.reiter@proxmox.com>]

To cater to the paranoid, a new datastore-wide setting "verify-new" is
introduced. When set, a verify job will be spawned right after a new
backup is added to the store (only verifying the added snapshot).

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Do we want this in the GUI as well? Would be easy to do ofc, but not sure we
want to expose this so readily, would give the feeling that it's necessary IMO.

 src/api2/backup.rs             | 15 ++++++++++++
 src/api2/backup/environment.rs | 44 +++++++++++++++++++++++++++++++++-
 src/backup/datastore.rs        | 12 ++++++++--
 src/config/datastore.rs        |  7 ++++++
 4 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index 626c603f..a8a8a4b7 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -203,14 +203,29 @@ async move {
                 tools::runtime::block_in_place(|| env.remove_backup())?;
                 return Ok(());
             }
+
+            let verify = |env: BackupEnvironment| {
+                // we are done, so we can safely drop the snapshot guard early
+                // to allow the verify task to acquire it for itself
+                std::mem::drop(_snap_guard);
+                if let Err(err) = env.verify_after_complete() {
+                    env.log(format!(
+                        "backup finished, but starting the requested verify task failed: {}",
+                        err
+                    ));
+                }
+            };
+
             match (res, env.ensure_finished()) {
                 (Ok(_), Ok(())) => {
                     env.log("backup finished successfully");
+                    verify(env);
                     Ok(())
                 },
                 (Err(err), Ok(())) => {
                     // ignore errors after finish
                     env.log(format!("backup had errors but finished: {}", err));
+                    verify(env);
                     Ok(())
                 },
                 (Ok(_), Err(err)) => {
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index c4f166df..316accdc 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -1,6 +1,6 @@
 use anyhow::{bail, format_err, Error};
 use std::sync::{Arc, Mutex};
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use ::serde::{Serialize};
 use serde_json::{json, Value};
@@ -494,6 +494,48 @@ impl BackupEnvironment {
         Ok(())
     }
 
+    pub fn verify_after_complete(&self) -> Result<(), Error> {
+        if !self.datastore.verify_new() {
+            // no verify requested, do nothing
+            return Ok(());
+        }
+
+        let worker_id = format!("{}_{}_{}_{:08X}",
+            self.datastore.name(),
+            self.backup_dir.group().backup_type(),
+            self.backup_dir.group().backup_id(),
+            self.backup_dir.backup_time());
+
+        let datastore = self.datastore.clone();
+        let backup_dir = self.backup_dir.clone();
+
+        WorkerTask::new_thread(
+            "verify",
+            Some(worker_id),
+            self.user.clone(),
+            false,
+            move |worker| {
+                worker.log("Automatically verifying newly added snapshot");
+
+                let verified_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024*16)));
+                let corrupt_chunks = Arc::new(Mutex::new(HashSet::with_capacity(64)));
+
+                if !verify_backup_dir(
+                    datastore,
+                    &backup_dir,
+                    verified_chunks,
+                    corrupt_chunks,
+                    worker.clone(),
+                    worker.upid().clone(),
+                )? {
+                    bail!("verification failed - please check the log for details");
+                }
+
+                Ok(())
+            },
+        ).map(|_| ())
+    }
+
     pub fn log<S: AsRef<str>>(&self, msg: S) {
         self.worker.log(msg);
     }
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index a856e2f4..2389cc6c 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -18,7 +18,7 @@ use super::fixed_index::{FixedIndexReader, FixedIndexWriter};
 use super::manifest::{MANIFEST_BLOB_NAME, MANIFEST_LOCK_NAME, CLIENT_LOG_BLOB_NAME, BackupManifest};
 use super::index::*;
 use super::{DataBlob, ArchiveType, archive_type};
-use crate::config::datastore;
+use crate::config::datastore::{self, DataStoreConfig};
 use crate::task::TaskState;
 use crate::tools;
 use crate::tools::format::HumanByte;
@@ -38,6 +38,7 @@ pub struct DataStore {
     chunk_store: Arc<ChunkStore>,
     gc_mutex: Mutex<bool>,
     last_gc_status: Mutex<GarbageCollectionStatus>,
+    verify_new: bool,
 }
 
 impl DataStore {
@@ -52,7 +53,9 @@ impl DataStore {
 
         if let Some(datastore) = map.get(name) {
             // Compare Config - if changed, create new Datastore object!
-            if datastore.chunk_store.base == path {
+            if datastore.chunk_store.base == path &&
+                datastore.verify_new == config.verify_new.unwrap_or(false)
+            {
                 return Ok(datastore.clone());
             }
         }
@@ -74,6 +77,7 @@ impl DataStore {
             chunk_store: Arc::new(chunk_store),
             gc_mutex: Mutex::new(false),
             last_gc_status: Mutex::new(gc_status),
+            verify_new: config.verify_new.unwrap_or(false),
         })
     }
 
@@ -680,4 +684,8 @@ impl DataStore {
 
         Ok(())
     }
+
+    pub fn verify_new(&self) -> bool {
+        self.verify_new
+    }
 }
diff --git a/src/config/datastore.rs b/src/config/datastore.rs
index aaf977a7..943364fd 100644
--- a/src/config/datastore.rs
+++ b/src/config/datastore.rs
@@ -72,6 +72,10 @@ pub const DIR_NAME_SCHEMA: Schema = StringSchema::new("Directory name").schema()
             optional: true,
             schema: PRUNE_SCHEMA_KEEP_YEARLY,
         },
+        "verify-new": {
+            optional: true,
+            type: bool,
+        },
     }
 )]
 #[serde(rename_all="kebab-case")]
@@ -100,6 +104,9 @@ pub struct DataStoreConfig {
     pub keep_monthly: Option<u64>,
     #[serde(skip_serializing_if="Option::is_none")]
     pub keep_yearly: Option<u64>,
+    /// If enabled, all backups will be verified right after completion.
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub verify_new: Option<bool>,
 }
 
 fn init() -> SectionConfig {
-- 
2.20.1