[pve-devel] [PATCH storage/manager] show comment/verification info for backups

[pve-devel] [PATCH storage/manager] show comment/verification info for backups

[Dominik Csapak]

show that information from pbs, and prepare for old-style vzdump
backups to get '.comment' files
(those i'll send tomorrow, still need to do some cleanup to my patches)

pve-storage:

Dominik Csapak (2):
  api2/storage/content: add comment and verification fields to content
    listing
  Storage/Plugin: read .comment files for comments

 PVE/API2/Storage/Content.pm | 20 ++++++++++++++++++++
 PVE/Storage/PBSPlugin.pm    |  3 +++
 PVE/Storage/Plugin.pm       | 23 +++++++++++++++++++++++
 3 files changed, 46 insertions(+)

pve-manager:

Dominik Csapak (1):
  ui: add comment/verification columns to backup/content grid

 www/manager6/grid/BackupView.js     | 39 +++++++++++++++++++++++++++++
 www/manager6/storage/ContentView.js |  8 +++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

-- 
2.20.1

[pve-devel] [PATCH storage 1/2] api2/storage/content: add comment and verification fields to content listing

[Dominik Csapak]

for now only for pbs, since we do not have such infos elsewhere

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Storage/Content.pm | 20 ++++++++++++++++++++
 PVE/Storage/PBSPlugin.pm    |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index f2e3e57..8d2ff32 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -87,6 +87,26 @@ __PACKAGE__->register_method ({
 		    minimum => 0,
 		    optional => 1,
 		},
+		comment => {
+		    description => "An optional Comment.",
+		    type => 'string',
+		    optional => 1,
+		},
+		verification => {
+		    description => "Last backup verification result, only useful for PBS storages.",
+		    type => 'object',
+		    properties => {
+			state => {
+			    description => "Last backup verification state.",
+			    type => 'string',
+			},
+			upid => {
+			    description => "Last backup verification UPID.",
+			    type => 'string',
+			},
+		    },
+		    optional => 1,
+		},
 	    },
 	},
 	links => [ { rel => 'child', href => "{volid}" } ],
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index 6403e2e..28f6a3b 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -523,6 +523,9 @@ sub list_volumes {
 	    ctime => $epoch,
 	};
 
+	$info->{verification} = $item->{verification} if defined($item->{verification});
+	$info->{comment} = $item->{comment} if defined($item->{comment});
+
 	push @$res, $info;
     }
 
-- 
2.20.1

[pve-devel] applied: [PATCH storage 1/2] api2/storage/content: add comment and verification fields to content listing

[Thomas Lamprecht]

On 12.11.20 16:26, Dominik Csapak wrote:
> for now only for pbs, since we do not have such infos elsewhere
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Storage/Content.pm | 20 ++++++++++++++++++++
>  PVE/Storage/PBSPlugin.pm    |  3 +++
>  2 files changed, 23 insertions(+)
> 
>

applied, thanks!

[pve-devel] [PATCH storage 2/2] Storage/Plugin: read .comment files for comments

[Dominik Csapak]

we have no way of setting them yet via api, but we can read them now

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Storage/Plugin.pm | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index a046640..00ce9cb 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -19,6 +19,8 @@ use base qw(PVE::SectionConfig);
 
 use constant COMPRESSOR_RE => 'gz|lzo|zst';
 
