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 CD2918AA44 for ; Fri, 21 Oct 2022 09:03:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A799B1E538 for ; Fri, 21 Oct 2022 09:02:58 +0200 (CEST) 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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 21 Oct 2022 09:02:57 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0816744AF7 for ; Fri, 21 Oct 2022 09:02:57 +0200 (CEST) Message-ID: <573cedfe-f43d-d219-7302-263622172a1f@proxmox.com> Date: Fri, 21 Oct 2022 09:02:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 From: Stefan Sterz To: Thomas Lamprecht , Proxmox VE development discussion References: <20221020071704.46578-1-s.sterz@proxmox.com> <20221020071704.46578-2-s.sterz@proxmox.com> <2d020cdd-9cea-48ef-d267-94bcc37d0b27@proxmox.com> Content-Language: en-US In-Reply-To: <2d020cdd-9cea-48ef-d267-94bcc37d0b27@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.111 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 v3 2/2] ui: only allow rbd pools to be added as rbd storage 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: Fri, 21 Oct 2022 07:03:28 -0000 On 10/20/22 15:00, Thomas Lamprecht wrote: > Am 20/10/2022 um 09:17 schrieb Stefan Sterz: >> previously the ui would allow adding all pools (even the default >> ceph-mon pools) as storage. this could lead to issues when users did >> use these pools as storage (e.g.: vms missing their disks after a >> migration). hence, restrict the pool selector to rbd pools. >> >> fails gracefully by reverting to the previous behavior if a pool has >> no application assigned to it. >> >> Signed-off-by: Stefan Sterz >> --- >> v3: changed the name of the filter function based on alwin antreich's >> suggestion >> >> www/manager6/form/CephPoolSelector.js | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/www/manager6/form/CephPoolSelector.js b/www/manager6/form/CephPoolSelector.js >> index 5b96398d..b98feb54 100644 >> --- a/www/manager6/form/CephPoolSelector.js >> +++ b/www/manager6/form/CephPoolSelector.js >> @@ -15,9 +15,17 @@ Ext.define('PVE.form.CephPoolSelector', { >> throw "no nodename given"; >> } >> >> + let onlyCephRBDPools = (item) => { >> + let apps = item.data?.applications; >> + return apps === undefined || apps?.rbd !== undefined; > > couldn't this be a one liner? and as nit I'd maybe drop the Ceph from the name, > suggests a bit that we got non-ceph rbd pools. > > // filter out rgw, metadata, cephfs, ... pools > onlyRBDPools = ({data}) => !!data?.applications?.rbd; > > i see your point on variable naming, but this isn't equivalent afaiu. the idea here was that the ui reverts back to including a pool if there is no application defined for it (this should help make the ui change independent from the api change). this wouldn't do that. you could do: onlyRBDPools = ({ data }) => !data?.applications || !!data?.applications?.rbd imo still pretty long, but maybe you have another trick up your sleeve :) if you don't care about keeping the front-end compatible with the previous behavior we can also use your one-liner. just wanted to err on the side of caution. as a sidenote: personally i also prefer checking for undefined-ness to relying on its falsy nature. e.g. if application.rbd = null, this would also return false, even though technically rbd is defined. since ceph seems to return empty objects for rbd pools by default, this currently works fine. however, id prefer being more explicit. this is just a personal preference though, so i'll do whatever you prefer :) there is also the "in" operator, but if irc another patch of mine once got rejected for using that. > more generally, the application could be also shown in our ceph pool list, could > be useful to see the usage type of each pool, would also reduce confusion for the > a bit odd metadata pool with its single PG. > sure i suppose that makes sense, i can add that to a v4. >> + }; >> + >> var store = Ext.create('Ext.data.Store', { >> fields: ['name'], >> sorters: 'name', >> + filters: [ >> + onlyCephRBDPools, >> + ], >> proxy: { >> type: 'proxmox', >> url: '/api2/json/nodes/' + me.nodename + '/ceph/pools', >> @@ -32,8 +40,10 @@ Ext.define('PVE.form.CephPoolSelector', { >> >> store.load({ >> callback: function(rec, op, success) { >> - if (success && rec.length > 0) { >> - me.select(rec[0]); >> + let filteredRec = rec.filter(onlyCephRBDPools); >> + >> + if (success && filteredRec.length > 0) { >> + me.select(filteredRec[0]); >> } >> }, >> }); >