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 5E7FC93F86 for ; Tue, 20 Sep 2022 15:54:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 526151D4BF for ; Tue, 20 Sep 2022 15:53:55 +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 ; Tue, 20 Sep 2022 15:53:53 +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 073A543FA8; Tue, 20 Sep 2022 15:53:53 +0200 (CEST) Message-ID: <3a684562-91c4-9067-1539-5a3d2baa2182@proxmox.com> Date: Tue, 20 Sep 2022 15:53:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:105.0) Gecko/20100101 Thunderbird/105.0 To: Proxmox VE development discussion , Stefan Hrdlicka References: <20220804144617.1133163-1-s.hrdlicka@proxmox.com> <20220804144617.1133163-2-s.hrdlicka@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20220804144617.1133163-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.169 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.182 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 V4 manager 1/2] fix #2822: add 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: Tue, 20 Sep 2022 13:54:25 -0000 first, the patch does not apply here: ---- Applying: fix #2822: add lvm, lvmthin & zfs storage for all cluster nodes error: corrupt patch at line 335 Patch failed at 0001 fix #2822: add lvm, lvmthin & zfs storage for all cluster nodes ---- also, some comments/nits inline (aside from those, it looks ok to me, could not test because of the above issue) On 8/4/22 16:46, Stefan Hrdlicka wrote: > This adds a dropdown box for 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. > iscsi is not mentioned here ;) > Signed-off-by: Stefan Hrdlicka > --- > www/manager6/storage/Base.js | 49 +++++++++++++++++++++++++ > www/manager6/storage/IScsiEdit.js | 39 +++++++++++++++----- > www/manager6/storage/LVMEdit.js | 30 ++++++++++++++-- > www/manager6/storage/LvmThinEdit.js | 55 ++++++++++++++++++++++------- > www/manager6/storage/ZFSPoolEdit.js | 29 +++++++++++++-- > 5 files changed, 175 insertions(+), 27 deletions(-) > > diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js > index 7f6d7a09..d7dd13cc 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') +')', > @@ -76,6 +77,54 @@ Ext.define('PVE.panel.StorageBase', { > }, > }); > > +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'); > + }, > +}); > + > +Ext.define('PVE.storage.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(); > + }, > + > +}); nit: imho these two belong in www/manager6/form, but it's not a big deal > + > Ext.define('PVE.storage.BaseEdit', { > extend: 'Proxmox.window.Edit', > > diff --git a/www/manager6/storage/IScsiEdit.js b/www/manager6/storage/IScsiEdit.js > index 2f35f882..e9177f9e 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.storage.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'); > @@ -77,8 +80,25 @@ Ext.define('PVE.storage.IScsiInputPanel', { > initComponent: function() { > var me = this; > > - me.column1 = [ > - { > + me.column1 = []; > + me.column1.push({ > + xtype: 'pveStorageScanNodeSelector', > + disabled: !me.isCreate, > + hidden: !me.isCreate, > + preferredValue: '', > + allowBlank: true, > + autoSelect: false, > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookupReference('iScsiTargetScan').setNodeName(value); > + me.lookupReference('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value) nit: you could use 'lookup' instead of 'lookupReference' (it's just a short alias) > + }, > + }, > + }, > + }); > + me.column1.push({ > xtype: me.isCreate ? 'textfield' : 'displayfield', > name: 'portal', > value: '', > @@ -94,15 +114,16 @@ Ext.define('PVE.storage.IScsiInputPanel', { > }, > }, > }, > - { > + ); > + me.column1.push( > + Ext.createWidget(me.isCreate ? 'pveIScsiScan' : 'displayfield', { > readOnly: !me.isCreate, > - xtype: me.isCreate ? 'pveIScsiScan' : 'displayfield', > name: 'target', > value: '', > - fieldLabel: 'Target', > + fieldLabel: gettext('Target'), nit: it's ok, but does not really belong into this patch > allowBlank: false, > - }, > - ]; > + reference: 'iScsiTargetScan', > + })); any particular reason why you use '.push()' here? AFAICS you could simply define them inline like this: me.column1 = [ { xtype: ... ... }, ... ]; like it was before > > me.column2 = [ > { > diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js > index 2a9cd283..e8c5c88a 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.storage.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,25 @@ 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.lookupReference('volumeGroupSelector') > + .setNodeName(value); > + me.lookupReference('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); s/lookupReference/lookup/ > + }, > + }, > + }, > + }); > > var baseField = Ext.createWidget('pveFileSelector', { > name: 'base', > diff --git a/www/manager6/storage/LvmThinEdit.js b/www/manager6/storage/LvmThinEdit.js > index 4eab7740..7cd50746 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.storage.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.storage.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,44 @@ 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.lookupReference('thinPoolSelector') > + .setNodeName(value); > + me.lookupReference('volumeGroupSelector') > + .setNodeName(value); > + me.lookupReference('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); s/lookupReference/lookup/ > + }, > + }, > + }, > }); > > - 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.lookupReference('thinPoolSelector'); s/lookupReference/lookup/ > 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..d7319a10 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.storage.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}`, > }, > }); > - nit: also not really relevant for this patch > store.sort('pool', 'ASC'); > > Ext.apply(me, { > @@ -45,9 +54,23 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', { > me.column1 = []; > > if (me.isCreate) { > + me.column1.push({ > + xtype: 'pveStorageScanNodeSelector', > + listeners: { > + change: { > + fn: function(field, value) { > + me.lookupReference('zfsPoolSelector') > + .setNodeName(value); > + me.lookupReference('storageNodeRestriction') > + .setValue(value === 'localhost' ? '' : value); s/lookupReference/lookup/ > + }, > + }, > + }, > + }); > me.column1.push(Ext.create('PVE.storage.ZFSPoolSelector', { > name: 'pool', > fieldLabel: gettext('ZFS Pool'), > + reference: 'zfsPoolSelector', > allowBlank: false, > })); > } else {