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

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

Am 22.06.22 um 16:39 schrieb Stefan Hrdlicka:
> This adds a dropdown box for iSCSI, 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.

I don't think we should change the default restriction, especially for
iSCSI and (shared) LVM, but also for local ones, as in many cases,
cluster nodes will be set-up with similar storage and the new default
might trip up some people.

> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  www/manager6/storage/Base.js        | 44 ++++++++++++++++++++++-
>  www/manager6/storage/IScsiEdit.js   | 54 ++++++++++++++++++++++-------
>  www/manager6/storage/LVMEdit.js     | 21 +++++++++--
>  www/manager6/storage/LvmThinEdit.js | 35 ++++++++++++++-----
>  www/manager6/storage/ZFSPoolEdit.js | 24 ++++++++++---
>  5 files changed, 148 insertions(+), 30 deletions(-)
> 
> diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
> index 7f6d7a09..bb497a5f 100644
> --- a/www/manager6/storage/Base.js
> +++ b/www/manager6/storage/Base.js
> @@ -34,8 +34,9 @@ Ext.define('PVE.panel.StorageBase', {
>  	me.column2 = me.column2 || [];
>  	me.column2.unshift(
>  	    {
> -		xtype: 'pveNodeSelector',
> +		xtype: 'pveScanNodeSelector',

This class/type alias is never defined? And why change the restriction
selector to a scan selector?

>  		name: 'nodes',
> +		reference: 'storageNodeRestriction',
>  		disabled: me.storageId === 'local',
>  		fieldLabel: gettext('Nodes'),
>  		emptyText: gettext('All') + ' (' + gettext('No restrictions') +')',
> @@ -74,6 +75,47 @@ Ext.define('PVE.panel.StorageBase', {
>  
>  	me.callParent();
>      },
> +    getPveScanNodeSelector: function() {

No need for this function. The properties can be set as part of the
class definition and overriden by the specific instances as needed.

> +	return {
> +	    xtype: 'pveScanNodeSelector',
> +	    name: 'node',
> +	    itemId: 'pveScanNodeSelector',
> +	    fieldLabel: gettext('Scan node'),
> +	    allowBlank: false,
> +	    disallowedNodes: undefined,
> +	    onlineValidator: true,
> +	    preferredValue: Proxmox.NodeName,
> +	    submitValue: false,
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext('Look for availabe storage options from selected node.'),

"Look" and "from" sound a bit strange to me (also typo in "availabe").
Maybe "Scan for available storages on (the) selected node"?

> +	    },
> +	};
> +    },
> +});
> +
> +Ext.define('PVE.storage.ComboBoxSetStoreNode', {
> +    extend: 'Ext.form.field.ComboBox',
> +    config: {
> +	apiBaseUrl: '/api2/json/nodes/',
> +	apiStoragePath: '',
> +    },
> +
> +    setNodeName: function(value, storeLoad = true) {
> +	let me = this;
> +	if (value === null || value === '') {
> +	    value = Proxmox.NodeName;
> +	}
> +
> +	let store = me.getStore();
> +	let proxy = store.getProxy();
> +	proxy.setUrl(me.apiBaseUrl + value + me.apiStoragePath);
> +	this.clearValue();
> +	if (storeLoad) {
> +	    store.load();
> +	}
> +    },
> +
>  });
>  
>  Ext.define('PVE.storage.BaseEdit', {

(...)

> @@ -140,6 +145,20 @@ Ext.define('PVE.storage.LvmThinInputPanel', {
>  		    },
>  		},
>  	    });
> +	    let pveScanNodeSelector = me.getPveScanNodeSelector();
> +	    pveScanNodeSelector.listeners = {
> +	        change: {
> +		    fn: function(field, value) {
> +			// don't reload the store, it requires a VG selected
> +			vgField.setNodeName(value, false);

I'd rather drop the additional parameter from setNodeName and override
it for this class. The class itself knows best when it can load and the
'vg' property should be reset as part of the setNodeName implementation.

> +			vgSelector.setNodeName(value);
> +			me.lookupReference('storageNodeRestriction').setValue(value);
> +		    },
> +	        },
> +	    };
> +	    me.column1.push(pveScanNodeSelector);
> +
> +	    me.column1.push(vgSelector);
>  
>  	    me.column1.push(vgField);
>  	}