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 BD0C970F89 for ; Tue, 7 Jun 2022 14:54:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B378D2DF2D for ; Tue, 7 Jun 2022 14:54:23 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 85F982DF20 for ; Tue, 7 Jun 2022 14:54:21 +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 5E51643A56 for ; Tue, 7 Jun 2022 14:54:21 +0200 (CEST) Message-ID: <4220bf4a-6f3c-d479-2478-bb516d2162fa@proxmox.com> Date: Tue, 7 Jun 2022 14:54:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Content-Language: en-US To: pve-devel@lists.proxmox.com, s.hrdlicka@proxmox.com References: <20220524144532.2698959-1-s.hrdlicka@proxmox.com> <20220524144532.2698959-3-s.hrdlicka@proxmox.com> From: Fabian Ebner In-Reply-To: <20220524144532.2698959-3-s.hrdlicka@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.507 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.889 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH pve-manager 2/2] fix #2822: add lvm, lvmthin & zfs storage on 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, 07 Jun 2022 12:54:53 -0000 Am 24.05.22 um 16:45 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. > IMHO it's not immediately clear what the "Scan node" is for if the selector is at the very top. I'd put it closer to the VG/thin pool/ZFS pool selectors it's affecting. And maybe worth adding a tooltip too? This patch changes the pre-selected nodes restriction to be the current node. Previously, it was all nodes. For LVM, when selecting an iSCSI storage as the base storage, the content/LUN listing could also be proxied to the "Scan node", because the storage might not be accessiable from the local node. > Signed-off-by: Stefan Hrdlicka > --- > > Depends on the change in pve-storage > > www/manager6/controller/StorageEdit.js | 20 ++++++++++++++++++++ > www/manager6/storage/Base.js | 25 +++++++++++++++++++++++++ > www/manager6/storage/LVMEdit.js | 9 ++++++++- > www/manager6/storage/LvmThinEdit.js | 14 +++++++++++++- > www/manager6/storage/ZFSPoolEdit.js | 24 +++++++++++++++--------- > 5 files changed, 81 insertions(+), 11 deletions(-) > > diff --git a/www/manager6/controller/StorageEdit.js b/www/manager6/controller/StorageEdit.js > index cb73b776..25745a5b 100644 > --- a/www/manager6/controller/StorageEdit.js > +++ b/www/manager6/controller/StorageEdit.js > @@ -25,3 +25,23 @@ Ext.define('PVE.controller.StorageEdit', { > }, > }, > }); > + > +Ext.define('PVE.storage.StorageLocalController', { > + extend: 'PVE.controller.StorageEdit', > + alias: 'controller.storageLocal', > + apiBaseUrl: '/api2/json/nodes/', > + > + clearValueSetUrl: function(reference, path, nodeSelector) { Nit: could pass nodeSelector.value directly and from the name it's not clear that the node restrictions are updated. > + var me = this; Please use let or const instead of var, see: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables > + > + var refObj = me.lookupReference(reference); > + refObj.clearValue(); > + > + var refObjStore = refObj.getStore(); > + refObjStore.getProxy().setUrl(me.apiBaseUrl + nodeSelector.value + path); > + refObjStore.load(); It's great that you were able to be so concise, but I'm not too happy about the bit of coupling that comes with it. To avoid accessing the internals of these other components, I'd teach them a setNodename() method. The clear() could also happen as part of the setNodename() methods. And we could call setNodename() and update the node restrictions directly, avoiding the need for this controller. But maybe others prefer the approach with the contorller? > + > + var nodeRestriction = me.lookupReference('nodeRestriction'); > + nodeRestriction.setValue(nodeSelector.value); > + }, > +}); > diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js > index 7f6d7a09..92654131 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: 'nodeRestriction', > disabled: me.storageId === 'local', > fieldLabel: gettext('Nodes'), > emptyText: gettext('All') + ' (' + gettext('No restrictions') +')', > @@ -76,6 +77,30 @@ Ext.define('PVE.panel.StorageBase', { > }, > }); > > +Ext.define('PVE.panel.StorageBaseLocal', { Not really accurate, because it's not a base for all local storage types and it's a base for LVM, which can also be shared. > + extend: 'PVE.panel.StorageBase', > + controller: 'storageLocal', > + > + initComponent: function() { > + var me = this; > + > + me.columnT = [ > + { > + xtype: 'pveNodeSelector', > + reference: 'pveNodeSelector', A bit generic for a reference, I'd at least include the "scan" part. > + name: 'node', > + itemId: 'pveNodeSelector', > + fieldLabel: gettext('Scan node'), > + allowBlank: false, > + disallowedNodes: undefined, > + onlineValidator: true, > + preferredValue: Proxmox.NodeName, > + }]; > + me.callParent(); > + }, > + > +}); > + > Ext.define('PVE.storage.BaseEdit', { > extend: 'Proxmox.window.Edit', > > diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js > index 2a9cd283..d6f8da06 100644 > --- a/www/manager6/storage/LVMEdit.js > +++ b/www/manager6/storage/LVMEdit.js > @@ -84,7 +84,7 @@ Ext.define('PVE.storage.BaseStorageSelector', { > }); > > Ext.define('PVE.storage.LVMInputPanel', { > - extend: 'PVE.panel.StorageBase', > + extend: 'PVE.panel.StorageBaseLocal', > > onlineHelp: 'storage_lvm', > > @@ -105,6 +105,7 @@ Ext.define('PVE.storage.LVMInputPanel', { > if (me.isCreate) { > var vgField = Ext.create('PVE.storage.VgSelector', { > name: 'vgname', > + reference: 'pveLVMVGSelector', > fieldLabel: gettext('Volume group'), > allowBlank: false, > }); > @@ -175,5 +176,11 @@ Ext.define('PVE.storage.LVMInputPanel', { > ]; > > me.callParent(); > + var nodeSelector = Ext.ComponentQuery.query('#pveNodeSelector')[0]; > + nodeSelector.on({ > + change: { > + fn: 'clearValueSetUrl', args: ['pveLVMVGSelector', '/scan/lvm'], > + }, Style nit: indented once too much. > + }); > }, > }); > diff --git a/www/manager6/storage/LvmThinEdit.js b/www/manager6/storage/LvmThinEdit.js > index 4eab7740..ab1abb4c 100644 > --- a/www/manager6/storage/LvmThinEdit.js > +++ b/www/manager6/storage/LvmThinEdit.js > @@ -93,7 +93,7 @@ Ext.define('PVE.storage.BaseVGSelector', { > }); > > Ext.define('PVE.storage.LvmThinInputPanel', { > - extend: 'PVE.panel.StorageBase', > + extend: 'PVE.panel.StorageBaseLocal', > > onlineHelp: 'storage_lvmthin', > > @@ -123,6 +123,7 @@ Ext.define('PVE.storage.LvmThinInputPanel', { > if (me.isCreate) { > var vgField = Ext.create('PVE.storage.TPoolSelector', { > name: 'thinpool', > + reference: 'pveLVMThinPoolSelector', > fieldLabel: gettext('Thin Pool'), > allowBlank: false, > }); > @@ -130,6 +131,7 @@ Ext.define('PVE.storage.LvmThinInputPanel', { > me.column1.push({ > xtype: 'pveBaseVGSelector', > name: 'vgname', > + reference: 'pveLVMThinVGSelector', > fieldLabel: gettext('Volume group'), > listeners: { > change: function(f, value) { > @@ -163,5 +165,15 @@ Ext.define('PVE.storage.LvmThinInputPanel', { > me.column2 = []; > > me.callParent(); > + > + var nodeSelector = Ext.ComponentQuery.query('#pveNodeSelector')[0]; > + nodeSelector.on({ > + change: { > + fn: function() { > + me.controller.clearValueSetUrl('pveLVMThinVGSelector', '/scan/lvm', nodeSelector); > + me.controller.clearValueSetUrl('pveLVMThinPoolSelector', '/scan/lvmthin', nodeSelector); > + }, > + }, Style nit: same here. > + }); > }, > }); > diff --git a/www/manager6/storage/ZFSPoolEdit.js b/www/manager6/storage/ZFSPoolEdit.js > index 8e689f0c..885f7a47 100644 > --- a/www/manager6/storage/ZFSPoolEdit.js > +++ b/www/manager6/storage/ZFSPoolEdit.js > @@ -2,6 +2,7 @@ Ext.define('PVE.storage.ZFSPoolSelector', { > extend: 'Ext.form.field.ComboBox', > alias: 'widget.pveZFSPoolSelector', > valueField: 'pool', > + reference: "pveZFSPoolSelector", > displayField: 'pool', > queryMode: 'local', > editable: false, > @@ -23,7 +24,6 @@ Ext.define('PVE.storage.ZFSPoolSelector', { > url: '/api2/json/nodes/' + me.nodename + '/scan/zfs', > }, > }); > - Nit: unrelated change. > store.sort('pool', 'ASC'); > > Ext.apply(me, { > @@ -35,7 +35,7 @@ Ext.define('PVE.storage.ZFSPoolSelector', { > }); > > Ext.define('PVE.storage.ZFSPoolInputPanel', { > - extend: 'PVE.panel.StorageBase', > + extend: 'PVE.panel.StorageBaseLocal', > > onlineHelp: 'storage_zfspool', > > @@ -63,13 +63,13 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', { > // while before it was a string > me.column1.push( > { > -xtype: 'pveContentTypeSelector', > - cts: ['images', 'rootdir'], > - fieldLabel: gettext('Content'), > - name: 'content', > - value: ['images', 'rootdir'], > - multiSelect: true, > - allowBlank: false, > + xtype: 'pveContentTypeSelector', > + cts: ['images', 'rootdir'], > + fieldLabel: gettext('Content'), > + name: 'content', > + value: ['images', 'rootdir'], > + multiSelect: true, > + allowBlank: false, Can still be improved, because of the mismatch between beginning me.column1.push( { and end }); It's an unrelated cleanup and thus should be its own patch. > }); > me.column2 = [ > { > @@ -89,5 +89,11 @@ xtype: 'pveContentTypeSelector', > ]; > > me.callParent(); > + var nodeSelector = Ext.ComponentQuery.query('#pveNodeSelector')[0]; > + nodeSelector.on({ > + change: { > + fn: 'clearValueSetUrl', args: ['pveZFSPoolSelector', '/scan/zfs'], > + }, Style nit: same here. > + }); > }, > });