From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D881B65FE3 for ; Wed, 9 Mar 2022 13:39:46 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C77561D619 for ; Wed, 9 Mar 2022 13:39:16 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9C4811D60E for ; Wed, 9 Mar 2022 13:39:15 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 6C4284582F for ; Wed, 9 Mar 2022 13:39:15 +0100 (CET) Message-ID: Date: Wed, 9 Mar 2022 13:39:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 Content-Language: en-US To: pve-devel@lists.proxmox.com, m.heiserer@proxmox.com References: <20220304115218.665615-1-m.heiserer@proxmox.com> <20220304115218.665615-3-m.heiserer@proxmox.com> From: Fabian Ebner In-Reply-To: <20220304115218.665615-3-m.heiserer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.122 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH manager 3/3] Storage GUI: Rewrite backup content view as TreePanel. X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Mar 2022 12:39:46 -0000 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 > --- > 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}'`); > + }, > + }, > + '->',