Re: [pve-devel] [PATCH manager v3 2/2] ui: only allow rbd pools to be added as rbd storage

[Stefan Sterz <s.sterz@proxmox.com>]

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 <s.sterz@proxmox.com>
>> ---
>> 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]);
>>  		}
>>  	    },
>>  	});
>