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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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'),