[pve-devel] [PATCH manager] fix #3451 ui: ceph create osd: show custom device classes

[pve-devel] [PATCH manager] fix #3451 ui: ceph create osd: show custom device classes

[Aaron Lauterer]

Showing already configured custom device classes makes it easier to
create new OSDs with custom device classes.

The Crush map contains a list of all OSDs in the cluster, including
their device class.
This means we can create a list of used device classes from it, avoiding
adding another API endpoint.

Fetching the crushmap should also be quite a bit less data that needs to
be transferred, compared to the other possible nodes/<node>/ceph/osd
endpoint, especially in larger clusters.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/ceph/OSD.js | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index 30671dc4..eb27d3b3 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -17,6 +17,38 @@ Ext.define('PVE.CephCreateOsd', {
 
 	me.isCreate = true;
 
+	let deviceClasses = [
+	    ['hdd', 'HDD'],
+	    ['ssd', 'SSD'],
+	    ['nvme', 'NVMe'],
+	];
+
+	Proxmox.Utils.API2Request({
+	    url: "/nodes/" + me.nodename + "/ceph/crush",
+	    method: 'GET',
+	    failure: function(response, opts) {
+		let msg = response.htmlStatus;
+		console.error('Failed to load crushmap', msg);
+		// We still need to show the default list of device classes
+		me.down('field[name=crush-device-class]').setComboItems(deviceClasses);
+	    },
+	    success: function(response, opts) {
+		let data = response.result.data;
+
+		let classes = new Set();
+		for (const match of data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)) {
+		    classes.add(match[1]);
+		}
+		for (const v of ['hdd', 'ssd', 'nvme']) {
+		    classes.delete(v);
+		}
+		for (const v of classes) {
+		    deviceClasses.push([v, v]);
+		}
+		me.down('field[name=crush-device-class]').setComboItems(deviceClasses);
+	    },
+	});
+
         Ext.applyIf(me, {
 	    url: "/nodes/" + me.nodename + "/ceph/osd",
 	    method: 'POST',
@@ -81,11 +113,6 @@ Ext.define('PVE.CephCreateOsd', {
 			},
 			{
 			    xtype: 'proxmoxKVComboBox',
-			    comboItems: [
-				['hdd', 'HDD'],
-				['ssd', 'SSD'],
-				['nvme', 'NVMe'],
-			    ],
 			    name: 'crush-device-class',
 			    nodename: me.nodename,
 			    fieldLabel: gettext('Device Class'),
-- 
2.30.2

Re: [pve-devel] [PATCH manager] fix #3451 ui: ceph create osd: show custom device classes

[Thomas Lamprecht]

On 21.01.22 15:51, Aaron Lauterer wrote:
> Showing already configured custom device classes makes it easier to
> create new OSDs with custom device classes.
> 
> The Crush map contains a list of all OSDs in the cluster, including
> their device class.
> This means we can create a list of used device classes from it, avoiding
> adding another API endpoint.
> 
> Fetching the crushmap should also be quite a bit less data that needs to
> be transferred, compared to the other possible nodes/<node>/ceph/osd
> endpoint, especially in larger clusters.
> 

thanks for this, I may have went a bit overboard on review, but JS sometimes
jumps at me and while I'd appreciate if you checked the comments and adapt
what seems sensible I won't put my foot down on nak'ing this on or the like.

> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  www/manager6/ceph/OSD.js | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
> index 30671dc4..eb27d3b3 100644
> --- a/www/manager6/ceph/OSD.js
> +++ b/www/manager6/ceph/OSD.js
> @@ -17,6 +17,38 @@ Ext.define('PVE.CephCreateOsd', {
>  
>  	me.isCreate = true;
>  
> +	let deviceClasses = [
> +	    ['hdd', 'HDD'],
> +	    ['ssd', 'SSD'],
> +	    ['nvme', 'NVMe'],
> +	];
> +
> +	Proxmox.Utils.API2Request({
> +	    url: "/nodes/" + me.nodename + "/ceph/crush",

please use a template string here

url: `/nodes/${me.nodename}/ceph/crush`,

> +	    method: 'GET',
> +	    failure: function(response, opts) {
> +		let msg = response.htmlStatus;

useless intermediate variable

> +		console.error('Failed to load crushmap', msg);

we normally alert to the user about API errors, or is this one failing
more often (false positive like)?

> +		// We still need to show the default list of device classes
> +		me.down('field[name=crush-device-class]').setComboItems(deviceClasses);

You could have kept setting the comboItems directly at the element definition
below then, or?

In general, most API failure definitions are probably fine to be the single line of

failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),

> +	    },
> +	    success: function(response, opts) {
> +		let data = response.result.data;
> +
> +		let classes = new Set();
> +		for (const match of data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)) {
> +		    classes.add(match[1]);
> +		}
> +		for (const v of ['hdd', 'ssd', 'nvme']) {
> +		    classes.delete(v);
> +		}
> +		for (const v of classes) {
> +		    deviceClasses.push([v, v]);
> +		}
> +		me.down('field[name=crush-device-class]').setComboItems(deviceClasses);

Something like the following would be a bit more concise while also only changing
the field if required, I did not test this at all besides verifying that eslint
eats it though ^^

success: function({ result: { data } }) {
    let classes = data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)
        .filter(v => !['hdd', 'ssd', 'nvme'].includes(v))
        .map(v => [v, v]);

    if (classes.length) {
        let kvField = me.down('field[name=crush-device-class]').setComboItems(classes);
        classes.unshift(kvField.comboItems);

        kvField.setComboItems(classes);
    }
},

Note, above wouldn't be OK if not in initComponent, i.e., if called more than once,
wouldn't be hard to fix but not required here FWICT.


> +	    },
> +	});
> +
>          Ext.applyIf(me, {
>  	    url: "/nodes/" + me.nodename + "/ceph/osd",
>  	    method: 'POST',
> @@ -81,11 +113,6 @@ Ext.define('PVE.CephCreateOsd', {
>  			},
>  			{
>  			    xtype: 'proxmoxKVComboBox',
> -			    comboItems: [
> -				['hdd', 'HDD'],
> -				['ssd', 'SSD'],
> -				['nvme', 'NVMe'],
> -			    ],
>  			    name: 'crush-device-class',
>  			    nodename: me.nodename,
>  			    fieldLabel: gettext('Device Class'),

Re: [pve-devel] [PATCH manager] fix #3451 ui: ceph create osd: show custom device classes

[Thomas Lamprecht]

On 24.01.22 16:32, Thomas Lamprecht wrote:
> On 21.01.22 15:51, Aaron Lauterer wrote:
>> +	    success: function(response, opts) {
>> +		let data = response.result.data;
>> +
>> +		let classes = new Set();
>> +		for (const match of data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)) {
>> +		    classes.add(match[1]);
>> +		}
>> +		for (const v of ['hdd', 'ssd', 'nvme']) {
>> +		    classes.delete(v);
>> +		}
>> +		for (const v of classes) {
>> +		    deviceClasses.push([v, v]);
>> +		}
>> +		me.down('field[name=crush-device-class]').setComboItems(deviceClasses);
> 
> Something like the following would be a bit more concise while also only changing
> the field if required, I did not test this at all besides verifying that eslint
> eats it though ^^
> 
> success: function({ result: { data } }) {
>     let classes = data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)
>         .filter(v => !['hdd', 'ssd', 'nvme'].includes(v))
>         .map(v => [v, v]);

Oh, forget to filter repeated clases here, so that would actually be something along
the lines of:

let classes = [...new Set(
    data.matchAll(/^device\s[0-9]*\sosd\.[0-9]*\sclass\s(.*)$/gim)
        .filter(v => !['hdd', 'ssd', 'nvme'].includes(v))
)].map(v => [v, v]);

> 
>     if (classes.length) {
>         let kvField = me.down('field[name=crush-device-class]').setComboItems(classes);
>         classes.unshift(kvField.comboItems);
> 
>         kvField.setComboItems(classes);
>     }
> },