[pbs-devel] [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup

[pbs-devel] [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup

[Stefan Reiter]

Commit 9070d11f4c26 introduced this change for other call sites,
assuming it is correct, this one was missed.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/api2/backup.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index 8b2b0e1a..626c603f 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -216,7 +216,7 @@ async move {
                 (Ok(_), Err(err)) => {
                     env.log(format!("backup ended and finish failed: {}", err));
                     env.log("removing unfinished backup");
-                    env.remove_backup()?;
+                    tools::runtime::block_in_place(|| env.remove_backup())?;
                     Err(err)
                 },
                 (Err(err), Err(_)) => {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] datastore: cleanup open and load config only once

[Stefan Reiter]

Force consumers to use the lookup_datastore method instead of
potentially opening a datastore twice, and pass the config we have
already loaded into open_with_path, removing the need for open(1).

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/backup/datastore.rs | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 46039576..a856e2f4 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -46,17 +46,18 @@ impl DataStore {
 
         let (config, _digest) = datastore::config()?;
         let config: datastore::DataStoreConfig = config.lookup("datastore", name)?;
+        let path = PathBuf::from(&config.path);
 
         let mut map = DATASTORE_MAP.lock().unwrap();
 
         if let Some(datastore) = map.get(name) {
             // Compare Config - if changed, create new Datastore object!
-            if datastore.chunk_store.base == PathBuf::from(&config.path) {
+            if datastore.chunk_store.base == path {
                 return Ok(datastore.clone());
             }
         }
 
-        let datastore = DataStore::open(name)?;
+        let datastore = DataStore::open_with_path(name, &path, config)?;
 
         let datastore = Arc::new(datastore);
         map.insert(name.to_string(), datastore.clone());
@@ -64,18 +65,7 @@ impl DataStore {
         Ok(datastore)
     }
 
-    pub fn open(store_name: &str) -> Result<Self, Error> {
-
-        let (config, _digest) = datastore::config()?;
-        let (_, store_config) = config.sections.get(store_name)
-            .ok_or(format_err!("no such datastore '{}'", store_name))?;
-
-        let path = store_config["path"].as_str().unwrap();
-
-        Self::open_with_path(store_name, Path::new(path))
-    }
-
-    pub fn open_with_path(store_name: &str, path: &Path) -> Result<Self, Error> {
+    fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
         let gc_status = GarbageCollectionStatus::default();
-- 
2.20.1

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

[Stefan Reiter]

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

[pbs-devel] applied: [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup

[Dietmar Maurer]

applied

[pbs-devel] applied: [PATCH proxmox-backup 2/3] datastore: cleanup open and load config only once

[Dietmar Maurer]

applied

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

[Dietmar Maurer]

> +            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

Instead, I would keepä the lock and use/expose "verify_backup_dir_no_lock()". Is that
feasible?


> +                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()) {

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

[Fabian Grünbichler]

On October 20, 2020 8:12 am, Dietmar Maurer wrote:
> 
>> +            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
> 
> Instead, I would keepä the lock and use/expose "verify_backup_dir_no_lock()". Is that
> feasible?

it will need a v2 in any case, since patch #2 without #3 applied as well 
does not build. applied the following fixup:


diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index a856e2f4..d8e7a794 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;
@@ -65,7 +65,7 @@ impl DataStore {
         Ok(datastore)
     }
 
-    fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
+    fn open_with_path(store_name: &str, path: &Path, _config: DataStoreConfig) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
         let gc_status = GarbageCollectionStatus::default();



please check that your series builds properly for every single commit, 
e.g. with `git rebase origin/master --exec "cargo build"`

> 
> 
>> +                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()) {
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>