Re: [pve-devel] [PATCH manager v4 3/6] added Config for Shared Filesystem Directories

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

Am 04/05/2023 um 10:31 schrieb Dominik Csapak:
> On 5/4/23 10:13, Thomas Lamprecht wrote:
>> Am 03/05/2023 um 13:26 schrieb Dominik Csapak:
>>> just a short comment since this series overlaps a bit with my
>>> cluster resource mapping series (i plan on sending a v4 soon).
>>>
>>> i'd prefer to have the configuration endpoints for mapping bundled in a subdirectory
>>> so instead of /nodes/<node>/dirs/ i'd put it in /nodes/<node>/mapping/dirs/
>>> (or /nodes/<node>/map/dirs )
>>>
>>> @thomas, @fabian, any other input on that?
>>>
>>
>> huh? aren't mappings per definition cluster wide i.e. /cluster/resource-map/<mapping-id>
>> than then allows to add/update the mapping of a resource on a specific node?
>> A node specific path makes no sense to me, at max it would if adding/removing a mapping
>> is completely decoupled from adding/removing/updaing entries to it – but that seems
>> convoluted from an usage POV and easy to get out of sync with the actual mapping list.
> 
> in markus series the mapping are only ever per node, so each node has it's
> own dir mapping

Every resource maping is always per node, so that's not really changing anything.

Rather what about migrations? Would be simpler from migration and ACL POV to have
it cluster wide,

> 
> in my series, the actual config was cluster-wide, but the api endpoint to configure
> them were sitting in the node path (e.g. /node/<nodes>/hardware-map/pci/*)

Please no.
 
> the reason is that to check the validity of the mapping (at least for creating/updating)
> need to happen at the node itself anyway since only that node can check it
> (e.g. for pci devices, if it exists, the ids are correct etc.)

That check is most relevant on using the map, not on updating/configuring it as there
the UX of getting the right one can be solved of providing a node selector per entry
that then loads the actual available devices/resources on that node. 

> 
> we *could* put them into the cluster path api, but we'd need to send a node parameter
> along and forward it there anyway, so that wouldn't really make a difference

no need for that, see above.

> 
> for reading the mappings, that could be done there, but in my series in the gui at least,
> i have to make a call to each node to get the current state of the mapping
> (e.g. if the pci device is still there)

For now not ideal but ok, in the future I'd rather go in the direction of broadcasting
some types of HW resources via pmxcfs KV and then this isn't an issue anymore.

> 
> if a mapping exists (globally) is not interesting most of the time, we only need to know
> if it exists at a specific node

that's looking at it backwards, the user and ACL only care for global mapings, how
the code implements that is then, well an implementation detail.

> 
> also, after seeing markus' patches, i also leaned more in the direction of splitting
> my global mapping config into a config per type+node (so node1/usb.conf, node1/pci.conf,

no, please no forest^W jungle of config trees :/

A /etc/pve/resource-map/<type>.conf must be enough, even a /etc/pve/resource-map.conf
should be tbh., but I could imagine that splitting per resource type makes some (schema)
things a bit easier and reduce some bug potential, so not to hard feelings on having one
cluster-wide per type; but really not more.