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 8CD728A87D for ; Wed, 27 Jul 2022 12:20:34 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8438740EF for ; Wed, 27 Jul 2022 12:20:34 +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 ; Wed, 27 Jul 2022 12:20:33 +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 6D30A42C6B for ; Wed, 27 Jul 2022 12:20:33 +0200 (CEST) Message-ID: <1b8b07f3-b25d-8a88-3177-a060bc34af39@proxmox.com> Date: Wed, 27 Jul 2022 12:19:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: pve-devel@lists.proxmox.com, Stefan Hrdlicka References: <20220719115707.4006326-1-s.hrdlicka@proxmox.com> <20220719115707.4006326-2-s.hrdlicka@proxmox.com> From: Fiona Ebner In-Reply-To: <20220719115707.4006326-2-s.hrdlicka@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.041 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 Subject: Re: [pve-devel] [PATCH V3 pve-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: Wed, 27 Jul 2022 10:20:34 -0000 Am 19.07.22 um 13:57 schrieb Stefan Hrdlicka: > 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. > > Signed-off-by: Stefan Hrdlicka > --- > www/manager6/storage/Base.js | 40 +++++++++++++++++++++ > www/manager6/storage/IScsiEdit.js | 54 ++++++++++++++++++++++------- > www/manager6/storage/LVMEdit.js | 27 +++++++++++++-- > www/manager6/storage/LvmThinEdit.js | 42 +++++++++++++++++----- > www/manager6/storage/ZFSPoolEdit.js | 32 ++++++++++++++--- > 5 files changed, 166 insertions(+), 29 deletions(-) > > diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js > index 7f6d7a09..28c5c9d0 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,45 @@ Ext.define('PVE.panel.StorageBase', { > }, > }); > > +Ext.define('PVE.panel.StorageBaseWithNodeSelector', { It's not a panel and not a StorageBaseWithNodeSelector? How about PVE.form.ScanNodeSelector or StorageScanNodeSelector? If you go for the latter, please adapt the xtype too to make it match. > + extend: 'PVE.form.NodeSelector', > + xtype: 'pveScanNodeSelector', > + > + name: 'node', This is a too generic name. Again, how about storageScanNode? > + itemId: 'pveScanNodeSelector', > + fieldLabel: gettext('Scan node'), > + allowBlank: true, I'd really like to see an emptyText with "localhost" or similar here, so that it's clear what's being scanned if nothing is selected. Alternatively, the local node should be explicitly preselected (but without pre-selecting a node restriction to keep current default behavior). > + disallowedNodes: undefined, > + onlineValidator: true, > + autoSelect: false, > + submitValue: false, > + autoEl: { > + tag: 'div', > + 'data-qtip': gettext('Scan for available storages on the selected node'), > + }, > +}); > + > +Ext.define('PVE.storage.ComboBoxSetStoreNode', { > + extend: 'Ext.form.field.ComboBox', > + config: { > + apiBaseUrl: '/api2/json/nodes/', > + apiStoragePath: '', Very minor style nit: this class doesn't really depend on anything being a storage, so we could also call this 'apiSuffix' or something. Then the name would still work for any future (not storage-related) re-use of the class. > + }, > + > + setNodeName: function(value) { > + let me = this; > + if (value === null || value === '') { > + value = Proxmox.NodeName; > + } Could also use value ||= Proxmox.NodeName; > + > + let store = me.getStore(); > + let proxy = store.getProxy(); Style nit: no need for these single-use variables > + proxy.setUrl(me.apiBaseUrl + value + me.apiStoragePath); > + this.clearValue(); Style nit: can use 'me' again. > + }, > + > +}); > + > 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..272c42d9 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: { > + apiStoragePath: '/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.apiStoragePath, Style nit: please keep using template string syntax Same for the other ones below > }, > }); > store.sort('target', 'ASC'); > @@ -77,8 +80,40 @@ Ext.define('PVE.storage.IScsiInputPanel', { > initComponent: function() { > var me = this; > > - me.column1 = [ > - { > + me.column1 = []; > + let target = null; > + if (me.isCreate) { > + target = Ext.createWidget('pveIScsiScan', { > + readOnly: !me.isCreate, > + name: 'target', > + value: '', > + fieldLabel: 'Target', > + allowBlank: false, > + }); > + > + me.column1.push({ > + xtype: 'pveScanNodeSelector', > + preferredValue: '', > + allowBlank: true, > + autoSelect: false, > + listeners: { > + change: { > + fn: function(field, value) { > + target.setNodeName(value); > + me.lookupReference('storageNodeRestriction').setValue(value); > + }, > + }, > + }, > + }); > + } else { > + target = Ext.createWidget('displayfield', { > + name: 'target', > + value: '', > + fieldLabel: gettext('Target'), > + allowBlank: false, > + }); > + } Style nit: I think you could use disabled: !me.isCreate hidden: !me.isCreate for the scan node selector and in the change listener, get the 'target' via a lookup or reference, to avoid duplicating the definition of 'target' and keep the more declarative style. The part with the reference to keep more declarative style also applies to the other ones below. But no big deal. > + me.column1.push({ > xtype: me.isCreate ? 'textfield' : 'displayfield', > name: 'portal', > value: '', > @@ -94,15 +129,8 @@ Ext.define('PVE.storage.IScsiInputPanel', { > }, > }, > }, > - { > - readOnly: !me.isCreate, > - xtype: me.isCreate ? 'pveIScsiScan' : 'displayfield', > - name: 'target', > - value: '', > - fieldLabel: 'Target', > - allowBlank: false, > - }, > - ]; > + ); > + me.column1.push(target); > > me.column2 = [ > {