[pve-devel] [PATCH manager 3/3] Storage GUI: Rewrite backup content view as TreePanel.

[Fabian Ebner]

Am 04.03.22 um 12:52 schrieb Matthias Heiserer:
> Should be easier to read/use than the current flat list.
> Backups are grouped by ID and type, so in case there are backups
> with ID 100 for both CT and VM, this would create two separate
> groups in the UI.

It might make sense to include qemu/lxc (for PBS either vm/ct or
qemu/lxc, not too sure) for displaying. Like that all containers backups
would come first like in PBS. Since we don't have notes for groups and
some people rely on notes to find VMs rather than the ID, it might be
better to start with a fully expanded tree.

If I have a manually renamed backup, it'll show below a node with name
'Root', maybe explicitly grouping them as 'Other' or 'None' would be better.

One other thing I noticed is that the selection is not remembered after
changing protection/editing notes. Not a big deal, but would be nice if
not too complicated.

> Date and size of group are taken from the latest backup.

It's a bit tricky. PBS has deduplication so doing that there makes
sense, but I'm sure many people would expect to see the sum of sizes
when dealing with non-PBS storages.

> Notes, Protection, Encrypted, and Verify State stay as default
> value empty, empty, No, and None, respectively.
> 
> Code adapted from the existing backup view and the pbs
> datastore content, where appropriate.
> 
> Signed-off-by: Matthias Heiserer <m.heiserer at proxmox.com>
> ---
>  www/manager6/storage/BackupView.js | 620 ++++++++++++++++++++---------
>  1 file changed, 433 insertions(+), 187 deletions(-)
> 
> diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
> index 2328c0fc..124a57c9 100644
> --- a/www/manager6/storage/BackupView.js
> +++ b/www/manager6/storage/BackupView.js
> @@ -1,36 +1,384 @@
> -Ext.define('PVE.storage.BackupView', {
> -    extend: 'PVE.storage.ContentView',

It'd be great if you could also remove the now unused stuff from the
ContentView base class.

Did you think about a way to re-use this new class for the guest's
backup view already?

> +Ext.define('pve-data-store-backups', {

Nit: datastore is not typical for PVE, and I'd prefer singular 'backup'.

> +    extend: 'Ext.data.Model',
> +    fields: [
> +	{
> +	    name: 'ctime',
> +	    date: 'date',
> +	    dateFormat: 'timestamp',
> +	},
> +	'format',
> +	'volid',
> +	'content',
> +	'vmid',
> +	'size',
> +	'protected',
> +	'notes',

Does not mention 'text' which is assigned later. And ContentView has
special handling to not display the full volid. Please also use
PVE.Utils.render_storage_content() for that here. Or we might want to
switch to using only the date for the leafs? But that needs some
consideration for filtering, and renamed backups which we currently
support..

> +    ],
> +});
> +
>  
> +Ext.define('PVE.storage.BackupView', {
> +    extend: 'Ext.tree.Panel',
>      alias: 'widget.pveStorageBackupView',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +    rootVisible: false,
> +
> +    title: gettext('Content'),
> +
> +    cbindData: function(initialCfg) {
> +	return {
> +	    notPBS: initialCfg.pluginType !== 'pbs',
> +	};
> +    },
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	init: function(view) {
> +	    let me = this;
> +	    me.storage = view.pveSelNode.data.storage;
> +	    if (!me.storage) {
> +		throw "No datastore specificed";
> +	    }
> +	    me.nodename = view.pveSelNode.data.node;
>  
> -    showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
> +	    me.store = Ext.create('Ext.data.Store', {
> +		model: 'pve-data-store-backups',
> +		groupField: 'vmid',
> +		filterer: 'bottomup',
> +	    });
> +	    me.store.on('load', me.onLoad, me);
> +
> +	    view.getStore().setSorters([
> +		'vmid',
> +		'text',

Now it's ascending by date again.

@Thomas: in PBS it's also ascending by date. Should that be changed or
do we switch back in PVE?

> +		'backup-time',

Isn't it ctime? That said, ContentView has some special handling for the
'vdate' column, so maybe we should continue using that. It seems like
the back-end automatically sets the ctime to be the one from the archive
name via archive_info() in case it's a standard name, so maybe it
doesn't actually matter.

> +	    ]);
> +	    view.getStore().setConfig('filterer', 'bottomup');
> +	    Proxmox.Utils.monStoreErrors(view, me.store);
> +	},
> +
> +	onLoad: function(store, records, success, operation) {
> +	    let me = this;
> +	    let view = me.getView();
>  
> -    initComponent: function() {
> -	var me = this;
> +	    let expanded = {};
> +	    view.getRootNode().cascadeBy({
> +		before: item => {
> +		    if (item.isExpanded() && !item.data.leaf) {
> +			let id = item.data.text;
> +			expanded[id] = true;
> +			return true;
> +		    }
> +		    return false;
> +		},
> +		after: Ext.emptyFn,
> +	    });
> +	    let groups = me.getRecordGroups(records, expanded);
>  
> -	var nodename = me.nodename = me.pveSelNode.data.node;
> -	if (!nodename) {
> -	    throw "no node name specified";
> -	}
> +	    for (const item of records.map(i => i.data)) {
> +		item.text = item.volid;
> +		item.leaf = true;
> +		item.ctime = new Date(item.ctime * 1000);
> +		item.iconCls = 'fa-file-o';
> +		groups[`${item.format}` + item.vmid].children.push(item);
> +	    }
> +
> +	    for (let [_name, group] of Object.entries(groups)) {
> +		let c = group.children;

Style nit: please no one character variable names (except for closures).

> +		let latest = c.reduce((l, r) => l.ctime > r.ctime ? l : r);
> +		let num_verified = c.reduce((l, r) => l + r.verification === 'ok', 0);

Does adding a boolean work reliably? Feels hacky.

> +		group.ctime = latest.ctime;
> +		group.size = latest.size;
> +		group.verified = num_verified / c.length;
> +	    }
> +
> +	    let children = [];
> +	    Object.entries(groups).forEach(e => children.push(e[1]));
> +	    view.setRootNode({
> +		expanded: true,
> +		children: children,
> +	    });
>  
> -	var storage = me.storage = me.pveSelNode.data.storage;
> -	if (!storage) {
> -	    throw "no storage ID specified";
> -	}
> +	    Proxmox.Utils.setErrorMask(view, false);
> +	},
>  
> -	me.content = 'backup';
> +	reload: function() {
> +	    let me = this;
> +	    let view = me.getView();
> +	    if (!view.store || !me.store) {
> +		console.warn('cannot reload, no store(s)');
> +		return;
> +	    }
>  
> -	var sm = me.sm = Ext.create('Ext.selection.RowModel', {});
> +	    let url = `/api2/json/nodes/${me.nodename}/storage/${me.storage}/content`;
> +	    me.store.setProxy({
> +		type: 'proxmox',
> +		timeout: 60*1000,

I don't think you need to set this. API calls have a 30 second timeout IIRC.

> +		url: url,
> +		extraParams: {
> +		    content: 'backup',
> +		},
> +	    });
>  
> -	var reload = function() {
>  	    me.store.load();
> -	};
> +	    Proxmox.Utils.monStoreErrors(view, me.store);
> +	},
>  
> -	let pruneButton = Ext.create('Proxmox.button.Button', {
> -	    text: gettext('Prune group'),
> +	getRecordGroups: function(records, expanded) {
> +	    let groups = {};
> +	    for (const item of records) {
> +		groups[`${item.data.format}` + item.data.vmid] = {

Format can be things like 'tar', 'tar.zst', etc. when it's not PBS.

> +		    vmid: item.data.vmid,
> +		    leaf: false,
> +		    children: [],
> +		    expanded: !!expanded[item.data.vmid],
> +		    text: item.data.vmid,
> +		    ctime: 0,
> +		    format: item.data.format,
> +		    volid: "volid",
> +		    content: "content",
> +		    size: 0,
> +		    iconCls: PVE.Utils.get_type_icon_cls(item.data.volid, item.data.format),
> +		};
> +	    }
> +	    return groups;
> +	},
> +
> +	restoreHandler: function(button, event, rec) {
> +	    let me = this;
> +	    let vmtype = PVE.Utils.get_backup_type(rec.data.volid, rec.data.format);
> +	    let win = Ext.create('PVE.window.Restore', {
> +		nodename: me.nodename,
> +		volid: rec.data.volid,
> +		volidText: PVE.Utils.render_storage_content(rec.data.volid, {}, rec),
> +		vmtype: vmtype,
> +		isPBS: me.isPBS,

me.isPBS doesn't seem to be set anywhere.

> +		view: me.view,
> +	    });
> +	    win.on('destroy', () => me.reload());
> +	    win.show();
> +	},
> +

----8<----

> @@ -38,186 +386,84 @@ Ext.define('PVE.storage.BackupView', {
>  
>  		    let vmtype;
>  		    if (name.startsWith('vzdump-lxc-') || format === "pbs-ct") {
> -			vmtype = 'lxc';
> +		        vmtype = 'lxc';
>  		    } else if (name.startsWith('vzdump-qemu-') || format === "pbs-vm") {
> -			vmtype = 'qemu';
> +		        vmtype = 'qemu';

Introduces whitespace errors and could use the helper from patch #2.

>  		    }
> -
>  		    if (vmid && vmtype) {
> -			this.setText(gettext('Prune group') + ` ${vmtype}/${vmid}`);
> -			this.vmid = vmid;
> -			this.vmtype = vmtype;
> -			this.setDisabled(false);
> +			me.setText(gettext('Prune group') + ` ${vmtype}/${vmid}`);
> +			me.vmid = vmid;
> +			me.vmtype = vmtype;
> +			me.setDisabled(false);
>  			return;
>  		    }
>  		}
> -		this.setText(gettext('Prune group'));
> -		this.vmid = null;
> -		this.vmtype = null;
> -		this.setDisabled(true);
> +		me.setText(gettext('Prune group'));
> +		me.vmid = null;
> +		me.vmtype = null;
> +		me.setDisabled(true);
> +	    },
> +	    handler: 'pruneGroupHandler',
> +	    parentXType: "treepanel",
> +	    disabled: true,
> +	    reference: 'pruneButton',
> +	    enableFn: () => true,
> +	},
> +	{
> +	    xtype: 'proxmoxButton',
> +	    text: gettext('Remove'),
> +	    handler: 'removeHandler',
> +	    parentXType: 'treepanel',
> +	    disabled: true,
> +	    enableFn: record => record.phantom === false && !record?.data?.protected,
> +	    confirmMsg: function(rec) {
> +		console.log("controller:", this.getController());

Left-over debug log.

> +		let name = rec.data.text;
> +		return Ext.String.format(gettext('Are you sure you want to remove entry {0}'), `'${name}'`);
> +	    },
> +	},
> +	'->',