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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

> Gabriel Goller <g.goller@proxmox.com> hat am 21.11.2024 14:35 CET geschrieben:
> 
>  
> 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 | 15 +++++++++++++--
>  pbs-datastore/src/manifest.rs    | 14 +++++++++++++-
>  src/api2/admin/datastore.rs      | 16 +++++++---------
>  src/api2/backup/mod.rs           | 13 ++++++-------
>  src/backup/verify.rs             |  7 +++----
>  5 files changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 62d12b1183df..2d8e0a6d92da 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,17 @@ impl BackupDir {
>  
>          Ok(())
>      }
> +
> +    /// Load the verify state from the manifest.
> +    pub fn verify_state(&self) -> Result<Option<VerifyState>, anyhow::Error> {
> +        let manifest = self.load_manifest()?;
> +        Ok(manifest
> +            .0
> +            .verify_state()
> +            .ok()
> +            .flatten()
> +            .map(|svs| svs.state))

this still looks slightly wrong to me - if verify_state() returns an error, it's mapped to None (by the call to `ok()`), which would hide an inner parse error for the verification state?

I think the following should be correctly bubble up errors when loading the manifest or when parsing the contained verify state while returning Ok(None) if no state is contained in the manifest:

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)?))

this looks good to me now! :)

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

this as well!

>  
>                  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 ea0d0292ec58..605c75e2dfa9 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -19,9 +19,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,13 +159,12 @@ 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) | Err(_) => {
>                          // no verify state found, treat as valid

this as well, although it might make sense to log this here as well (pre-existing)

>                          Some(info)
>                      }
> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> index 6ef7e8eb3ebb..20c605c4dde6 100644
> --- a/src/backup/verify.rs
> +++ b/src/backup/verify.rs
> @@ -553,10 +553,9 @@ 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() {
> +        Ok(None) | Err(_) => true, // no last verification, always include

same here! I think/hope the Err path for these should only trigger when somebody messes up manifests, but..

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