[pbs-devel] [PATCH proxmox-backup v6 1/4] snapshot: add helper function to retrieve verify_state

[Gabriel Goller <g.goller@proxmox.com>]

Add helper functions to retrieve the verify_state from the manifest of a
snapshot. Replaced all the manual "verify_state" parsing with the helper
function.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-datastore/src/backup_info.rs |  9 +++++++--
 pbs-datastore/src/manifest.rs    | 14 +++++++++++++-
 src/api2/admin/datastore.rs      | 16 +++++++---------
 src/api2/backup/mod.rs           | 18 +++++++++++-------
 src/backup/verify.rs             | 13 ++++++++-----
 5 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 62d12b1183df..a581d75757b4 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -8,8 +8,8 @@ use anyhow::{bail, format_err, Error};
 use proxmox_sys::fs::{lock_dir_noblock, replace_file, CreateOptions};
 
 use pbs_api_types::{
-    Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, GroupFilter, BACKUP_DATE_REGEX,
-    BACKUP_FILE_REGEX,
+    Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, GroupFilter, VerifyState,
+    BACKUP_DATE_REGEX, BACKUP_FILE_REGEX,
 };
 use pbs_config::{open_backup_lockfile, BackupLockGuard};
 
@@ -555,6 +555,11 @@ impl BackupDir {
 
         Ok(())
     }
+
+    /// Load the verify state from the manifest.
+    pub fn verify_state(&self) -> Result<Option<VerifyState>, anyhow::Error> {
+        Ok(self.load_manifest()?.0.verify_state()?.map(|svs| svs.state))
+    }
 }
 
 impl AsRef<pbs_api_types::BackupNamespace> for BackupDir {
diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index c3df014272a0..3013fab97221 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -5,7 +5,7 @@ use anyhow::{bail, format_err, Error};
 use serde::{Deserialize, Serialize};
 use serde_json::{json, Value};
 
-use pbs_api_types::{BackupType, CryptMode, Fingerprint};
+use pbs_api_types::{BackupType, CryptMode, Fingerprint, SnapshotVerifyState};
 use pbs_tools::crypt_config::CryptConfig;
 
 pub const MANIFEST_BLOB_NAME: &str = "index.json.blob";
@@ -242,6 +242,18 @@ impl BackupManifest {
         let manifest: BackupManifest = serde_json::from_value(json)?;
         Ok(manifest)
     }
+
+    /// Get the verify state of the snapshot
+    ///
+    /// Note: New snapshots, which have not been verified yet, do not have a status and this
+    /// function will return `Ok(None)`.
+    pub fn verify_state(&self) -> Result<Option<SnapshotVerifyState>, anyhow::Error> {
+        let verify = self.unprotected["verify_state"].clone();
+        if verify.is_null() {
+            return Ok(None);
+        }
+        Ok(Some(serde_json::from_value::<SnapshotVerifyState>(verify)?))
+    }
 }
 
 impl TryFrom<super::DataBlob> for BackupManifest {
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 99b579f02c50..3624dba41199 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -537,15 +537,13 @@ unsafe fn list_snapshots_blocking(
                     }
                 };
 
-                let verification = manifest.unprotected["verify_state"].clone();
-                let verification: Option<SnapshotVerifyState> =
-                    match serde_json::from_value(verification) {
-                        Ok(verify) => verify,
-                        Err(err) => {
-                            eprintln!("error parsing verification state : '{}'", err);
-                            None
-                        }
-                    };
+                let verification: Option<SnapshotVerifyState> = match manifest.verify_state() {
+                    Ok(verify) => verify,
+                    Err(err) => {
+                        eprintln!("error parsing verification state : '{}'", err);
+                        None
+                    }
+                };
 
                 let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 63c49f6537f7..dce40e5318b8 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -8,6 +8,7 @@ use hyper::http::request::Parts;
 use hyper::{Body, Request, Response, StatusCode};
 use serde::Deserialize;
 use serde_json::{json, Value};
+use tracing::warn;
 
 use proxmox_rest_server::{H2Service, WorkerTask};
 use proxmox_router::{http_err, list_subdirs_api_method};
@@ -19,9 +20,9 @@ use proxmox_sortable_macro::sortable;
 use proxmox_sys::fs::lock_dir_noblock_shared;
 
 use pbs_api_types::{
-    Authid, BackupNamespace, BackupType, Operation, SnapshotVerifyState, VerifyState,
-    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
-    BACKUP_TYPE_SCHEMA, CHUNK_DIGEST_SCHEMA, DATASTORE_SCHEMA, PRIV_DATASTORE_BACKUP,
+    Authid, BackupNamespace, BackupType, Operation, VerifyState, BACKUP_ARCHIVE_NAME_SCHEMA,
+    BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
+    CHUNK_DIGEST_SCHEMA, DATASTORE_SCHEMA, PRIV_DATASTORE_BACKUP,
 };
 use pbs_config::CachedUserInfo;
 use pbs_datastore::index::IndexFile;
@@ -159,15 +160,18 @@ fn upgrade_to_backup_protocol(
             let info = backup_group.last_backup(true).unwrap_or(None);
             if let Some(info) = info {
                 let (manifest, _) = info.backup_dir.load_manifest()?;
-                let verify = manifest.unprotected["verify_state"].clone();
-                match serde_json::from_value::<SnapshotVerifyState>(verify) {
-                    Ok(verify) => match verify.state {
+                match manifest.verify_state() {
+                    Ok(Some(verify)) => match verify.state {
                         VerifyState::Ok => Some(info),
                         VerifyState::Failed => None,
                     },
-                    Err(_) => {
+                    Ok(None) => {
                         // no verify state found, treat as valid
                         Some(info)
+                    },
+                    Err(err) => {
+                        warn!("error parsing the snapshot manifest: {err:#}");
+                        Some(info)
                     }
                 }
             } else {
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 6ef7e8eb3ebb..c1abe69a4fde 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -5,7 +5,7 @@ use std::time::Instant;
 
 use anyhow::{bail, format_err, Error};
 use nix::dir::Dir;
-use tracing::{error, info};
+use tracing::{error, info, warn};
 
 use proxmox_sys::fs::lock_dir_noblock_shared;
 use proxmox_worker_task::WorkerTaskContext;
@@ -553,10 +553,13 @@ pub fn verify_filter(
         return true;
     }
 
-    let raw_verify_state = manifest.unprotected["verify_state"].clone();
-    match serde_json::from_value::<SnapshotVerifyState>(raw_verify_state) {
-        Err(_) => true, // no last verification, always include
-        Ok(last_verify) => {
+    match manifest.verify_state() {
+        Err(err) => {
+            warn!("error reading manifest: {err:#}");
+            true
+        }
+        Ok(None) => true, // no last verification, always include
+        Ok(Some(last_verify)) => {
             match outdated_after {
                 None => false, // never re-verify if ignored and no max age
                 Some(max_age) => {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel