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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 6839591980 for ; Thu, 29 Sep 2022 14:02:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 487823AFE for ; Thu, 29 Sep 2022 14:02:14 +0200 (CEST) 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 for ; Thu, 29 Sep 2022 14:02:12 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D50F74460E; Thu, 29 Sep 2022 14:02:05 +0200 (CEST) Message-ID: <3f9295bf-8160-1bec-3f83-8433a733fa0e@proxmox.com> Date: Thu, 29 Sep 2022 14:02:03 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Thunderbird/106.0 To: Proxmox VE development discussion , Stefan Hrdlicka References: <20220923124651.484774-1-s.hrdlicka@proxmox.com> <20220923124651.484774-2-s.hrdlicka@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20220923124651.484774-2-s.hrdlicka@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.229 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 -2.319 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 Subject: Re: [pve-devel] [PATCH V5 pve-manager 1/2] fix #2822: add iscsi, lvm, lvmthin & zfs storage for all cluster nodes 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, 29 Sep 2022 12:02:44 -0000 thanks for the resend, i can now apply it ;) i played a bit around with this and i'm not completely happy with the ux here. having 'localhost' as text in the box is not really helpful because the user might not actually know what 'localhost' actuall is. also it's possible to empty the field, of which the meaning is also not really clear? my suggestion would be to set allowBlank to false, and by default select the current nodename (available in Proxmox.NodeName) That way the current node is preselected, and there is no misunderstanding which node is scanned at any time. i'd leave the restriction logic like it is though, because if the storage is available everywhere, you don't have to change the nodename at all but if you change it you want it probably restricted (like already mentioned in a previous review) aside from that the patches LGTM On 9/23/22 14:46, Stefan Hrdlicka wrote: > This adds a dropdown box for iSCSI, LVM, LVMThin & ZFS storage options where a > cluster node needs to be chosen. As default the current node is > selected. It restricts the the storage to be only availabe on the > selected node. > > Signed-off-by: Stefan Hrdlicka > --- > www/manager6/Makefile | 2 + > www/manager6/form/ComboBoxSetStoreNode.js | 16 ++++++ > www/manager6/form/StorageScanNodeSelector.js | 30 +++++++++++ > www/manager6/storage/Base.js | 1 + > www/manager6/storage/IScsiEdit.js | 32 +++++++++--- > www/manager6/storage/LVMEdit.js | 29 +++++++++-- > www/manager6/storage/LvmThinEdit.js | 52 +++++++++++++++----- > www/manager6/storage/ZFSPoolEdit.js | 28 +++++++++-- > 8 files changed, 166 insertions(+), 24 deletions(-) > create mode 100644 www/manager6/form/ComboBoxSetStoreNode.js > create mode 100644 www/manager6/form/StorageScanNodeSelector.js > > diff --git a/www/manager6/Makefile b/www/manager6/Makefile > index d16770b1..81f5e5d8 100644 > --- a/www/manager6/Makefile > +++ b/www/manager6/Makefile > @@ -27,6 +27,7 @@ JSSRC= \ > form/CalendarEvent.js \ > form/CephPoolSelector.js \ > form/CephFSSelector.js \ > + form/ComboBoxSetStoreNode.js \ > form/CompressionSelector.js \ > form/ContentTypeSelector.js \ > form/ControllerSelector.js \ > @@ -62,6 +63,7 @@ JSSRC= \ > form/SecurityGroupSelector.js \ > form/SnapshotSelector.js \ > form/SpiceEnhancementSelector.js \ > + form/StorageScanNodeSelector.js \ > form/StorageSelector.js \ > form/TFASelector.js \ > form/TokenSelector.js \ > diff --git a/www/manager6/form/ComboBoxSetStoreNode.js b/www/manager6/form/ComboBoxSetStoreNode.js > new file mode 100644 > index 00000000..3490ddd7 > --- /dev/null > +++ b/www/manager6/form/ComboBoxSetStoreNode.js > @@ -0,0 +1,16 @@ > +Ext.define('PVE.form.ComboBoxSetStoreNode', { > + extend: 'Ext.form.field.ComboBox', > + config: { > + apiBaseUrl: '/api2/json/nodes/', > + apiSuffix: '', > + }, > + > + setNodeName: function(value) { > + let me = this; > + value ||= Proxmox.NodeName; > + > + me.getStore().getProxy().setUrl(`${me.apiBaseUrl}${value}${me.apiSuffix}`); > + this.clearValue(); > + }, > + > +}); > diff --git a/www/manager6/form/StorageScanNodeSelector.js b/www/manager6/form/StorageScanNodeSelector.js > new file mode 100644 > index 00000000..80188707 > --- /dev/null > +++ b/www/manager6/form/StorageScanNodeSelector.js > @@ -0,0 +1,30 @@ > +Ext.define('PVE.form.StorageScanNodeSelector', { > + extend: 'PVE.form.NodeSelector', > + xtype: 'pveStorageScanNodeSelector', > + > + name: 'storageScanNode', > + itemId: 'pveStorageScanNodeSelector', > + fieldLabel: gettext('Scan node'), > + allowBlank: true, > + disallowedNodes: undefined, > + autoSelect: false, > + submitValue: false, > + value: 'localhost', > + autoEl: { > + tag: 'div', > + 'data-qtip': gettext('Scan for available storages on the selected node'), > + }, > + triggers: { > + clear: { > + handler: function() { > + let me = this; > + me.setValue('localhost'); > + }, > + }, > + }, > + setValue: function(value) { > + let me = this; > + me.callParent([value]); > + me.triggers.clear.setVisible(me.triggers.clear.isVisible() && value !== 'localhost'); > + }, > +}); > diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js > index 7f6d7a09..1df7a8dd 100644 > --- a/www/manager6/storage/Base.js > +++ b/www/manager6/storage/Base.js > @@ -36,6 +36,7 @@ Ext.define('PVE.panel.StorageBase', { > { > xtype: 'pveNodeSelector', > name: 'nodes', > + reference: 'storageNodeRestriction', > disabled: me.storageId === 'local', > fieldLabel: gettext('Nodes'), > emptyText: gettext('All') + ' (' + gettext('No restrictions') +')', > diff --git a/www/manager6/storage/IScsiEdit.js b/www/manager6/storage/IScsiEdit.js > index 2f35f882..393de3c3 100644 > --- a/www/manager6/storage/IScsiEdit.js > +++ b/www/manager6/storage/IScsiEdit.js > @@ -1,5 +1,5 @@ > Ext.define('PVE.storage.IScsiScan', { > - extend: 'Ext.form.field.ComboBox', > + extend: 'PVE.form.ComboBoxSetStoreNode', > alias: 'widget.pveIScsiScan', > > queryParam: 'portal', > @@ -10,6 +10,9 @@ Ext.define('PVE.storage.IScsiScan', { > loadingText: gettext('Scanning...'), > width: 350, > }, > + config: { > + apiSuffix: '/scan/iscsi', > + }, > doRawQuery: function() { > // do nothing > }, > @@ -42,7 +45,7 @@ Ext.define('PVE.storage.IScsiScan', { > fields: ['target', 'portal'], > proxy: { > type: 'proxmox', > - url: `/api2/json/nodes/${me.nodename}/scan/iscsi`, > + url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`, > }, > }); > store.sort('target', 'ASC'); > @@ -78,6 +81,23 @@ Ext.define('PVE.storage.IScsiInputPanel', { > var me = this; > > me.column1 = [ > + { > + xtype: 'pveStorageScanNodeSelector', > + disabled: !me.isCreate, > + hidden: !me.isCreate, > + preferredValue: '', > + allowBlank: true, > + autoSelect: false, > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookup('iScsiTargetScan').setNodeName(value); > + me.lookup('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); > + }, > + }, > + }, > + }, > { > xtype: me.isCreate ? 'textfield' : 'displayfield', > name: 'portal', > @@ -94,14 +114,14 @@ Ext.define('PVE.storage.IScsiInputPanel', { > }, > }, > }, > - { > + Ext.createWidget(me.isCreate ? 'pveIScsiScan' : 'displayfield', { > readOnly: !me.isCreate, > - xtype: me.isCreate ? 'pveIScsiScan' : 'displayfield', > name: 'target', > value: '', > - fieldLabel: 'Target', > + fieldLabel: gettext('Target'), > allowBlank: false, > - }, > + reference: 'iScsiTargetScan', > + }), > ]; > > me.column2 = [ > diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js > index 2a9cd283..04d96786 100644 > --- a/www/manager6/storage/LVMEdit.js > +++ b/www/manager6/storage/LVMEdit.js > @@ -1,10 +1,20 @@ > Ext.define('PVE.storage.VgSelector', { > - extend: 'Ext.form.field.ComboBox', > + extend: 'PVE.form.ComboBoxSetStoreNode', > alias: 'widget.pveVgSelector', > valueField: 'vg', > displayField: 'vg', > queryMode: 'local', > editable: false, > + config: { > + apiSuffix: '/scan/lvm', > + }, > + > + setNodeName: function(value) { > + let me = this; > + me.callParent([value]); > + me.getStore().load(); > + }, > + > initComponent: function() { > var me = this; > > @@ -17,7 +27,7 @@ Ext.define('PVE.storage.VgSelector', { > fields: ['vg', 'size', 'free'], > proxy: { > type: 'proxmox', > - url: '/api2/json/nodes/' + me.nodename + '/scan/lvm', > + url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`, > }, > }); > > @@ -103,11 +113,24 @@ Ext.define('PVE.storage.LVMInputPanel', { > }); > > if (me.isCreate) { > - var vgField = Ext.create('PVE.storage.VgSelector', { > + let vgField = Ext.create('PVE.storage.VgSelector', { > name: 'vgname', > fieldLabel: gettext('Volume group'), > + reference: 'volumeGroupSelector', > allowBlank: false, > }); > + me.column1.push({ > + xtype: 'pveStorageScanNodeSelector', > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookup('volumeGroupSelector').setNodeName(value); > + me.lookup('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); > + }, > + }, > + }, > + }); > > var baseField = Ext.createWidget('pveFileSelector', { > name: 'base', > diff --git a/www/manager6/storage/LvmThinEdit.js b/www/manager6/storage/LvmThinEdit.js > index 4eab7740..e9202ffa 100644 > --- a/www/manager6/storage/LvmThinEdit.js > +++ b/www/manager6/storage/LvmThinEdit.js > @@ -1,5 +1,5 @@ > Ext.define('PVE.storage.TPoolSelector', { > - extend: 'Ext.form.field.ComboBox', > + extend: 'PVE.form.ComboBoxSetStoreNode', > alias: 'widget.pveTPSelector', > > queryParam: 'vg', > @@ -7,6 +7,10 @@ Ext.define('PVE.storage.TPoolSelector', { > displayField: 'lv', > editable: false, > > + config: { > + apiSuffix: '/scan/lvmthin', > + }, > + > doRawQuery: function() { > // nothing > }, > @@ -40,7 +44,7 @@ Ext.define('PVE.storage.TPoolSelector', { > fields: ['lv'], > proxy: { > type: 'proxmox', > - url: '/api2/json/nodes/' + me.nodename + '/scan/lvmthin', > + url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`, > }, > }); > > @@ -58,13 +62,23 @@ Ext.define('PVE.storage.TPoolSelector', { > }); > > Ext.define('PVE.storage.BaseVGSelector', { > - extend: 'Ext.form.field.ComboBox', > + extend: 'PVE.form.ComboBoxSetStoreNode', > alias: 'widget.pveBaseVGSelector', > > valueField: 'vg', > displayField: 'vg', > queryMode: 'local', > editable: false, > + config: { > + apiSuffix: '/scan/lvm', > + }, > + > + setNodeName: function(value) { > + let me = this; > + me.callParent([value]); > + me.getStore().load(); > + }, > + > initComponent: function() { > var me = this; > > @@ -77,7 +91,7 @@ Ext.define('PVE.storage.BaseVGSelector', { > fields: ['vg', 'size', 'free'], > proxy: { > type: 'proxmox', > - url: '/api2/json/nodes/' + me.nodename + '/scan/lvm', > + url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`, > }, > }); > > @@ -121,27 +135,41 @@ Ext.define('PVE.storage.LvmThinInputPanel', { > }); > > if (me.isCreate) { > - var vgField = Ext.create('PVE.storage.TPoolSelector', { > - name: 'thinpool', > - fieldLabel: gettext('Thin Pool'), > - allowBlank: false, > + me.column1.push({ > + xtype: 'pveStorageScanNodeSelector', > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookup('thinPoolSelector').setNodeName(value); > + me.lookup('volumeGroupSelector').setNodeName(value); > + me.lookup('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); > + }, > + }, > + }, > }); > > - me.column1.push({ > - xtype: 'pveBaseVGSelector', > + me.column1.push(Ext.create('PVE.storage.BaseVGSelector', { > name: 'vgname', > fieldLabel: gettext('Volume group'), > + reference: 'volumeGroupSelector', > listeners: { > change: function(f, value) { > if (me.isCreate) { > + let vgField = me.lookup('thinPoolSelector'); > vgField.setVG(value); > vgField.setValue(''); > } > }, > }, > - }); > + })); > > - me.column1.push(vgField); > + me.column1.push(Ext.create('PVE.storage.TPoolSelector', { > + name: 'thinpool', > + fieldLabel: gettext('Thin Pool'), > + reference: 'thinPoolSelector', > + allowBlank: false, > + })); > } > > me.column1.push(vgnameField); > diff --git a/www/manager6/storage/ZFSPoolEdit.js b/www/manager6/storage/ZFSPoolEdit.js > index 8e689f0c..98034de9 100644 > --- a/www/manager6/storage/ZFSPoolEdit.js > +++ b/www/manager6/storage/ZFSPoolEdit.js > @@ -1,5 +1,5 @@ > Ext.define('PVE.storage.ZFSPoolSelector', { > - extend: 'Ext.form.field.ComboBox', > + extend: 'PVE.form.ComboBoxSetStoreNode', > alias: 'widget.pveZFSPoolSelector', > valueField: 'pool', > displayField: 'pool', > @@ -8,6 +8,16 @@ Ext.define('PVE.storage.ZFSPoolSelector', { > listConfig: { > loadingText: gettext('Scanning...'), > }, > + config: { > + apiSuffix: '/scan/zfs', > + }, > + > + setNodeName: function(value) { > + let me = this; > + me.callParent([value]); > + me.getStore().load(); > + }, > + > initComponent: function() { > var me = this; > > @@ -20,10 +30,9 @@ Ext.define('PVE.storage.ZFSPoolSelector', { > fields: ['pool', 'size', 'free'], > proxy: { > type: 'proxmox', > - url: '/api2/json/nodes/' + me.nodename + '/scan/zfs', > + url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`, > }, > }); > - > store.sort('pool', 'ASC'); > > Ext.apply(me, { > @@ -45,9 +54,22 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', { > me.column1 = []; > > if (me.isCreate) { > + me.column1.push({ > + xtype: 'pveStorageScanNodeSelector', > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookup('zfsPoolSelector').setNodeName(value); > + me.lookup('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); > + }, > + }, > + }, > + }); > me.column1.push(Ext.create('PVE.storage.ZFSPoolSelector', { > name: 'pool', > fieldLabel: gettext('ZFS Pool'), > + reference: 'zfsPoolSelector', > allowBlank: false, > })); > } else {