From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 1B650632E0 for ; Mon, 24 Jan 2022 16:33:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 12571248DE for ; Mon, 24 Jan 2022 16:32:42 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 56621248D0 for ; Mon, 24 Jan 2022 16:32:41 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 26C1146139 for ; Mon, 24 Jan 2022 16:32:41 +0100 (CET) Message-ID: Date: Mon, 24 Jan 2022 16:32:39 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Thunderbird/97.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20220121145115.1374455-1-a.lauterer@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220121145115.1374455-1-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.058 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager] fix #3451 ui: ceph create osd: show custom device classes X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 24 Jan 2022 15:33:12 -0000 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//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 > --- > 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'),