[pbs-devel] [PATCH backup 1/2] save last verify result in snapshot manifest

[pbs-devel] [PATCH backup 1/2] save last verify result in snapshot manifest

[Thomas Lamprecht]

Save the state ("ok" or "failed") and the UPID of the respective
verify task. With this we can easily allow to open  the relevant task
log and show when the last verify happened.

As we already load the manifest when listing the snapshots, just add
it there directly.

We need to make the verify API call protected to be able to save the manifest.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

One thing I'm not 100% happy with is the serde_json::to_value(manifest) on
storing it again. But I did not wanted to introduce an extra method to store a
manifest derived from the BackupManifest type directly just for this.

 src/api2/admin/datastore.rs | 16 ++++++++++++++--
 src/api2/types/mod.rs       | 27 +++++++++++++++++++++++++++
 src/backup/verify.rs        | 17 +++++++++++++++--
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 7e811de9..6a1e0d63 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -361,7 +361,7 @@ pub fn list_snapshots (
 
         let mut size = None;
 
-        let (comment, files) = match get_all_snapshot_files(&datastore, &info) {
+        let (comment, verification, files) = match get_all_snapshot_files(&datastore, &info) {
             Ok((manifest, files)) => {
                 size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
                 // extract the first line from notes
@@ -370,11 +370,21 @@ pub fn list_snapshots (
                     .and_then(|notes| notes.lines().next())
                     .map(String::from);
 
-                (comment, files)
+                let verify = manifest.unprotected["verify_state"].clone();
+                let verify: Option<SnapshotVerifyState> = match serde_json::from_value(verify) {
+                    Ok(verify) => verify,
+                    Err(err) => {
+                        eprintln!("error parsing verification state : '{}'", err);
+                        None
+                    }
+                };
+
+                (comment, verify, files)
             },
             Err(err) => {
                 eprintln!("error during snapshot file listing: '{}'", err);
                 (
+                    None,
                     None,
                     info
                         .files
@@ -394,6 +404,7 @@ pub fn list_snapshots (
             backup_id: group.backup_id().to_string(),
             backup_time: info.backup_dir.backup_time().timestamp(),
             comment,
+            verification,
             files,
             size,
             owner: Some(owner),
@@ -453,6 +464,7 @@ pub fn status(
     returns: {
         schema: UPID_SCHEMA,
     },
+    protected: true,
     access: {
         permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP, true), // fixme
     },
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index d3f20020..6854fdf0 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -6,6 +6,7 @@ use proxmox::const_regex;
 use proxmox::{IPRE, IPV4RE, IPV6RE, IPV4OCTET, IPV6H16, IPV6LS32};
 
 use crate::backup::CryptMode;
+use crate::server::UPID;
 
 #[macro_use]
 mod macros;
@@ -379,6 +380,25 @@ pub struct GroupListItem {
     pub owner: Option<Userid>,
 }
 
+#[api(
+    properties: {
+        upid: {
+            schema: UPID_SCHEMA
+        },
+        state: {
+            type: String
+        },
+    },
+)]
+#[derive(Serialize, Deserialize)]
+/// Task properties.
+pub struct SnapshotVerifyState {
+    /// UPID of the verify task
+    pub upid: UPID,
+    /// State of the verification. "failed" or "ok"
+    pub state: String,
+}
+
 #[api(
     properties: {
         "backup-type": {
@@ -394,6 +414,10 @@ pub struct GroupListItem {
             schema: SINGLE_LINE_COMMENT_SCHEMA,
             optional: true,
         },
+        verification: {
+            type: SnapshotVerifyState,
+            optional: true,
+        },
         files: {
             items: {
                 schema: BACKUP_ARCHIVE_NAME_SCHEMA
@@ -415,6 +439,9 @@ pub struct SnapshotListItem {
     /// The first line from manifest "notes"
     #[serde(skip_serializing_if="Option::is_none")]
     pub comment: Option<String>,
+    /// The result of the last run verify task
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub verification: Option<SnapshotVerifyState>,
     /// List of contained archive files.
     pub files: Vec<BackupContent>,
     /// Overall snapshot size (sum of all archive sizes).
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index f9437ff0..dc8be22c 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -1,8 +1,9 @@
 use std::collections::HashSet;
 
-use anyhow::{bail, Error};
+use anyhow::{bail, format_err, Error};
 
 use crate::server::WorkerTask;
+use crate::api2::types::*;
 
 use super::{
     DataStore, BackupGroup, BackupDir, BackupInfo, IndexFile,
@@ -178,7 +179,7 @@ pub fn verify_backup_dir(
     worker: &WorkerTask
 ) -> Result<bool, Error> {
 
-    let manifest = match datastore.load_manifest(&backup_dir) {
+    let mut manifest = match datastore.load_manifest(&backup_dir) {
         Ok((manifest, _)) => manifest,
         Err(err) => {
             worker.log(format!("verify {}:{} - manifest load error: {}", datastore.name(), backup_dir, err));
@@ -190,6 +191,7 @@ pub fn verify_backup_dir(
 
     let mut error_count = 0;
 
+    let mut verify_result = "ok";
     for info in manifest.files() {
         let result = proxmox::try_block!({
             worker.log(format!("  check {}", info.filename));
@@ -221,9 +223,20 @@ pub fn verify_backup_dir(
         if let Err(err) = result {
             worker.log(format!("verify {}:{}/{} failed: {}", datastore.name(), backup_dir, info.filename, err));
             error_count += 1;
+            verify_result = "failed";
         }
+
     }
 
+    let verify_state = SnapshotVerifyState {
+        state: verify_result.to_string(),
+        upid: worker.upid().clone(),
+    };
+    manifest.unprotected["verify_state"] = serde_json::to_value(verify_state)?;
+    datastore.store_manifest(&backup_dir, serde_json::to_value(manifest)?)
+        .map_err(|err| format_err!("unable to store manifest blob - {}", err))?;
+
+
     Ok(error_count == 0)
 }
 
-- 
2.27.0

[pbs-devel] [PATCH backup 2/2] ui: datastore content: show last verify result from a snapshot

[Thomas Lamprecht]

Double-click on the verify grid-cell of a specific snapshot (not the
group) opens the relevant task log.

The date of the last verify is shown as tool-tip.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

The double-click to open the last verify task log of a snapshot could be more
intuitive if it was a full blown button, or action button - as now one needs to
know/guess that they can double-click that field. Could be definitively
improved.

 www/DataStoreContent.js | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/www/DataStoreContent.js b/www/DataStoreContent.js
index 62e57817..6a5cec0e 100644
--- a/www/DataStoreContent.js
+++ b/www/DataStoreContent.js
@@ -10,6 +10,7 @@ Ext.define('pbs-data-store-snapshots', {
 	},
 	'files',
 	'owner',
+	'verification',
 	{ name: 'size', type: 'int', allowNull: true, },
 	{
 	    name: 'crypt-mode',
@@ -209,6 +210,10 @@ Ext.define('PBS.DataStoreContent', {
 			group.size = item.size;
 			group.owner = item.owner;
 		    }
+		    if (item.verification &&
+		       (!group.verification || group.verification.state !== 'failed')) {
+			group.verification = item.verification;
+		    }
 
 		}
 		group.count = group.children.length;
@@ -563,6 +568,41 @@ Ext.define('PBS.DataStoreContent', {
 		return (iconTxt + PBS.Utils.cryptText[v]) || Proxmox.Utils.unknownText
 	    }
 	},
+	{
+	    header: gettext('Verify State'),
+	    sortable: true,
+	    dataIndex: 'verification',
+	    renderer: (v, meta, record) => {
+		if (v === undefined || v === null || !v.state) {
+		    //meta.tdCls = "x-grid-row-loading";
+		    return record.data.leaf ? '' : gettext('None');
+		}
+		let task = Proxmox.Utils.parse_task_upid(v.upid);
+		let verify_time = Proxmox.Utils.render_timestamp(task.starttime);
+		let iconCls = v.state === 'ok' ? 'check good' : 'times critical';
+		let tip = `Verify task started on ${verify_time}`;
+		if (record.parentNode.id === 'root') {
+		    tip = v.state === 'ok'
+			? 'All verification OK in backup group'
+			: 'At least one failed verification in backup group!';
+		}
+		return `<span data-qtip="${tip}">
+		    <i class="fa fa-fw fa-${iconCls}"></i> ${v.state}
+		</span>`;
+	    },
+	    listeners: {
+		dblclick: function(view, el, row, col, ev, rec) {
+		    let data = rec.data || {};
+		    let verify = data.verification;
+		    if (verify && verify.upid && rec.parentNode.id !== 'root') {
+			let win = Ext.create('Proxmox.window.TaskViewer', {
+			    upid: verify.upid,
+			});
+			win.show();
+		    }
+		},
+	    },
+	},
     ],
 
     tbar: [
-- 
2.27.0

Re: [pbs-devel] [PATCH backup 1/2] save last verify result in snapshot manifest

[Dietmar Maurer]

> On 08/25/2020 5:30 PM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> Save the state ("ok" or "failed") and the UPID of the respective
> verify task. With this we can easily allow to open  the relevant task
> log and show when the last verify happened.
> 
> As we already load the manifest when listing the snapshots, just add
> it there directly.
> 
> We need to make the verify API call protected to be able to save the manifest.

Why? IMHO this should not be necessary, because backups are written by the 'backup' user.

[pbs-devel] applied: [PATCH backup 1/2] save last verify result in snapshot manifest

[Dietmar Maurer]

applied, but removed the 'protected' flag (which is not required)

> On 08/25/2020 5:30 PM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> Save the state ("ok" or "failed") and the UPID of the respective
> verify task. With this we can easily allow to open  the relevant task
> log and show when the last verify happened.
> 
> As we already load the manifest when listing the snapshots, just add
> it there directly.
> 
> We need to make the verify API call protected to be able to save the manifest.
>

Re: [pbs-devel] [PATCH backup 1/2] save last verify result in snapshot manifest

[Thomas Lamprecht]

On 26.08.20 06:30, Dietmar Maurer wrote:
> 
>> On 08/25/2020 5:30 PM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
>>
>>  
>> Save the state ("ok" or "failed") and the UPID of the respective
>> verify task. With this we can easily allow to open  the relevant task
>> log and show when the last verify happened.
>>
>> As we already load the manifest when listing the snapshots, just add
>> it there directly.
>>
>> We need to make the verify API call protected to be able to save the manifest.
> 
> Why? IMHO this should not be necessary, because backups are written by the 'backup' user.
> 

You are right. I had some backups made during a debugging session a month
ago where I mistakenly ran the proxy as root - so those had wrong file owners,
from that I made a wrong assumption..