* [pbs-devel] [PATCH proxmox-backup 2/3] datastore: cleanup open and load config only once
2020-10-19 14:45 [pbs-devel] [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup Stefan Reiter
@ 2020-10-19 14:45 ` Stefan Reiter
2020-10-20 5:54 ` [pbs-devel] applied: " Dietmar Maurer
2020-10-19 14:45 ` [pbs-devel] [PATCH proxmox-backup 3/3] fix #2988: allow verification after finishing a snapshot Stefan Reiter
2020-10-20 5:53 ` [pbs-devel] applied: [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup Dietmar Maurer
2 siblings, 1 reply; 7+ messages in thread
From: Stefan Reiter @ 2020-10-19 14:45 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [pbs-devel] [PATCH proxmox-backup 3/3] fix #2988: allow verification after finishing a snapshot
2020-10-19 14:45 [pbs-devel] [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup Stefan Reiter
2020-10-19 14:45 ` [pbs-devel] [PATCH proxmox-backup 2/3] datastore: cleanup open and load config only once Stefan Reiter
@ 2020-10-19 14:45 ` Stefan Reiter
2020-10-20 6:12 ` Dietmar Maurer
2020-10-20 5:53 ` [pbs-devel] applied: [PATCH proxmox-backup 1/3] fix missing block_in_place for remove_backup Dietmar Maurer
2 siblings, 1 reply; 7+ messages in thread
From: Stefan Reiter @ 2020-10-19 14:45 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread