[pbs-devel] [PATCH v1 proxmox-backup 0/3] close #3459: add --ignore-verified and --outdated-after to CLI and api2

[pbs-devel] [PATCH v1 proxmox-backup 0/3] close #3459: add --ignore-verified and --outdated-after to CLI and api2

[Hannes Laimer]

Hannes Laimer (3):
  verify-job: move building of snapshot filter into function
  api2: add ignore-verified and outdated-after to datastore verify
    endpoint
  manager_bin: add --ignore-verified and --outdated-after parameters

 src/api2/admin/datastore.rs       | 18 +++++++++++++++---
 src/backup/verify.rs              | 29 +++++++++++++++++++++++++++++
 src/bin/proxmox-backup-manager.rs | 10 +++++++++-
 src/server/verify_job.rs          | 25 ++-----------------------
 4 files changed, 55 insertions(+), 27 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH v1 proxmox-backup 1/3] verify-job: move building of snapshot filter into function

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/backup/verify.rs     | 29 +++++++++++++++++++++++++++++
 src/server/verify_job.rs | 25 ++-----------------------
 2 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index a1b1e6dd..a0b8de58 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -1,5 +1,6 @@
 use nix::dir::Dir;
 use std::collections::HashSet;
+use std::panic::RefUnwindSafe;
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::Instant;
@@ -575,3 +576,31 @@ pub fn verify_all_backups(
 
     Ok(errors)
 }
