[pve-devel] [PATCH V3 pve-manager 0/2] add iscsi, lvm, lvmthin & zfs storage on all cluster nodes (fix #2822)

[pve-devel] [PATCH V3 pve-manager 0/2] add iscsi, lvm, lvmthin & zfs storage on all cluster nodes (fix #2822)

[Stefan Hrdlicka]

V1 -> V2:
# pve-storage
* removed because patch is not needed

# pve-manager (1/3)
* remove storage controller from V1
* added custom ComboBox with API URL & setNodeName function
* added scan node selection for iSCSI 
* scan node selection field no longer send to server

## (optional) pve-manager (2/3): cleanup related files
* var to let statement change
* some indentation

## ((very) optional) pve-manager (3/3): cleanup all var statements
* replaces all var with let statements


V2 -> V3:
# pve-manager (1/2)
* fix broken interface (broken in V2 :()
* improve tooltip
* replace jNodeSelector function with class object
  (PVE.panel.StorageBaseWithNodeSelector)
* scan node selector empty by default, if selected it also sets the node
  restriction

# other things:
* removed "very optional cleanup"
* nothing changed for "Base storage" selector. It is still possible to
  select for example an iSCSI device only availabe on one node that
  isn't availabe on the other ones. I wasn't sure if this should be
  changed in this context as well.

Stefan Hrdlicka (2):
  fix #2822: add lvm, lvmthin & zfs storage for all cluster nodes
  cleanup: "var" to "let", fix some indentation in related files

 www/manager6/storage/Base.js        | 50 +++++++++++++++++++++---
 www/manager6/storage/IScsiEdit.js   | 60 +++++++++++++++++++++--------
 www/manager6/storage/LVMEdit.js     | 41 +++++++++++++++-----
 www/manager6/storage/LvmThinEdit.js | 60 +++++++++++++++++++++--------
 www/manager6/storage/ZFSPoolEdit.js | 55 ++++++++++++++++++--------
 5 files changed, 201 insertions(+), 65 deletions(-)

-- 
2.30.2

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

[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', {
+    extend: 'PVE.form.NodeSelector',
+    xtype: 'pveScanNodeSelector',
+
+    name: 'node',
+    itemId: 'pveScanNodeSelector',
+    fieldLabel: gettext('Scan node'),
+    allowBlank: true,
+    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: '',
+    },
+
+    setNodeName: function(value) {
+	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();
+    },
+
+});
+
 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,
 	    },
 	});
 	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,
