From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <s.hrdlicka@proxmox.com>
References: <20220719115707.4006326-1-s.hrdlicka@proxmox.com>
<20220719115707.4006326-2-s.hrdlicka@proxmox.com>
From: Fiona Ebner <f.ebner@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <s.hrdlicka@proxmox.com>
> ---
> 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 = [
> {