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

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

[Hannes Laimer]

v2, based on Dominik Csapak <d.csapak@proxmox.com>'s feedback:
 - new function is now the filter, instead of returning a closure
 - used extract_output_format in the binary


Moved the snapshot filter into a function, so verify jobs and 
the datastore verify endpoint don't have duplicated code. Also adds
--ignore-verified and --outdated-after to the CLI and api2 endpoint for
verifing datastores.
It might make sense to also make those option avaiable in the WebUI.

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

 src/api2/admin/datastore.rs       | 28 +++++++++++++++++++++++---
 src/backup/verify.rs              | 27 +++++++++++++++++++++++++
 src/bin/proxmox-backup-manager.rs | 14 ++++++++++---
 src/server/verify_job.rs          | 33 +++++++++----------------------
 4 files changed, 72 insertions(+), 30 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH v2 proxmox-backup 1/3] close #3459: verify-job: move snapshot filter into function

[Hannes Laimer]

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

diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index a1b1e6dd..1a51d3fb 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -575,3 +575,30 @@ pub fn verify_all_backups(
 
     Ok(errors)
 }
+
+/// Filter for the verification of snapshots
+pub fn verify_filter(
+    ignore_verified_snapshots: bool,
+    outdated_after: Option<i64>,
+    manifest: &BackupManifest,
+) -> bool {
+    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..878fade5 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,
+        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!("{}:{}",
@@ -68,7 +46,14 @@ 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) => {
-- 
2.20.1

Re: [pbs-devel] [PATCH v2 proxmox-backup 1/3] close #3459: verify-job: move snapshot filter into function

[Thomas Lamprecht]

On 22.06.21 09:56, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---

just a general observation: this alone does not closes #3459, so not quite correct
to have that in the commit messages' subject.
Preparatory commits, should rather note so in the commit messages' body,
"preparatory for a fix for #3459"

For partial fixes, i.e., where the patch is part of the actual fix but not enough
on its own, its common to use "partially closes #XYZ"

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

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
When using a schema ref it is not possible to omit the Option<_> around
parameters, even if the referenced schema would contain a default value.
What would be possible is to remove the schema ref and put the default
directly instead, but that didn't make a lot of sense to me.

 src/api2/admin/datastore.rs | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 7b7f3102..b65c12ad 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;
@@ -719,7 +730,9 @@ pub fn verify(
                     &verify_worker,
                     &backup_dir,
                     worker.upid().clone(),
-                    None,
+                    Some(&move |manifest| {
+                        verify_filter(ignore_verified, outdated_after, manifest)
+                    }),
                 )? {
                     res.push(backup_dir.to_string());
                 }
@@ -730,7 +743,9 @@ pub fn verify(
                     &backup_group,
                     &mut StoreProgress::new(1),
                     worker.upid(),
-                    None,
+                    Some(&move |manifest| {
+                        verify_filter(ignore_verified, outdated_after, manifest)
+                    }),
                 )?;
                 failed_dirs
             } else {
@@ -743,7 +758,14 @@ pub fn verify(
                     None
                 };
 
-                verify_all_backups(&verify_worker, worker.upid(), owner, None)?
+                verify_all_backups(
+                    &verify_worker,
+                    worker.upid(),
+                    owner,
+                    Some(&move |manifest| {
+                        verify_filter(ignore_verified, outdated_after, manifest)
+                    }),
+                )?
             };
             if !failed_dirs.is_empty() {
                 worker.log("Failed to verify the following snapshots/groups:");
-- 
2.20.1

[pbs-devel] [PATCH v2 proxmox-backup 3/3] close #3459: 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 | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index c3806a31..fa5e277d 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,
@@ -279,14 +287,14 @@ async fn pull_datastore(
 /// Verify backups
 async fn verify(
     store: String,
-    param: Value,
+    mut param: Value,
 ) -> Result<Value, Error> {
 
-    let output_format = get_output_format(&param);
+    let output_format = extract_output_format(&mut param);
 
     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 v2 proxmox-backup 0/3] close #3459: add --ignore-verified and --outdated-after to CLI and api2

[Dominik Csapak]

looks good (aside from the commit message note from
Thomas), works as advertised

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>
Tested-By: Dominik Csapak <d.csapak@proxmox.com>

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

[Thomas Lamprecht]

On 22.06.21 09:56, Hannes Laimer wrote:
> v2, based on Dominik Csapak <d.csapak@proxmox.com>'s feedback:
>  - new function is now the filter, instead of returning a closure
>  - used extract_output_format in the binary
> 
> 
> Moved the snapshot filter into a function, so verify jobs and 
> the datastore verify endpoint don't have duplicated code. Also adds
> --ignore-verified and --outdated-after to the CLI and api2 endpoint for
> verifing datastores.
> It might make sense to also make those option avaiable in the WebUI.
> 
> Hannes Laimer (3):
>   close #3459: verify-job: move snapshot filter into function
>   close #3459: api2: add ignore-verified and outdated-after to datastore
>     verify endpoint
>   close: #3459: manager_bin: add --ignore-verified and --outdated-after
>     parameters
> 
>  src/api2/admin/datastore.rs       | 28 +++++++++++++++++++++++---
>  src/backup/verify.rs              | 27 +++++++++++++++++++++++++
>  src/bin/proxmox-backup-manager.rs | 14 ++++++++++---
>  src/server/verify_job.rs          | 33 +++++++++----------------------
>  4 files changed, 72 insertions(+), 30 deletions(-)
> 



applied, thanks! Adapted  some comment messages do not falsely state to fix
an bugzilla entry they do not actively fix and added Dominik's T-b/R-b tags.