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

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

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 <s.hrdlicka@proxmox.com>
> ---
> 
> 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.

> +	});
>      },
>  });