Re: [pve-devel] [PATCH V3 pve-manager 1/2] fix #2822: add lvm, lvmthin & zfs storage for all cluster nodes

[Fiona Ebner <f.ebner@proxmox.com>]

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 = [
>  	    {