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 6F6248A14F for ; Mon, 1 Aug 2022 16:30:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5FE8030BFD for ; Mon, 1 Aug 2022 16:30:33 +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 ; Mon, 1 Aug 2022 16:30:31 +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 5226340F21 for ; Mon, 1 Aug 2022 16:30:31 +0200 (CEST) Message-ID: <6ba2ac6a-8c65-fc13-aa8f-c78e02e1042c@proxmox.com> Date: Mon, 1 Aug 2022 16:30:29 +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: Fiona Ebner , pve-devel@lists.proxmox.com References: <20220719115707.4006326-1-s.hrdlicka@proxmox.com> <20220719115707.4006326-2-s.hrdlicka@proxmox.com> <1b8b07f3-b25d-8a88-3177-a060bc34af39@proxmox.com> From: Stefan Hrdlicka In-Reply-To: <1b8b07f3-b25d-8a88-3177-a060bc34af39@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.067 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: Mon, 01 Aug 2022 14:30:33 -0000 Hello :) On 7/27/22 12:19, Fiona Ebner wrote: > 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 Exactly this one: url: `/api2/json/nodes/${me.nodename}/scan/iscsi` or do you mean or would something like this be ok as well: url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}` > >> }, >> }); >> 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 = [ >> { >