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 5FD8168BC7 for ; Thu, 10 Mar 2022 11:50:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4DAE01922B for ; Thu, 10 Mar 2022 11:49:30 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id F04071927E for ; Thu, 10 Mar 2022 11:49:28 +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 C7E114640B for ; Thu, 10 Mar 2022 11:49:28 +0100 (CET) Message-ID: Date: Thu, 10 Mar 2022 11:49:25 +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, Aaron Lauterer References: <20220307100722.257128-1-a.lauterer@proxmox.com> <20220307100722.257128-2-a.lauterer@proxmox.com> From: Fabian Ebner In-Reply-To: <20220307100722.257128-2-a.lauterer@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 v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu 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: Thu, 10 Mar 2022 10:50:00 -0000 Am 07.03.22 um 11:07 schrieb Aaron Lauterer: > For the new HDReassign component, we follow the approach of HDMove to > have one componend for qemu and lxc. > > To avoid button clutter, a new "Disk/Volume action" button is > introduced. It holds the Move, Reassign and Resize buttons in a submenu. > Some small general things I noticed: * Since re-assign to/from templates is not possible, we could filter those out in the selection/disable the action (assuming the information whether it's a template or not is readily available). * For such drop-down menus we mostly (always?) have icons too. * The move disk and reassign windows seem to have a bit much padding. > Signed-off-by: Aaron Lauterer > --- > changes since > > v2: > * switch from generic to Proxmox Edit window > * add new submenu for disk/volume specific actions > * code style improvements > * simplify some labels, removing "disk" and "volume" as the context > already gives this away > > > v1: incorporated feedback I got off list > > * use more modern approaches > * arrow functions > * autoShow > * template strings > * reduce predefined cbind values and use arrow functions in the cbind > directly in many cases > * some code style issues and cleanup > > www/manager6/Makefile | 1 + > www/manager6/lxc/Resources.js | 62 +++++-- > www/manager6/qemu/HDReassign.js | 274 ++++++++++++++++++++++++++++++ > www/manager6/qemu/HardwareView.js | 60 ++++++- > 4 files changed, 378 insertions(+), 19 deletions(-) > create mode 100644 www/manager6/qemu/HDReassign.js > > diff --git a/www/manager6/Makefile b/www/manager6/Makefile > index e6e01bd1..94a78d89 100644 > --- a/www/manager6/Makefile > +++ b/www/manager6/Makefile > @@ -214,6 +214,7 @@ JSSRC= \ > qemu/HDTPM.js \ > qemu/HDMove.js \ > qemu/HDResize.js \ > + qemu/HDReassign.js \ Nit: not ordered alphabetically > qemu/HardwareView.js \ > qemu/IPConfigEdit.js \ > qemu/KeyboardEdit.js \ > diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js > index 15ee3c67..306a4988 100644 > --- a/www/manager6/lxc/Resources.js > +++ b/www/manager6/lxc/Resources.js > @@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', { > }); > }; > > - var run_move = function(b, e, rec) { > + var run_move = function() { > + let rec = me.selModel.getSelection()[0]; Style nit: could switch to let instead of var. > if (!rec) { > return; > } > @@ -168,6 +169,24 @@ Ext.define('PVE.lxc.RessourceView', { > win.on('destroy', me.reload, me); > }; > > + let run_reassign = function() { > + let rec = me.selModel.getSelection()[0]; > + if (!rec) { > + return; > + } > + > + Ext.create('PVE.window.HDReassign', { > + disk: rec.data.key, > + nodename: nodename, > + autoShow: true, > + vmid: vmid, > + type: 'lxc', > + listeners: { > + destroy: () => me.reload(), > + }, > + }); > + }; > + > var edit_btn = new Proxmox.button.Button({ > text: gettext('Edit'), > selModel: me.selModel, > @@ -182,7 +201,7 @@ Ext.define('PVE.lxc.RessourceView', { > handler: function() { me.run_editor(); }, > }); > > - var resize_btn = new Proxmox.button.Button({ > + var resize_menuitem = new Ext.menu.Item({ Same here. > text: gettext('Resize disk'), > selModel: me.selModel, > disabled: true, ----8<---- > @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', { > } > edit_btn.setDisabled(noedit); > > - remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending); > - resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk); > - move_btn.setDisabled(!isDisk || !diskCap); > + volumeaction_btn.setDisabled(!isDisk); > + reassign_menuitem.setDisabled(isRootFS); > + > + remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending); > + resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk); > + move_menuitem.setDisabled(!isDisk || !diskCap); Nothing new, but could use '|| isUnusedDisk' too: cannot move volume 'unused0', only configured volumes can be moved to another storage > revert_btn.setDisabled(!pending); > > remove_btn.setText(isUsedDisk ? remove_btn.altText : remove_btn.defaultText); > @@ -327,8 +370,7 @@ Ext.define('PVE.lxc.RessourceView', { > }, > edit_btn, > remove_btn, > - resize_btn, > - move_btn, > + volumeaction_btn, > revert_btn, > ], > rows: rows, > diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js > new file mode 100644 > index 00000000..48bc0b1f > --- /dev/null > +++ b/www/manager6/qemu/HDReassign.js > @@ -0,0 +1,274 @@ > +Ext.define('PVE.window.HDReassign', { > + extend: 'Proxmox.window.Edit', > + mixins: ['Proxmox.Mixin.CBind'], > + > + resizable: false, > + modal: true, > + width: 350, > + border: false, > + layout: 'fit', > + showReset: false, > + showProgress: true, > + method: 'POST', > + > + viewModel: { > + data: { > + mpType: '', > + }, > + formulas: { > + mpMaxCount: get => get('mpType') === 'mp' > + ? PVE.Utils.mp_counts.mps - 1 > + : PVE.Utils.mp_counts.unused - 1, > + }, > + }, > + > + cbindData: function() { > + let me = this; > + return { > + vmid: me.vmid, > + disk: me.disk, > + isQemu: me.type === 'qemu', > + nodename: me.nodename, > + url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`, > + }; > + }, > + > + cbind: { > + title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'), > + submitText: get => get('title'), > + qemu: '{isQemu}', > + url: '{url}', > + }, > + > + submitUrl: function(url, values) { > + url += this.qemu ? 'move_disk' : 'move_volume'; Why not already construct the full URL above? > + return url; > + }, > + > + getValues: function() { > + let me = this; > + let values = me.formPanel.getForm().getValues(); > + > + let params = { > + vmid: me.vmid, > + 'target-vmid': values.targetVmid, > + }; > + > + params[me.qemu ? 'disk' : 'volume'] = me.disk; > + > + if (me.qemu) { > + params['target-disk'] = `${values.controller}${values.deviceid}`; > + } else { > + params['target-volume'] = `${values.mpType}${values.mpId}`; > + } > + return params; > + }, > + > + controller: { > + xclass: 'Ext.app.ViewController', > + > + initViewModel: function(model) { > + let view = this.getView(); > + let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp'; > + model.set('mpType', mpTypeValue); > + }, > + > + onMpTypeChange: function(value) { > + this.getView().getViewModel().set('mpType', value.getValue()); > + this.getView().lookup('mpIdSelector').validate(); > + }, > + > + onTargetVMChange: function(f, vmid) { > + let me = this; > + let view = me.getView(); > + let diskSelector = view.lookup('diskSelector'); > + if (!vmid) { > + diskSelector.setVMConfig(null); > + me.VMConfig = null; > + return; > + } > + > + let type = view.qemu ? 'qemu' : 'lxc'; > + > + let url = `/nodes/${view.nodename}/${type}/${vmid}/config`; > + Proxmox.Utils.API2Request({ > + url: url, > + method: 'GET', > + failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + success: function(response, options) { > + if (view.qemu) { > + diskSelector.setVMConfig(response.result.data); > + diskSelector.setDisabled(false); > + } else { > + let mpIdSelector = view.lookup('mpIdSelector'); > + let mpType = view.lookup('mpType'); > + > + view.VMConfig = response.result.data; > + > + PVE.Utils.forEachMP((bus, i) => { Feels hacky and in fact only works in all cases, because the number of max unused and max mp is the same. The iterator only iterates over mount points when called like this and you ignore the bus. Using a new helper akin to PVE.Utils.nextFreeDisk or simply inlining the required loop here, would be more robust. > + let name = view.getViewModel().get('mpType') + i.toString(); > + if (!Ext.isDefined(view.VMConfig[name])) { > + mpIdSelector.setValue(i); > + return false; > + } > + return undefined; > + }); > + > + mpType.setDisabled(false); > + mpIdSelector.setDisabled(false); > + mpIdSelector.validate(); > + } > + }, > + }); > + }, > + }, > + > + items: [ > + { > + xtype: 'form', > + reference: 'moveFormPanel', > + bodyPadding: 10, > + border: false, > + fieldDefaults: { > + labelWidth: 100, > + anchor: '100%', > + }, > + items: [ > + { > + xtype: 'displayfield', > + name: 'sourceDisk', > + fieldLabel: gettext('Source'), > + cbind: { > + name: get => get('isQemu') ? 'disk' : 'volume', > + value: '{disk}', > + }, > + vtype: 'StorageId', But it's not a storage ID. > + allowBlank: false, > + }, > + { > + xtype: 'vmComboSelector', > + reference: 'targetVMID', > + name: 'targetVmid', > + allowBlank: false, > + fieldLabel: gettext('Target'), > + bind: { > + value: '{targetVMID}', > + }, > + store: { > + model: 'PVEResources', > + autoLoad: true, > + sorters: 'vmid', > + cbind: {}, // for nested cbinds > + filters: [ > + { > + property: 'type', > + cbind: { > + value: get => get('isQemu') ? /qemu/ : /lxc/, Nit: the type can be matched exaclty, or was there a reason for using a regex? > + }, > + }, > + { > + property: 'node', > + cbind: { > + value: '{nodename}', > + }, > + }, > + { > + property: 'vmid', > + operator: '!=', > + cbind: { > + value: '{vmid}', > + }, > + }, > + ], > + }, > + listeners: { change: 'onTargetVMChange' }, > + }, > + { > + xtype: 'pveControllerSelector', > + reference: 'diskSelector', > + withUnused: true, > + disabled: true, > + cbind: { > + hidden: '{!isQemu}', > + }, > + }, > + { > + xtype: 'container', > + layout: 'hbox', > + cbind: { > + hidden: '{isQemu}', > + disabled: '{isQemu}', > + }, > + items: [ > + { > + xtype: 'pmxDisplayEditField', > + cbind: { > + editable: get => !get('disk').match(/^unused\d+/), > + value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp', > + }, > + disabled: true, > + name: 'mpType', > + reference: 'mpType', > + fieldLabel: gettext('Add as'), > + submitValue: true, > + flex: 4, > + editConfig: { > + xtype: 'proxmoxKVComboBox', > + name: 'mpTypeCombo', > + reference: 'mpTypeCombo', > + deleteEmpty: false, > + cbind: { > + hidden: '{isQemu}', > + }, > + comboItems: [ > + ['mp', gettext('Mount Point')], > + ['unused', gettext('Unused')], > + ], > + listeners: { change: 'onMpTypeChange' }, > + }, > + }, > + { > + xtype: 'proxmoxintegerfield', > + name: 'mpId', > + reference: 'mpIdSelector', > + minValue: 0, > + flex: 1, > + allowBlank: false, > + validateOnChange: true, > + disabled: true, > + bind: { > + maxValue: '{mpMaxCount}', > + }, > + validator: function(value) { > + let view = this.up('window'); > + let type = view.getViewModel().get('mpType'); > + if (Ext.isDefined(view.VMConfig[type+value])) { Style nit: could also use `${type}${value}` > + return "Mount point is already in use."; > + } > + return true; > + }, > + }, > + ], > + }, > + ], > + }, > + ], > + > + initComponent: function() { > + let me = this; > + > + if (!me.nodename) { > + throw "no node name specified"; > + } > + > + if (!me.vmid) { > + throw "no VM ID specified"; > + } > + > + if (!me.type) { > + throw "no type specified"; > + } > + > + me.callParent(); > + }, > +}); ----8<----