+	    });
+	}
+	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 = [
 	    {
diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js
index 2a9cd283..e7f9344b 100644
--- a/www/manager6/storage/LVMEdit.js
+++ b/www/manager6/storage/LVMEdit.js
@@ -1,10 +1,20 @@
 Ext.define('PVE.storage.VgSelector', {
-    extend: 'Ext.form.field.ComboBox',
+    extend: 'PVE.storage.ComboBoxSetStoreNode',
     alias: 'widget.pveVgSelector',
     valueField: 'vg',
     displayField: 'vg',
     queryMode: 'local',
     editable: false,
+    config: {
+	apiStoragePath: '/scan/lvm',
+    },
+
+    setNodeName: function(value) {
+	let me = this;
+	me.callParent([value]);
+	me.getStore().load();
+    },
+
     initComponent: function() {
 	var me = this;
 
@@ -17,7 +27,7 @@ Ext.define('PVE.storage.VgSelector', {
 	    fields: ['vg', 'size', 'free'],
 	    proxy: {
 		type: 'proxmox',
-		url: '/api2/json/nodes/' + me.nodename + '/scan/lvm',
+		url: me.apiBaseUrl + me.nodename + me.apiStoragePath,
 	    },
 	});
 
@@ -103,11 +113,22 @@ Ext.define('PVE.storage.LVMInputPanel', {
 	});
 
 	if (me.isCreate) {
-	    var vgField = Ext.create('PVE.storage.VgSelector', {
+	    let vgField = Ext.create('PVE.storage.VgSelector', {
 		name: 'vgname',
 		fieldLabel: gettext('Volume group'),
 		allowBlank: false,
 	    });
+	    me.column1.push({
+	        xtype: 'pveScanNodeSelector',
+	        listeners: {
+	            change: {
+			fn: function(field, value) {
+			    vgField.setNodeName(value);
+			    me.lookupReference('storageNodeRestriction').setValue(value);
+			},
+		    },
+	        },
+	    });
 
 	    var baseField = Ext.createWidget('pveFileSelector', {
 		name: 'base',
diff --git a/www/manager6/storage/LvmThinEdit.js b/www/manager6/storage/LvmThinEdit.js
index 4eab7740..a109f7af 100644
--- a/www/manager6/storage/LvmThinEdit.js
+++ b/www/manager6/storage/LvmThinEdit.js
@@ -1,5 +1,5 @@
 Ext.define('PVE.storage.TPoolSelector', {
-    extend: 'Ext.form.field.ComboBox',
+    extend: 'PVE.storage.ComboBoxSetStoreNode',
     alias: 'widget.pveTPSelector',
 
     queryParam: 'vg',
@@ -7,6 +7,10 @@ Ext.define('PVE.storage.TPoolSelector', {
     displayField: 'lv',
     editable: false,
 
+    config: {
+	apiStoragePath: '/scan/lvmthin',
+    },
+
     doRawQuery: function() {
 	// nothing
     },
@@ -40,7 +44,7 @@ Ext.define('PVE.storage.TPoolSelector', {
 	    fields: ['lv'],
 	    proxy: {
 		type: 'proxmox',
-		url: '/api2/json/nodes/' + me.nodename + '/scan/lvmthin',
+		url: me.apiBaseUrl + me.nodename + me.apiStoragePath,
 	    },
 	});
 
@@ -58,13 +62,23 @@ Ext.define('PVE.storage.TPoolSelector', {
 });
 
 Ext.define('PVE.storage.BaseVGSelector', {
-    extend: 'Ext.form.field.ComboBox',
+    extend: 'PVE.storage.ComboBoxSetStoreNode',
     alias: 'widget.pveBaseVGSelector',
 
     valueField: 'vg',
     displayField: 'vg',
     queryMode: 'local',
     editable: false,
+    config: {
+	apiStoragePath: '/scan/lvm',
+    },
+
+    setNodeName: function(value) {
+	let me = this;
+	me.callParent([value]);
+	me.getStore().load();
+    },
+
     initComponent: function() {
 	var me = this;
 
@@ -77,7 +91,7 @@ Ext.define('PVE.storage.BaseVGSelector', {
 	    fields: ['vg', 'size', 'free'],
 	    proxy: {
 		type: 'proxmox',
-		url: '/api2/json/nodes/' + me.nodename + '/scan/lvm',
+		url: me.apiBaseUrl + me.nodename + me.apiStoragePath,
 	    },
 	});
 
@@ -121,14 +135,12 @@ Ext.define('PVE.storage.LvmThinInputPanel', {
 	});
 
 	if (me.isCreate) {
-	    var vgField = Ext.create('PVE.storage.TPoolSelector', {
+	    let vgField = Ext.create('PVE.storage.TPoolSelector', {
 		name: 'thinpool',
 		fieldLabel: gettext('Thin Pool'),
 		allowBlank: false,
 	    });
-
-	    me.column1.push({
-		xtype: 'pveBaseVGSelector',
+	    let vgSelector = Ext.create('PVE.storage.BaseVGSelector', {
 		name: 'vgname',
 		fieldLabel: gettext('Volume group'),
 		listeners: {
@@ -140,6 +152,20 @@ Ext.define('PVE.storage.LvmThinInputPanel', {
 		    },
 		},
 	    });
+	    me.column1.push({
+	        xtype: 'pveScanNodeSelector',
+	        listeners: {
+		    change: {
+			fn: function(field, value) {
+			    vgField.setNodeName(value);
+			    vgSelector.setNodeName(value);
+			    me.lookupReference('storageNodeRestriction').setValue(value);
+			},
+		    },
+		},
+	    });
+
+	    me.column1.push(vgSelector);
 
 	    me.column1.push(vgField);
 	}
diff --git a/www/manager6/storage/ZFSPoolEdit.js b/www/manager6/storage/ZFSPoolEdit.js
index 8e689f0c..1cb15b8b 100644
--- a/www/manager6/storage/ZFSPoolEdit.js
+++ b/www/manager6/storage/ZFSPoolEdit.js
@@ -1,5 +1,5 @@
 Ext.define('PVE.storage.ZFSPoolSelector', {
-    extend: 'Ext.form.field.ComboBox',
+    extend: 'PVE.storage.ComboBoxSetStoreNode',
     alias: 'widget.pveZFSPoolSelector',
     valueField: 'pool',
     displayField: 'pool',
@@ -8,6 +8,16 @@ Ext.define('PVE.storage.ZFSPoolSelector', {
     listConfig: {
 	loadingText: gettext('Scanning...'),
     },
+    config: {
+	apiStoragePath: '/scan/zfs',
+    },
+
+    setNodeName: function(value) {
+	let me = this;
+	me.callParent([value]);
+	me.getStore().load();
+    },
+
     initComponent: function() {
 	var me = this;
 
@@ -20,10 +30,9 @@ Ext.define('PVE.storage.ZFSPoolSelector', {
 	    fields: ['pool', 'size', 'free'],
 	    proxy: {
 		type: 'proxmox',
-		url: '/api2/json/nodes/' + me.nodename + '/scan/zfs',
+		url: me.apiBaseUrl + me.nodename + me.apiStoragePath,
 	    },
 	});
-
 	store.sort('pool', 'ASC');
 
 	Ext.apply(me, {
@@ -45,11 +54,24 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', {
 	me.column1 = [];
 
 	if (me.isCreate) {
-	    me.column1.push(Ext.create('PVE.storage.ZFSPoolSelector', {
+	    let zfsPoolSelector = Ext.create('PVE.storage.ZFSPoolSelector', {
 		name: 'pool',
 		fieldLabel: gettext('ZFS Pool'),
 		allowBlank: false,
-	    }));
+	    });
+
+	    me.column1.push({
+	        xtype: 'pveScanNodeSelector',
+	        listeners: {
+		    change: {
+			fn: function(field, value) {
+			    zfsPoolSelector.setNodeName(value);
+			    me.lookupReference('storageNodeRestriction').setValue(value);
+			},
+		    },
+		},
+	    });
+	    me.column1.push(zfsPoolSelector);
 	} else {
 	    me.column1.push(Ext.createWidget('displayfield', {
 		name: 'pool',
-- 
2.30.2

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

[Fiona Ebner]

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

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

[Stefan Hrdlicka]

Hello :)

On 7/27/22 12:19, Fiona Ebner wrote:
> 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

Exactly this one:
	url: `/api2/json/nodes/${me.nodename}/scan/iscsi` or do you mean or 
would something like this be ok as well:
	url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`



> 
>>   	    },
>>   	});
>>   	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 = [
>>   	    {
> 

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

[Fiona Ebner]

Am 01.08.22 um 16:30 schrieb Stefan Hrdlicka:
> Hello :)
> 
> On 7/27/22 12:19, Fiona Ebner wrote:
>> Am 19.07.22 um 13:57 schrieb Stefan Hrdlicka:
>>> @@ -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
> 
> Exactly this one:
>     url: `/api2/json/nodes/${me.nodename}/scan/iscsi` or do you mean or
> would something like this be ok as well:
>     url: `${me.apiBaseUrl}${me.nodename}${me.apiSuffix}`
> 

The second one please ;)
The change itself is fine, but the template string style is preferred.
In this case, it's not that big of a deal, but when mixing with actual
string literals, it's just nicer to read. And in other rare cases it can
prevent accidental numerical addition.

[pve-devel] [PATCH V3 pve-manager 2/2] cleanup: "var" to "let", fix some indentation in related files

[Stefan Hrdlicka]

replace all "var" with "let" in files related to patch for ticket 2822

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 www/manager6/storage/Base.js        | 10 +++++-----
 www/manager6/storage/IScsiEdit.js   |  6 +++---
 www/manager6/storage/LVMEdit.js     | 14 +++++++-------
 www/manager6/storage/LvmThinEdit.js | 18 +++++++++---------
 www/manager6/storage/ZFSPoolEdit.js | 23 +++++++++++------------
 5 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
index 28c5c9d0..602b53f3 100644
--- a/www/manager6/storage/Base.js
+++ b/www/manager6/storage/Base.js
@@ -5,7 +5,7 @@ Ext.define('PVE.panel.StorageBase', {
     type: '',
 
     onGetValues: function(values) {
-	var me = this;
+	let me = this;
 
 	if (me.isCreate) {
 	    values.type = me.type;
@@ -20,7 +20,7 @@ Ext.define('PVE.panel.StorageBase', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.column1.unshift({
 	    xtype: me.isCreate ? 'textfield' : 'displayfield',
@@ -127,7 +127,7 @@ Ext.define('PVE.storage.BaseEdit', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.isCreate = !me.storageId;
 
@@ -187,8 +187,8 @@ Ext.define('PVE.storage.BaseEdit', {
 	if (!me.isCreate) {
 	    me.load({
 		success: function(response, options) {
-		    var values = response.result.data;
-		    var ctypes = values.content || '';
+		    let values = response.result.data;
+		    let ctypes = values.content || '';
 
 		    values.content = ctypes.split(',');
 
diff --git a/www/manager6/storage/IScsiEdit.js b/www/manager6/storage/IScsiEdit.js
index 272c42d9..277d0545 100644
--- a/www/manager6/storage/IScsiEdit.js
+++ b/www/manager6/storage/IScsiEdit.js
@@ -64,7 +64,7 @@ Ext.define('PVE.storage.IScsiInputPanel', {
     onlineHelp: 'storage_open_iscsi',
 
     onGetValues: function(values) {
-	var me = this;
+	let me = this;
 
 	values.content = values.luns ? 'images' : 'none';
 	delete values.luns;
@@ -78,7 +78,7 @@ Ext.define('PVE.storage.IScsiInputPanel', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.column1 = [];
 	let target = null;
@@ -122,7 +122,7 @@ Ext.define('PVE.storage.IScsiInputPanel', {
 		listeners: {
 		    change: function(f, value) {
 			if (me.isCreate) {
-			    var exportField = me.down('field[name=target]');
+			    let exportField = me.down('field[name=target]');
 			    exportField.setPortal(value);
 			    exportField.setValue('');
 			}
diff --git a/www/manager6/storage/LVMEdit.js b/www/manager6/storage/LVMEdit.js
index e7f9344b..830fdd99 100644
--- a/www/manager6/storage/LVMEdit.js
+++ b/www/manager6/storage/LVMEdit.js
@@ -16,13 +16,13 @@ Ext.define('PVE.storage.VgSelector', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    me.nodename = 'localhost';
 	}
 
-	var store = Ext.create('Ext.data.Store', {
+	let store = Ext.create('Ext.data.Store', {
 	    autoLoad: {}, // true,
 	    fields: ['vg', 'size', 'free'],
 	    proxy: {
@@ -55,9 +55,9 @@ Ext.define('PVE.storage.BaseStorageSelector', {
     valueField: 'storage',
     displayField: 'text',
     initComponent: function() {
-	var me = this;
+	let me = this;
 
-	var store = Ext.create('Ext.data.Store', {
+	let store = Ext.create('Ext.data.Store', {
 	    autoLoad: {
 		addRecords: true,
 		params: {
@@ -99,11 +99,11 @@ Ext.define('PVE.storage.LVMInputPanel', {
     onlineHelp: 'storage_lvm',
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.column1 = [];
 
-	var vgnameField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
+	let vgnameField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
 	    name: 'vgname',
 	    hidden: !!me.isCreate,
 	    disabled: !!me.isCreate,
@@ -130,7 +130,7 @@ Ext.define('PVE.storage.LVMInputPanel', {
 	        },
 	    });
 
-	    var baseField = Ext.createWidget('pveFileSelector', {
+	    let baseField = Ext.createWidget('pveFileSelector', {
 		name: 'base',
 		hidden: true,
 		disabled: true,
diff --git a/www/manager6/storage/LvmThinEdit.js b/www/manager6/storage/LvmThinEdit.js
index a109f7af..725b7b83 100644
--- a/www/manager6/storage/LvmThinEdit.js
+++ b/www/manager6/storage/LvmThinEdit.js
@@ -16,7 +16,7 @@ Ext.define('PVE.storage.TPoolSelector', {
     },
 
     onTriggerClick: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.queryCaching || me.lastQuery !== me.vg) {
 	    me.store.removeAll();
@@ -28,19 +28,19 @@ Ext.define('PVE.storage.TPoolSelector', {
     },
 
     setVG: function(myvg) {
-	var me = this;
+	let me = this;
 
 	me.vg = myvg;
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    me.nodename = 'localhost';
 	}
 
-	var store = Ext.create('Ext.data.Store', {
+	let store = Ext.create('Ext.data.Store', {
 	    fields: ['lv'],
 	    proxy: {
 		type: 'proxmox',
@@ -80,13 +80,13 @@ Ext.define('PVE.storage.BaseVGSelector', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    me.nodename = 'localhost';
 	}
 
-	var store = Ext.create('Ext.data.Store', {
+	let store = Ext.create('Ext.data.Store', {
 	    autoLoad: {},
 	    fields: ['vg', 'size', 'free'],
 	    proxy: {
@@ -112,11 +112,11 @@ Ext.define('PVE.storage.LvmThinInputPanel', {
     onlineHelp: 'storage_lvmthin',
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.column1 = [];
 
-	var vgnameField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
+	let vgnameField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
 	    name: 'vgname',
 	    hidden: !!me.isCreate,
 	    disabled: !!me.isCreate,
@@ -125,7 +125,7 @@ Ext.define('PVE.storage.LvmThinInputPanel', {
 	    allowBlank: false,
 	});
 
-	var thinpoolField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
+	let thinpoolField = Ext.createWidget(me.isCreate ? 'textfield' : 'displayfield', {
 	    name: 'thinpool',
 	    hidden: !!me.isCreate,
 	    disabled: !!me.isCreate,
diff --git a/www/manager6/storage/ZFSPoolEdit.js b/www/manager6/storage/ZFSPoolEdit.js
index 1cb15b8b..c4ba71e0 100644
--- a/www/manager6/storage/ZFSPoolEdit.js
+++ b/www/manager6/storage/ZFSPoolEdit.js
@@ -19,13 +19,13 @@ Ext.define('PVE.storage.ZFSPoolSelector', {
     },
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    me.nodename = 'localhost';
 	}
 
-	var store = Ext.create('Ext.data.Store', {
+	let store = Ext.create('Ext.data.Store', {
 	    autoLoad: {}, // true,
 	    fields: ['pool', 'size', 'free'],
 	    proxy: {
@@ -49,7 +49,7 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', {
     onlineHelp: 'storage_zfspool',
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	me.column1 = [];
 
@@ -83,15 +83,14 @@ Ext.define('PVE.storage.ZFSPoolInputPanel', {
 
 	// value is an array,
 	// 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,
+	me.column1.push({
+	    xtype: 'pveContentTypeSelector',
+	    cts: ['images', 'rootdir'],
+	    fieldLabel: gettext('Content'),
+	    name: 'content',
+	    value: ['images', 'rootdir'],
+	    multiSelect: true,
+	    allowBlank: false,
 	});
 	me.column2 = [
 	    {
-- 
2.30.2