[pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[Aaron Lauterer]

Makes it possible to configure the RBD namespace via the GUI.

RBD namespaces must be configured manually. The most likely use case is
when connecting to an external Ceph cluster as this makes it possible to
separate client PVE clusters by namespace, not by pool.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Right now the warning field is below the checkbox to select between
external or hyperconverged ceph.

I wasn't sure which position would be better. Above that checkbox or
directly below the namespace field (same column)?

 www/manager6/storage/RBDEdit.js | 56 +++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/www/manager6/storage/RBDEdit.js b/www/manager6/storage/RBDEdit.js
index be29dc8a..fad9305b 100644
--- a/www/manager6/storage/RBDEdit.js
+++ b/www/manager6/storage/RBDEdit.js
@@ -5,6 +5,7 @@ Ext.define('PVE.storage.Ceph.Model', {
     data: {
 	pveceph: true,
 	pvecephPossible: true,
+	namespacePresent: false,
     },
 });
 
@@ -26,10 +27,18 @@ Ext.define('PVE.storage.Ceph.Controller', {
 	    disable: 'resetField',
 	    enable: 'resetField',
 	},
+	'textfield[name=namespace]': {
+	    change: 'updateNamespaceHint',
+	},
     },
     resetField: function(field) {
 	field.reset();
     },
+    updateNamespaceHint(field, newVal, oldVal) {
+	var vm = this.getViewModel();
+	let namespacePresent = newVal? true : false;
+	vm.set('namespacePresent', namespacePresent);
+    },
     queryMonitors: function(field, newVal, oldVal) {
 	// we get called with two signatures, the above one for a field
 	// change event and the afterrender from the view, this check only
@@ -88,6 +97,9 @@ Ext.define('PVE.storage.RBDInputPanel', {
 	    this.lookupReference('pvecephRef').setValue(false);
 	    this.lookupReference('pvecephRef').resetOriginalValue();
 	}
+	if (values.namespace) {
+	    this.viewModel.set('namespacePresent', true);
+	}
 	this.callParent([values]);
     },
 
@@ -170,6 +182,13 @@ Ext.define('PVE.storage.RBDInputPanel', {
 		fieldLabel: gettext('User name'),
 		allowBlank: true,
 	    },
+	    {
+		xtype: me.isCreate ? 'textfield' : 'displayfield',
+		name: 'namespace',
+		value: '',
+		fieldLabel: gettext('Namespace'),
+		allowBlank: true,
+	    },
 	);
 
 	me.column2 = [
@@ -190,20 +209,31 @@ Ext.define('PVE.storage.RBDInputPanel', {
 	    },
 	];
 
-	me.columnB = [{
-	    xtype: 'proxmoxcheckbox',
-	    name: 'pveceph',
-	    reference: 'pvecephRef',
-	    bind: {
-		disabled: '{!pvecephPossible}',
-		value: '{pveceph}',
+	me.columnB = [
+	    {
+		xtype: 'proxmoxcheckbox',
+		name: 'pveceph',
+		reference: 'pvecephRef',
+		bind: {
+		    disabled: '{!pvecephPossible}',
+		    value: '{pveceph}',
+		},
+		checked: true,
+		uncheckedValue: 0,
+		submitValue: false,
+		hidden: !me.isCreate,
+		boxLabel: gettext('Use Proxmox VE managed hyper-converged ceph pool'),
+	    },
+	    {
+		xtype: 'displayfield',
+		name: 'namespace-hint',
+		userCls: 'pmx-hint',
+		value: gettext('RBD namespaces must be created manually!'),
+		bind: {
+		    hidden: '{!namespacePresent}',
+		},
 	    },
-	    checked: true,
-	    uncheckedValue: 0,
-	    submitValue: false,
-	    hidden: !me.isCreate,
-	    boxLabel: gettext('Use Proxmox VE managed hyper-converged ceph pool'),
-	}];
+	];
 
 	me.callParent();
     },
-- 
2.20.1

Re: [pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[Thomas Lamprecht]

On 16.04.21 16:10, Aaron Lauterer wrote:
> Makes it possible to configure the RBD namespace via the GUI.
> 
> RBD namespaces must be configured manually. The most likely use case is
> when connecting to an external Ceph cluster as this makes it possible to
> separate client PVE clusters by namespace, not by pool.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Right now the warning field is below the checkbox to select between
> external or hyperconverged ceph.
> 
> I wasn't sure which position would be better. Above that checkbox or
> directly below the namespace field (same column)?
> 
>  www/manager6/storage/RBDEdit.js | 56 +++++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/www/manager6/storage/RBDEdit.js b/www/manager6/storage/RBDEdit.js
> index be29dc8a..fad9305b 100644
> --- a/www/manager6/storage/RBDEdit.js
> +++ b/www/manager6/storage/RBDEdit.js
> @@ -5,6 +5,7 @@ Ext.define('PVE.storage.Ceph.Model', {
>      data: {
>  	pveceph: true,
>  	pvecephPossible: true,
> +	namespacePresent: false,
>      },
>  });
>  
> @@ -26,10 +27,18 @@ Ext.define('PVE.storage.Ceph.Controller', {
>  	    disable: 'resetField',
>  	    enable: 'resetField',
>  	},
> +	'textfield[name=namespace]': {
> +	    change: 'updateNamespaceHint',
> +	},
>      },
>      resetField: function(field) {
>  	field.reset();
>      },
> +    updateNamespaceHint(field, newVal, oldVal) {

why isn't this a function like the others?

> +	var vm = this.getViewModel();
> +	let namespacePresent = newVal? true : false;

we really need to get eslint pronto for pve-manager, as it would complain about above :)
A ternary resolving to bool almost never makes sense.

The whole thing can be

updateNamespaceHint: function(field, newVal, oldVal) {
    this.getViewModel().set('namespacePresent', newVal);
}


> +	vm.set('namespacePresent', namespacePresent);
> +    },
>      queryMonitors: function(field, newVal, oldVal) {
>  	// we get called with two signatures, the above one for a field
>  	// change event and the afterrender from the view, this check only
> @@ -88,6 +97,9 @@ Ext.define('PVE.storage.RBDInputPanel', {
>  	    this.lookupReference('pvecephRef').setValue(false);
>  	    this.lookupReference('pvecephRef').resetOriginalValue();
>  	}
> +	if (values.namespace) {
> +	    this.viewModel.set('namespacePresent', true);

use the getter for the view model, like you do already above in updateNamespaceHint:
this.getViewModel().set(...)

> +	}
>  	this.callParent([values]);
>      },
>  
> @@ -170,6 +182,13 @@ Ext.define('PVE.storage.RBDInputPanel', {
>  		fieldLabel: gettext('User name'),
>  		allowBlank: true,
>  	    },
> +	    {
> +		xtype: me.isCreate ? 'textfield' : 'displayfield',

you could use a 'pmxDisplayEditField' instead

> +		name: 'namespace',
> +		value: '',
> +		fieldLabel: gettext('Namespace'),
> +		allowBlank: true,
> +	    },
>  	);
>  
>  	me.column2 = [
> @@ -190,20 +209,31 @@ Ext.define('PVE.storage.RBDInputPanel', {
>  	    },
>  	];
>  
> -	me.columnB = [{
> -	    xtype: 'proxmoxcheckbox',
> -	    name: 'pveceph',
> -	    reference: 'pvecephRef',
> -	    bind: {
> -		disabled: '{!pvecephPossible}',
> -		value: '{pveceph}',
> +	me.columnB = [
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		name: 'pveceph',
> +		reference: 'pvecephRef',
> +		bind: {
> +		    disabled: '{!pvecephPossible}',
> +		    value: '{pveceph}',
> +		},
> +		checked: true,
> +		uncheckedValue: 0,
> +		submitValue: false,
> +		hidden: !me.isCreate,
> +		boxLabel: gettext('Use Proxmox VE managed hyper-converged ceph pool'),
> +	    },
> +	    {
> +		xtype: 'displayfield',
> +		name: 'namespace-hint',
> +		userCls: 'pmx-hint',
> +		value: gettext('RBD namespaces must be created manually!'),
> +		bind: {
> +		    hidden: '{!namespacePresent}',
> +		},

Alternatively we could probe the rbd storage if the namespace is present on submit?

I.e., do the RADOS command equivalent to `rbd namespace list -p <pool` in the
storage add/update hook if the namespace param is present?

>  	    },
> -	    checked: true,
> -	    uncheckedValue: 0,
> -	    submitValue: false,
> -	    hidden: !me.isCreate,
> -	    boxLabel: gettext('Use Proxmox VE managed hyper-converged ceph pool'),
> -	}];
> +	];
>  
>  	me.callParent();
>      },
> 

Re: [pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[Aaron Lauterer]

Thx for the feedback and yep, I forgot to run the linter...

On 4/16/21 4:56 PM, Thomas Lamprecht wrote:
> On 16.04.21 16:10, Aaron Lauterer wrote:
>> Makes it possible to configure the RBD namespace via the GUI.

[....]

>> +		xtype: 'displayfield',
>> +		name: 'namespace-hint',
>> +		userCls: 'pmx-hint',
>> +		value: gettext('RBD namespaces must be created manually!'),
>> +		bind: {
>> +		    hidden: '{!namespacePresent}',
>> +		},
> 
> Alternatively we could probe the rbd storage if the namespace is present on submit?
> 
> I.e., do the RADOS command equivalent to `rbd namespace list -p <pool` in the
> storage add/update hook if the namespace param is present?

Anything against doing it via `rbd -p pool namespace ls`? AFAICT there is no lightweight command to list namespaces via `rados`. Only thing I found was listing all objects in the pool and fetching used namespaces from that output: `rados -p rbd ls --format json` but that is an expensive operation.

One thing though that we should consider: so far adding an external RBD storage worked even if the auth keyring wasn't present. The storage would not get activated until the keyring file was present. But one could still do that after adding the storage config. With this check we would make it a requirement to first place the keyring file and then add the storage config.

> 
>>   	    },
>> -	    checked: true,
>> -	    uncheckedValue: 0,
>> -	    submitValue: false,
>> -	    hidden: !me.isCreate,
>> -	    boxLabel: gettext('Use Proxmox VE managed hyper-converged ceph pool'),
>> -	}];
>> +	];
>>   
>>   	me.callParent();
>>       },
>>
> 

