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 4BEDD62474 for ; Mon, 21 Feb 2022 16:45:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4245D1B7CC for ; Mon, 21 Feb 2022 16:44:43 +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 7E9561B7C1 for ; Mon, 21 Feb 2022 16:44:41 +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 574E0462AD; Mon, 21 Feb 2022 16:44:41 +0100 (CET) Message-ID: Date: Mon, 21 Feb 2022 16:44:38 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Thunderbird/98.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20211115150209.717122-1-a.lauterer@proxmox.com> <20211115150209.717122-2-a.lauterer@proxmox.com> From: Dominik Csapak In-Reply-To: <20211115150209.717122-2-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.156 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 v2 manager 1/3] ui: lxc/qemu: add disk reassign 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: Mon, 21 Feb 2022 15:45:13 -0000 sorry for the late review some comments inline On 11/15/21 16:02, Aaron Lauterer wrote: > For the new HDReassign component, we follow the approach of HDMove to > have one componend for qemu and lxc as they are the same for most parts. > > The 'Move disk/volume' button is now a SplitButton which has the > 'Reassign' button as menu item. In the lxc resource panel the menu item > is defined extra so we can easily disable it, should the selected mp be > the rootfs. > > In the lxc resource and qemu hardware panel we currently add a new > button to handle unused disks/volumes. The button is "switched" with the > 'Move' in this case. The width of the buttons is aligned to avoid > movement of other buttons. > > Once we are able to also move unused disks/volumes to other storages, we > can remove this. > > Signed-off-by: Aaron Lauterer > --- > Not all cbind values can be omitted AFAICT as we do not have access to > the component context when declaring our items. From what I know, we can > use regular bind, cbind (if we only need to set the value once) or set > the values manually in the initComponent. > > Triggering the validation of the mountpoint integerfield when the mount > point type changes (onMpTypeChange) is happening because it is not done > so automatically but necessary as the other MP type (e.g. unused) could > already be in use with that number. There might be a better way that I > am not aware of. > > > changes since 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 | 316 ++++++++++++++++++++++++++++++ > www/manager6/qemu/HardwareView.js | 57 +++++- > 4 files changed, 432 insertions(+), 4 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 \ > 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..bec7cf14 100644 > --- a/www/manager6/lxc/Resources.js > +++ b/www/manager6/lxc/Resources.js > @@ -156,6 +156,11 @@ Ext.define('PVE.lxc.RessourceView', { > return; > } > > + if (rec.data.key.match(/^unused/)) { > + Ext.Msg.alert('Error', gettext('Not yet supported for unused volumes')); > + return; > + } > + since we already hide/disable that button accordingly, why have that error message at all? if the user somehow shows the button via the browser console, the api will return an error anyway.. that way, we'd save a gettext > var win = Ext.create('PVE.window.HDMove', { > disk: rec.data.key, > nodename: nodename, > @@ -168,6 +173,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, > @@ -227,12 +250,40 @@ Ext.define('PVE.lxc.RessourceView', { > }, > }); > > - var move_btn = new Proxmox.button.Button({ > + let reassign_menuitem = new Ext.menu.Item({ > + text: gettext('Reassign volume'), > + tooltip: gettext('Reassign volume to another VM'), > + handler: run_reassign, > + iconCls: 'fa fa-mail-forward', > + reference: 'reassing_item', > + }); > + > + let move_btn = new PVE.button.Split({ > text: gettext('Move Volume'), > selModel: me.selModel, > disabled: true, > dangerous: true, > handler: run_move, > + menu: { > + items: [reassign_menuitem], > + }, > + }); > + > + // needed until we can move unused volumes to other storages > + let reassign_btn = new Proxmox.button.Button({ > + text: gettext('Reassign volume'), > + tooltip: gettext('Reassign volume to another VM'), > + handler: run_reassign, > + selModel: me.selModel, > + hidden: true, > + listeners: { > + render: function(btn) { > + // hack: avoid layout-flickering due to size change, use max width for both > + let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width); > + move_btn.setSize({ width: maxWidth }); > + btn.setSize({ width: maxWidth }); mhmm... while for remove/detach disks this seems to work ok, for this the gettext are so different, the reassign button look very weird.. maybe putting both in a menu (where the top button itself has no action) and disable accordingly would make more sense here? i.e. Volume/Disk Actions - Resize - Move - relocate ? > + }, > + }, > }); > > var revert_btn = new PVE.button.PendingRevert(); > @@ -253,6 +304,7 @@ Ext.define('PVE.lxc.RessourceView', { > > var pending = rec.data.delete || me.hasPendingChanges(key); > var isDisk = rowdef.tdCls === 'pve-itype-icon-storage'; > + let isRootFS = rec.data.key === 'rootfs'; > var isUnusedDisk = key.match(/^unused\d+/); > var isUsedDisk = isDisk && !isUnusedDisk; > > @@ -265,7 +317,12 @@ Ext.define('PVE.lxc.RessourceView', { > } > edit_btn.setDisabled(noedit); > > - remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending); > + reassign_btn.setHidden(!isUnusedDisk); > + reassign_btn.setDisabled(!isUnusedDisk); > + move_btn.setHidden(isUnusedDisk); > + reassign_menuitem.setDisabled(isRootFS); > + > + remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending); > resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk); > move_btn.setDisabled(!isDisk || !diskCap); > revert_btn.setDisabled(!pending); > @@ -329,6 +386,7 @@ Ext.define('PVE.lxc.RessourceView', { > remove_btn, > resize_btn, > move_btn, > + reassign_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..e6b59cb4 > --- /dev/null > +++ b/www/manager6/qemu/HDReassign.js > @@ -0,0 +1,316 @@ > +Ext.define('PVE.window.HDReassign', { > + extend: 'Ext.window.Window', > + mixins: ['Proxmox.Mixin.CBind'], > + > + resizable: false, > + modal: true, > + width: 350, > + border: false, > + layout: 'fit', > + > + 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, > + }; > + }, > + > + cbind: { > + title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'), > + qemu: '{isQemu}', > + }, > + > + 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); > + }, > + > + reassign_disk: function(values) { this should probably be 'reassignDisk' according to our usual naming scheme > + let me = this; > + let view = me.getView(); > + let qemu = view.qemu; > + let params = { > + vmid: view.vmid, > + 'target-vmid': values.targetVmid, > + }; > + > + params[qemu ? 'disk' : 'volume'] = view.disk; > + > + if (view.qemu) { > + params['target-disk'] = `${values.controller}${values.deviceid}`; > + } else { > + params['target-volume'] = `${values.mpType}${values.mpId}`; > + } > + > + let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`; > + url += qemu ? 'move_disk' : 'move_volume'; > + > + Proxmox.Utils.API2Request({ > + params: params, > + url: url, > + waitMsgTarget: me.getView(), > + method: 'POST', > + failure: response => Ext.Msg.alert('Error', response.htmlStatus), > + success: function(response, options) { > + let upid = response.result.data; > + view.hide(); > + Ext.create('Proxmox.window.TaskProgress', { > + upid, > + autoShow: true, > + taskDone: success => success ? view.close() : view.show(), so if the task is not successful, we show ourselves again (over the error of the progress window)? what about the task log window (if opened)? > + }); > + }, > + }); > + }, > + > + validateForm: function(fp, isValid) { > + this.getView().lookup('submitButton').setDisabled(!isValid); > + }, > + > + onReassignClick: function() { > + let me = this; > + let view = me.getView(); > + let form = view.lookup('moveFormPanel').getForm(); > + if (form.isValid()) { > + me.reassign_disk(form.getValues()); > + } > + }, > + > + onMpTypeChange: function(value) { > + this.getView().getViewModel().set('mpType', value.getValue()); > + this.getView().lookup('mpIdSelector').validate(); > + }, these functions make me question if it wouldn't have been easier to use an 'edit window' instead? any reason for that? > + > + 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.type === 'qemu' ? 'qemu' : 'lxc'; here you use 'view.type' but above you use 'view.qemu' ? i'd prefer to have a consistent way of checking the type > + > + let url = `/nodes/${view.nodename}/${type}/${vmid}/config`; > + Proxmox.Utils.API2Request({ > + url: url, > + method: 'GET', > + failure: function(response, opts) { > + Ext.Msg.alert('Error', response.htmlStatus); > + }, > + success: function(response, options) { > + if (type === 'qemu') { same here > + 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) => { > + 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(); > + } > + }, > + }); > + }, > + }, > + > + buttons: [ > + { > + xtype: 'button', > + reference: 'submitButton', > + cbind: { > + text: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'), > + }, > + handler: 'onReassignClick', > + disabled: true, > + }, > + ], > + > + items: [ > + { > + xtype: 'form', > + reference: 'moveFormPanel', > + bodyPadding: 10, > + border: false, > + fieldDefaults: { > + labelWidth: 100, > + anchor: '100%', > + }, > + listeners: { > + validitychange: 'validateForm', > + }, > + items: [ > + { > + xtype: 'displayfield', > + name: 'sourceDisk', > + cbind: { > + name: get => get('isQemu') ? 'disk' : 'volume', > + fieldLabel: get => get('isQemu') > + ? gettext('Source Disk') > + : gettext('Source Mount Point'), wouldn't 'Source' suffice for both? IMHO the context is enough that the user knows that it's a disk/mp > + value: '{disk}', > + }, > + vtype: 'StorageId', > + allowBlank: false, > + }, > + { > + xtype: 'vmComboSelector', > + reference: 'targetVMID', > + name: 'targetVmid', > + allowBlank: false, > + bind: { > + value: '{targetVMID}', > + }, > + cbind: { > + fieldLabel: get => get('isQemu') ? gettext('Target VM') : gettext('Target CT'), same here but with 'Target' > + }, > + store: { > + model: 'PVEResources', > + autoLoad: true, > + sorters: 'vmid', > + cbind: {}, // for nested cbinds > + filters: [ > + { > + property: 'type', > + cbind: { > + value: get => get('isQemu') ? /qemu/ : /lxc/, > + }, > + }, > + { > + 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])) { > + 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(); > + }, > +}); > diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js > index 6cea4287..b76814e8 100644 > --- a/www/manager6/qemu/HardwareView.js > +++ b/www/manager6/qemu/HardwareView.js > @@ -406,6 +406,11 @@ Ext.define('PVE.qemu.HardwareView', { > return; > } > > + if (rec.data.key.match(/^unused/)) { > + Ext.Msg.alert('Error', gettext('Not yet supported for unused disks')); > + return; > + } > + same question as with containers > var win = Ext.create('PVE.window.HDMove', { > disk: rec.data.key, > nodename: nodename, > @@ -417,6 +422,24 @@ Ext.define('PVE.qemu.HardwareView', { > win.on('destroy', me.reload, me); > }; > > + let run_reassign = function() { > + let rec = sm.getSelection()[0]; > + if (!rec) { > + return; > + } > + > + Ext.create('PVE.window.HDReassign', { > + disk: rec.data.key, > + nodename: nodename, > + autoShow: true, > + vmid: vmid, > + type: 'qemu', > + listeners: { > + destroy: () => me.reload(), > + }, > + }); > + }; > + > var edit_btn = new Proxmox.button.Button({ > text: gettext('Edit'), > selModel: sm, > @@ -431,11 +454,36 @@ Ext.define('PVE.qemu.HardwareView', { > handler: run_resize, > }); > > - var move_btn = new Proxmox.button.Button({ > + let move_btn = new PVE.button.Split({ > text: gettext('Move disk'), > selModel: sm, > disabled: true, > handler: run_move, > + menu: { > + items: [{ > + text: gettext('Reassign disk'), > + tooltip: gettext('Reassign disk to another VM'), > + handler: run_reassign, > + iconCls: 'fa fa-mail-forward', > + }], > + }, > + }); > + > + // needed until we can move unused disk to other storages > + let reassign_btn = new Proxmox.button.Button({ > + text: gettext('Reassign disk'), > + tooltip: gettext('Reassign disk to another VM'), > + handler: run_reassign, > + selModel: sm, > + hidden: true, > + listeners: { > + render: function(btn) { > + // hack: avoid layout-flickering due to size change, use max width for both > + let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width); > + move_btn.setSize({ width: maxWidth }); > + btn.setSize({ width: maxWidth }); > + }, > + }, > }); > > var remove_btn = new Proxmox.button.Button({ > @@ -613,7 +661,11 @@ Ext.define('PVE.qemu.HardwareView', { > > resize_btn.setDisabled(pending || !isUsedDisk || !diskCap); > > - move_btn.setDisabled(pending || !(isUsedDisk || isEfi || tpmMoveable) || !diskCap); > + reassign_btn.setHidden(!isUnusedDisk); > + reassign_btn.setDisabled(!isUnusedDisk); > + move_btn.setHidden(isUnusedDisk); > + > + move_btn.setDisabled(pending || isCloudInit || !(isDisk || isEfi || tpmMoveable) || !diskCap); > > revert_btn.setDisabled(!pending); > }; > @@ -777,6 +829,7 @@ Ext.define('PVE.qemu.HardwareView', { > edit_btn, > resize_btn, > move_btn, > + reassign_btn, > revert_btn, > ], > rows: rows,