+use constant COMMENT_EXT => ".comment";
+
 our @COMMON_TAR_FLAGS = qw(
     --one-file-system
     -p --sparse --numeric-owner --acls
@@ -987,8 +989,18 @@ my $get_subdir_files = sub {
 
     my $res = [];
 
+    my $has_comment = {};
+
     foreach my $fn (<$path/*>) {
 
+	if (COMMENT_EXT eq substr($fn, -length(COMMENT_EXT))) {
+	    my $real_fn = substr($fn, 0, length($fn) -  length(COMMENT_EXT));
+	    if (!defined($has_comment->{$real_fn})) {
+		$has_comment->{$real_fn} = (-f $fn);
+	    }
+	    next; # we do not need to do anything with comments themselves
+	}
+
 	my $st = File::stat::stat($fn);
 
 	next if (!$st || S_ISDIR($st->mode));
@@ -1008,6 +1020,7 @@ my $get_subdir_files = sub {
 	} elsif ($tt eq 'backup') {
 	    next if defined($vmid) && $fn !~  m/\S+-$vmid-\S+/;
 	    next if $fn !~ m!/([^/]+\.(tgz|(?:(?:tar|vma)(?:\.(${\COMPRESSOR_RE}))?)))$!;
+	    my $original = $fn;
 	    my $format = $2;
 	    $fn = $1;
 	    $info = { volid => "$sid:backup/$fn", format => $format };
@@ -1020,6 +1033,16 @@ my $get_subdir_files = sub {
 		$info->{vmid} = $vmid // $1;
 	    }
 
+	    my $comment_fn = $original.COMMENT_EXT;
+	    if (!defined($has_comment->{$original})) {
+		$has_comment->{$original} = (-f $comment_fn);
+	    }
+
+	    if ($has_comment->{$original}) {
+		my $comment = PVE::Tools::file_read_firstline($comment_fn);
+		$info->{comment} = $comment if defined($comment);
+	    }
+
 	} elsif ($tt eq 'snippets') {
 
 	    $info = {
-- 
2.20.1

Re: [pve-devel] [PATCH storage 2/2] Storage/Plugin: read .comment files for comments

[Thomas Lamprecht]

On 12.11.20 16:26, Dominik Csapak wrote:
> we have no way of setting them yet via api, but we can read them now
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/Storage/Plugin.pm | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 

applied, but dropped the relatively complicated optimizations which avoided
a probably cached stat in an edge case - I do not see much value in such
over optimizations which make a previous simple sub harder to understand
without even noting why it was done anywhere - especially without any stats
providing some hard numbers about if and in what environments this actually
helps.

> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index a046640..00ce9cb 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -19,6 +19,8 @@ use base qw(PVE::SectionConfig);
>  
>  use constant COMPRESSOR_RE => 'gz|lzo|zst';
>  
> +use constant COMMENT_EXT => ".comment";
> +
>  our @COMMON_TAR_FLAGS = qw(
>      --one-file-system
>      -p --sparse --numeric-owner --acls
> @@ -987,8 +989,18 @@ my $get_subdir_files = sub {
>  
>      my $res = [];
>  
> +    my $has_comment = {};
> +
>      foreach my $fn (<$path/*>) {
>  
> +	if (COMMENT_EXT eq substr($fn, -length(COMMENT_EXT))) {
> +	    my $real_fn = substr($fn, 0, length($fn) -  length(COMMENT_EXT));
> +	    if (!defined($has_comment->{$real_fn})) {
> +		$has_comment->{$real_fn} = (-f $fn);

this is a stat too, so this all only avoided one unused away stat for each
unlikely case that a backup has a comment file.

> +	    }
> +	    next; # we do not need to do anything with comments themselves
> +	}
> +
>  	my $st = File::stat::stat($fn);
>  
>  	next if (!$st || S_ISDIR($st->mode));
> @@ -1008,6 +1020,7 @@ my $get_subdir_files = sub {
>  	} elsif ($tt eq 'backup') {
>  	    next if defined($vmid) && $fn !~  m/\S+-$vmid-\S+/;
>  	    next if $fn !~ m!/([^/]+\.(tgz|(?:(?:tar|vma)(?:\.(${\COMPRESSOR_RE}))?)))$!;
> +	    my $original = $fn;
>  	    my $format = $2;
>  	    $fn = $1;
>  	    $info = { volid => "$sid:backup/$fn", format => $format };
> @@ -1020,6 +1033,16 @@ my $get_subdir_files = sub {
>  		$info->{vmid} = $vmid // $1;
>  	    }
>  
> +	    my $comment_fn = $original.COMMENT_EXT;
> +	    if (!defined($has_comment->{$original})) {
> +		$has_comment->{$original} = (-f $comment_fn);
> +	    }
> +
> +	    if ($has_comment->{$original}) {
> +		my $comment = PVE::Tools::file_read_firstline($comment_fn);
> +		$info->{comment} = $comment if defined($comment);
> +	    }
> +
>  	} elsif ($tt eq 'snippets') {
>  
>  	    $info = {
> 

[pve-devel] [PATCH manager 1/1] ui: add comment/verification columns to backup/content grid

[Dominik Csapak]

verification column only shows in the backup grid and for
pbs storages

(renderer is mostly copied from proxmox-backup)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/grid/BackupView.js     | 39 +++++++++++++++++++++++++++++
 www/manager6/storage/ContentView.js |  8 +++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index ff8d69ec..a9c6be5e 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -99,6 +99,15 @@ Ext.define('PVE.grid.BackupView', {
 	    allowBlank: false,
 	    listeners: {
 		change: function(f, value) {
+		    let storage = f.getStore().findRecord('storage', value);
+		    if (storage) {
+			let isPbs = storage.data.type === 'pbs';
+			me.getColumns().forEach((column) => {
+			    if (column.dataIndex === 'verification') {
+				column.setHidden(!isPbs);
+			    }
+			});
+		    }
 		    setStorage(value);
 		}
 	    }
@@ -251,6 +260,36 @@ Ext.define('PVE.grid.BackupView', {
 		    dataIndex: 'vmid',
 		    hidden: true,
 		},
+		{
+		    header: gettext('Comment'),
+		    dataIndex: 'comment',
+		    width: 100,
+		    renderer: Ext.htmlEncode,
+		},
+		{
+		    header: gettext('Verify State'),
+		    dataIndex: 'verification',
+		    renderer: function(v) {
+			let i = (cls, txt) => `<i class="fa fa-fw fa-${cls}"></i> ${txt}`;
+			if (v === undefined || v === null) {
+			    return i('question-circle-o warning', gettext('None'));
+			}
+			let tip = ""
+			let txt = gettext('Failed');
+			let iconCls = 'times critical';
+			if (v.state === 'ok') {
+			    txt = gettext('OK');
+			    iconCls = 'check good';
+			    let now = Date.now() / 1000;
+			    let task = Proxmox.Utils.parse_task_upid(v.upid);
+			    if (now - v.starttime > 30 * 24 * 60 * 60) {
+				tip = `Last verify task over 30 days ago: ${verify_time}`;
+				iconCls = 'check warning';
+			    }
+			}
+			return `<span data-qtip="${tip}"> ${i(iconCls, txt)} </span>`;
+		    }
+		}
 	    ]
 	});
 
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index 194ad42e..6b30167a 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -609,6 +609,12 @@ Ext.define('PVE.storage.ContentView', {
 		    width: 100,
 		    renderer: Proxmox.Utils.format_size,
 		    dataIndex: 'size'
+		},
+		{
+		    header: gettext('Comment'),
+		    width: 100,
+		    renderer: Ext.htmlEncode,
+		    dataIndex: 'comment',
 		}
 	    ],
 	    listeners: {
@@ -655,7 +661,7 @@ Ext.define('PVE.storage.ContentView', {
 	extend: 'Ext.data.Model',
 	fields: [
 	    'volid', 'content', 'format', 'size', 'used', 'vmid',
-	    'channel', 'id', 'lun',
+	    'channel', 'id', 'lun', 'comment', 'verification',
 	    {
 		name: 'text',
 		convert: function(value, record) {
-- 
2.20.1

[pve-devel] applied: [PATCH manager 1/1] ui: add comment/verification columns to backup/content grid

[Thomas Lamprecht]

On 12.11.20 16:26, Dominik Csapak wrote:
> verification column only shows in the backup grid and for
> pbs storages
> 
> (renderer is mostly copied from proxmox-backup)

missing some crucial parts (see below)

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/grid/BackupView.js     | 39 +++++++++++++++++++++++++++++
>  www/manager6/storage/ContentView.js |  8 +++++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
> index ff8d69ec..a9c6be5e 100644
> --- a/www/manager6/grid/BackupView.js
> +++ b/www/manager6/grid/BackupView.js
> @@ -99,6 +99,15 @@ Ext.define('PVE.grid.BackupView', {
>  	    allowBlank: false,
>  	    listeners: {
>  		change: function(f, value) {
> +		    let storage = f.getStore().findRecord('storage', value);
> +		    if (storage) {
> +			let isPbs = storage.data.type === 'pbs';
> +			me.getColumns().forEach((column) => {
> +			    if (column.dataIndex === 'verification') {
> +				column.setHidden(!isPbs);
> +			    }
> +			});
> +		    }
>  		    setStorage(value);
>  		}
>  	    }
> @@ -251,6 +260,36 @@ Ext.define('PVE.grid.BackupView', {
>  		    dataIndex: 'vmid',
>  		    hidden: true,
>  		},
> +		{
> +		    header: gettext('Comment'),
> +		    dataIndex: 'comment',
> +		    width: 100,
> +		    renderer: Ext.htmlEncode,
> +		},

re-ordered that column as second, like PBS does it.

> +		{
> +		    header: gettext('Verify State'),
> +		    dataIndex: 'verification',
> +		    renderer: function(v) {
> +			let i = (cls, txt) => `<i class="fa fa-fw fa-${cls}"></i> ${txt}`;
> +			if (v === undefined || v === null) {
> +			    return i('question-circle-o warning', gettext('None'));
> +			}
> +			let tip = ""
> +			let txt = gettext('Failed');
> +			let iconCls = 'times critical';
> +			if (v.state === 'ok') {
> +			    txt = gettext('OK');
> +			    iconCls = 'check good';
> +			    let now = Date.now() / 1000;
> +			    let task = Proxmox.Utils.parse_task_upid(v.upid);

.                           ^^^~ ununsed

> +			    if (now - v.starttime > 30 * 24 * 60 * 60) {
> +				tip = `Last verify task over 30 days ago: ${verify_time}`;

.                                                                     ^^^^^^^^^^~ undefined

fixed up in followup, adding also the missing tip in the non outdated case.

We really need to get pve-manager also eslint ready, as then such mistakes
can get caught easily.