Re: [pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[Thomas Lamprecht]

On 16.04.21 17:39, Aaron Lauterer wrote:
> Thx for the feedback and yep, I forgot to run the linter...
> 

there's to much noise still in it for this repo, so totally understandable.

> Anything against doing it via `rbd -p pool namespace ls`? AFAICT there is no
> lightweight command to list namespaces via `rados`. Only thing I found was
> listing all objects in the pool and fetching used namespaces from that
> output: `rados -p rbd ls --format json` but that is an expensive operation.

I mean, this is not a frequent operation, so we could ignore the overhead of
fork + exec. So yes, I would accept that. If we find something more efficient 
it could always get replaced by that transparently any way.

> One thing though that we should consider: so far adding an external RBD
> storage worked even if the auth keyring wasn't present. The storage would
> not get activated until the keyring file was present. But one could still do
> that after adding the storage config. With this check we would make it a
> requirement to first place the keyring file and then add the storage config.

Actually, I requested the feature to be able to add one through the storage
add and update API/web-interface a long time ago from Alwin, was lost in the
cogs of time though...

So; I do not see that as issue but rather as another small feature we could
finally do.

Handling in the backend should be similar to other secrets, e.g., pbs password
or encryption-key, cifs password, etc.

Re: [pve-devel] [PATCH manager] ui: RBDStorage: add field for RBD namespace

[Aaron Lauterer]

On 4/16/21 5:48 PM, Thomas Lamprecht wrote:
> On 16.04.21 17:39, Aaron Lauterer wrote:
>> Thx for the feedback and yep, I forgot to run the linter...
>>
> 
> there's to much noise still in it for this repo, so totally understandable.
> 
>> Anything against doing it via `rbd -p pool namespace ls`? AFAICT there is no
>> lightweight command to list namespaces via `rados`. Only thing I found was
>> listing all objects in the pool and fetching used namespaces from that
>> output: `rados -p rbd ls --format json` but that is an expensive operation.
> 
> I mean, this is not a frequent operation, so we could ignore the overhead of
> fork + exec. So yes, I would accept that. If we find something more efficient
> it could always get replaced by that transparently any way.
> 
>> One thing though that we should consider: so far adding an external RBD
>> storage worked even if the auth keyring wasn't present. The storage would
>> not get activated until the keyring file was present. But one could still do
>> that after adding the storage config. With this check we would make it a
>> requirement to first place the keyring file and then add the storage config.
> 
> Actually, I requested the feature to be able to add one through the storage
> add and update API/web-interface a long time ago from Alwin, was lost in the
> cogs of time though...
> 
> So; I do not see that as issue but rather as another small feature we could
> finally do.
> 
> Handling in the backend should be similar to other secrets, e.g., pbs password
> or encryption-key, cifs password, etc.

Okay, good to know. So I guess I will send a v2 of this patch and put more back-end checks (pool and namespace can be accessed/seen) on my todo list in combination with the possibility to pass the auth key for external RBD pools directly at creation time.