+
+/// Builds a filter for the verification of snapshots
+pub fn build_verify_filter(
+    ignore_verified_snapshots: bool,
+    outdated_after: Option<i64>,
+) -> Box<dyn std::marker::Sync + RefUnwindSafe + Fn(&BackupManifest) -> bool> {
+    Box::new(move |manifest: &BackupManifest| {
+        if !ignore_verified_snapshots {
+            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 outdated_after {
+                    None => false, // never re-verify if ignored and no max age
+                    Some(max_age) => {
+                        let now = proxmox::tools::time::epoch_i64();
+                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
+
+                        days_since_last_verify > max_age
+                    }
+                }
+            }
+        }
+    })
+}
\ No newline at end of file
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 1dd8baa7..47ac4da4 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -7,7 +7,7 @@ use crate::{
     config::verify::VerificationJobConfig,
     backup::{
         DataStore,
-        BackupManifest,
+        build_verify_filter,
         verify_all_backups,
     },
     task_log,
@@ -26,28 +26,6 @@ pub fn do_verification_job(
     let outdated_after = verification_job.outdated_after;
     let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
 
-    let filter = move |manifest: &BackupManifest| {
-        if !ignore_verified_snapshots {
-            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 outdated_after {
-                    None => false, // never re-verify if ignored and no max age
-                    Some(max_age) => {
-                        let now = proxmox::tools::time::epoch_i64();
-                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
-
-                        days_since_last_verify > max_age
-                    }
-                }
-            }
-        }
-    };
-
     let (email, notify) = crate::server::lookup_datastore_notify_settings(&verification_job.store);
 
     let job_id = format!("{}:{}",
@@ -67,6 +45,7 @@ pub fn do_verification_job(
                 task_log!(worker,"task triggered by schedule '{}'", event_str);
             }
 
+            let filter = build_verify_filter(ignore_verified_snapshots, outdated_after);
             let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore);
             let result = verify_all_backups(&verify_worker, worker.upid(), None, Some(&filter));
             let job_result = match result {
-- 
2.20.1

Re: [pbs-devel] [PATCH v1 proxmox-backup 1/3] verify-job: move building of snapshot filter into function

[Dominik Csapak]

while factoring that out is a good idea, there is no
need to make a builder function that returns a closure,
a much simpler diff is:

--->8---
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index a1b1e6dd..684fde67 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -575,3 +575,30 @@ pub fn verify_all_backups(

      Ok(errors)
  }
+
+/// Filter function for verification of snapshots
+pub fn verify_filter(
+    ignore_verified_snapshots: bool,
+    outdated_after: Option<i64>,
+    manifest: &BackupManifest,
+) -> bool {
+    .... old function
+    }
+}
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 1dd8baa7..85245aca 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -7,8 +7,8 @@ use crate::{
      config::verify::VerificationJobConfig,
      backup::{
          DataStore,
-        BackupManifest,
          verify_all_backups,
+        verify_filter,
      },
      task_log,
  };
      let (email, notify) = 
crate::server::lookup_datastore_notify_settings(&verification_job.store);

      let job_id = format!("{}:{}",
@@ -68,7 +46,12 @@ pub fn do_verification_job(
              }

              let verify_worker = 
crate::backup::VerifyWorker::new(worker.clone(), datastore);
-            let result = verify_all_backups(&verify_worker, 
worker.upid(), None, Some(&filter));
+            let result = verify_all_backups(
+                &verify_worker,
+                worker.upid(),
+                None,
+                Some(&move |manifest| 
verify_filter(ignore_verified_snapshots, outdated_after, manifest)),
+            );
              let job_result = match result {
                  Ok(ref failed_dirs) if failed_dirs.is_empty() => Ok(()),
                  Ok(ref failed_dirs) => {
---8<---
(i omitted the obvious parts)

so simply have the filter function, and build
a small closure when needed

the only change this needs in 2/3 is that
let filter = build....;
must now be let filter = move |manifest| ....;


On 6/11/21 09:50, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/backup/verify.rs     | 29 +++++++++++++++++++++++++++++
>   src/server/verify_job.rs | 25 ++-----------------------
>   2 files changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> index a1b1e6dd..a0b8de58 100644
> --- a/src/backup/verify.rs
> +++ b/src/backup/verify.rs
> @@ -1,5 +1,6 @@
>   use nix::dir::Dir;
>   use std::collections::HashSet;
> +use std::panic::RefUnwindSafe;
>   use std::sync::atomic::{AtomicUsize, Ordering};
>   use std::sync::{Arc, Mutex};
>   use std::time::Instant;
> @@ -575,3 +576,31 @@ pub fn verify_all_backups(
>   
>       Ok(errors)
>   }
> +
> +/// Builds a filter for the verification of snapshots
> +pub fn build_verify_filter(
> +    ignore_verified_snapshots: bool,
> +    outdated_after: Option<i64>,
> +) -> Box<dyn std::marker::Sync + RefUnwindSafe + Fn(&BackupManifest) -> bool> {
> +    Box::new(move |manifest: &BackupManifest| {
> +        if !ignore_verified_snapshots {
> +            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 outdated_after {
> +                    None => false, // never re-verify if ignored and no max age
> +                    Some(max_age) => {
> +                        let now = proxmox::tools::time::epoch_i64();
> +                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
> +
> +                        days_since_last_verify > max_age
> +                    }
> +                }
> +            }
> +        }
> +    })
> +}
> \ No newline at end of file
> diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
> index 1dd8baa7..47ac4da4 100644
> --- a/src/server/verify_job.rs
> +++ b/src/server/verify_job.rs
> @@ -7,7 +7,7 @@ use crate::{
>       config::verify::VerificationJobConfig,
>       backup::{
>           DataStore,
> -        BackupManifest,
> +        build_verify_filter,
>           verify_all_backups,
>       },
>       task_log,
> @@ -26,28 +26,6 @@ pub fn do_verification_job(
>       let outdated_after = verification_job.outdated_after;
>       let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
>   
> -    let filter = move |manifest: &BackupManifest| {
> -        if !ignore_verified_snapshots {
> -            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 outdated_after {
> -                    None => false, // never re-verify if ignored and no max age
> -                    Some(max_age) => {
> -                        let now = proxmox::tools::time::epoch_i64();
> -                        let days_since_last_verify = (now - last_verify.upid.starttime) / 86400;
> -
> -                        days_since_last_verify > max_age
> -                    }
> -                }
> -            }
> -        }
> -    };
> -
>       let (email, notify) = crate::server::lookup_datastore_notify_settings(&verification_job.store);
>   
>       let job_id = format!("{}:{}",
> @@ -67,6 +45,7 @@ pub fn do_verification_job(
>                   task_log!(worker,"task triggered by schedule '{}'", event_str);
>               }
>   
> +            let filter = build_verify_filter(ignore_verified_snapshots, outdated_after);
>               let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore);
>               let result = verify_all_backups(&verify_worker, worker.upid(), None, Some(&filter));
>               let job_result = match result {
> 

[pbs-devel] [PATCH v1 proxmox-backup 2/3] api2: add ignore-verified and outdated-after to datastore verify endpoint

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/admin/datastore.rs | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index e0dfeecc..13830298 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -646,6 +646,14 @@ pub fn status(
                 schema: BACKUP_ID_SCHEMA,
                 optional: true,
             },
+            "ignore-verified": {
+                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,
+                optional: true,
+            },
+            "outdated-after": {
+                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
+                optional: true,
+            },
             "backup-time": {
                 schema: BACKUP_TIME_SCHEMA,
                 optional: true,
@@ -668,9 +676,12 @@ pub fn verify(
     backup_type: Option<String>,
     backup_id: Option<String>,
     backup_time: Option<i64>,
+    ignore_verified: Option<bool>,
+    outdated_after: Option<i64>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
+    let ignore_verified = ignore_verified.unwrap_or(true);
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let worker_id;
@@ -712,6 +723,7 @@ pub fn verify(
         auth_id.clone(),
         to_stdout,
         move |worker| {
+            let filter = build_verify_filter(ignore_verified, outdated_after);
             let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore);
             let failed_dirs = if let Some(backup_dir) = backup_dir {
                 let mut res = Vec::new();
@@ -719,7 +731,7 @@ pub fn verify(
                     &verify_worker,
                     &backup_dir,
                     worker.upid().clone(),
-                    None,
+                    Some(&filter),
                 )? {
                     res.push(backup_dir.to_string());
                 }
@@ -730,7 +742,7 @@ pub fn verify(
                     &backup_group,
                     &mut StoreProgress::new(1),
                     worker.upid(),
-                    None,
+                    Some(&filter),
                 )?;
                 failed_dirs
             } else {
@@ -743,7 +755,7 @@ pub fn verify(
                     None
                 };
 
-                verify_all_backups(&verify_worker, worker.upid(), owner, None)?
+                verify_all_backups(&verify_worker, worker.upid(), owner, Some(&filter))?
             };
             if !failed_dirs.is_empty() {
                 worker.log("Failed to verify the following snapshots/groups:");
-- 
2.20.1

Re: [pbs-devel] [PATCH v1 proxmox-backup 2/3] api2: add ignore-verified and outdated-after to datastore verify endpoint

[Dominik Csapak]

comment(s) inline

On 6/11/21 09:50, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index e0dfeecc..13830298 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -646,6 +646,14 @@ pub fn status(
>                   schema: BACKUP_ID_SCHEMA,
>                   optional: true,
>               },
> +            "ignore-verified": {
> +                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,
> +                optional: true,

if we add a 'default: true' here, we can
|
v

> +            },
> +            "outdated-after": {
> +                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +                optional: true,
> +            },
>               "backup-time": {
>                   schema: BACKUP_TIME_SCHEMA,
>                   optional: true,
> @@ -668,9 +676,12 @@ pub fn verify(
>       backup_type: Option<String>,
>       backup_id: Option<String>,
>       backup_time: Option<i64>,
> +    ignore_verified: Option<bool>,

omit the Option here and simply make that a bool then
|
v

> +    outdated_after: Option<i64>,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
> +    let ignore_verified = ignore_verified.unwrap_or(true);

this line can be removed

>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let worker_id;
> @@ -712,6 +723,7 @@ pub fn verify(
>           auth_id.clone(),
>           to_stdout,
>           move |worker| {
> +            let filter = build_verify_filter(ignore_verified, outdated_after);
>               let verify_worker = crate::backup::VerifyWorker::new(worker.clone(), datastore);
>               let failed_dirs = if let Some(backup_dir) = backup_dir {
>                   let mut res = Vec::new();
> @@ -719,7 +731,7 @@ pub fn verify(
>                       &verify_worker,
>                       &backup_dir,
>                       worker.upid().clone(),
> -                    None,
> +                    Some(&filter),
>                   )? {
>                       res.push(backup_dir.to_string());
>                   }
> @@ -730,7 +742,7 @@ pub fn verify(
>                       &backup_group,
>                       &mut StoreProgress::new(1),
>                       worker.upid(),
> -                    None,
> +                    Some(&filter),
>                   )?;
>                   failed_dirs
>               } else {
> @@ -743,7 +755,7 @@ pub fn verify(
>                       None
>                   };
>   
> -                verify_all_backups(&verify_worker, worker.upid(), owner, None)?
> +                verify_all_backups(&verify_worker, worker.upid(), owner, Some(&filter))?
>               };
>               if !failed_dirs.is_empty() {
>                   worker.log("Failed to verify the following snapshots/groups:");
> 

[pbs-devel] [PATCH v1 proxmox-backup 3/3] manager_bin: add --ignore-verified and --outdated-after parameters

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox-backup-manager.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index c3806a31..e0baa20d 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -269,6 +269,14 @@ async fn pull_datastore(
             "store": {
                 schema: DATASTORE_SCHEMA,
             },
+            "ignore-verified": {
+                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,
+                optional: true,
+            },
+            "outdated-after": {
+                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
+                optional: true,
+            },
             "output-format": {
                 schema: OUTPUT_FORMAT,
                 optional: true,
@@ -286,7 +294,7 @@ async fn verify(
 
     let mut client = connect_to_localhost()?;
 
-    let args = json!({});
+    let args = json!(param);
 
     let path = format!("api2/json/admin/datastore/{}/verify", store);
 
-- 
2.20.1

Re: [pbs-devel] [PATCH v1 proxmox-backup 3/3] manager_bin: add --ignore-verified and --outdated-after parameters

[Dominik Csapak]

comment inline:

On 6/11/21 09:50, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/bin/proxmox-backup-manager.rs | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
> index c3806a31..e0baa20d 100644
> --- a/src/bin/proxmox-backup-manager.rs
> +++ b/src/bin/proxmox-backup-manager.rs
> @@ -269,6 +269,14 @@ async fn pull_datastore(
>               "store": {
>                   schema: DATASTORE_SCHEMA,
>               },
> +            "ignore-verified": {
> +                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,
> +                optional: true,
> +            },
> +            "outdated-after": {
> +                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +                optional: true,
> +            },
>               "output-format": {
>                   schema: OUTPUT_FORMAT,
>                   optional: true,
> @@ -286,7 +294,7 @@ async fn verify(
>   
>       let mut client = connect_to_localhost()?;
>   
> -    let args = json!({});
> +    let args = json!(param);
>   

caution, this still potentially contains the 'output-format' parameter
which will not work with the api, so we have to remove it here
(or use the 'extract_output_format' instead above)

>       let path = format!("api2/json/admin/datastore/{}/verify", store);
>   
>