[pve-devel] [PATCH storage 1/1] storage/plugins: pass scfg to parse_volname

[Roland Kammerer]

On Wed, Mar 13, 2024 at 04:38:57PM +0100, Wolfgang Bumiller wrote:
> My thoughts on this: (TLDR: we should just merge it and probably also
> consider adding a separate method to get the *format* of a volid)
> 
> - Adding the parameter itself is fine, not thinking about how/why it is
>   used. Generally, it makes sense for all storage API methods to also
>   know the storage's config anyway.
> - Most (if not all) invocations that actually need the owner vmid (which
>   is the part which becomes expensive here) AFAICT are already within a
>   more expensive context anyway.
> - We have a *lot* of callers which actually only want the disk *format*,
>   which IMO means we could introduce a separate storage API call for
>   this (this can be backward compatible with a fallback to the parse
>   method if the plugin does not provide the new method) 

FWIW all of that sounds very reasonable to me makes sense to me as a
plugin maintainer.

I did not follow up with a v2 because I think we found a solution that
would work for us and would not require this patch (while I still also
think it would be good in general to pass the storage config along).

For those interested, quick history:

- Solution 1:
keep "virtual" vm-$VMID-disk-$DID names in PVE, but use pm-$UUID on
LINSTOR level. Store the mapping in the LINSTOR "DB".

Pro: keeps the usual names, no patches required

Con: pretty hard for users to debug which "virtual" disk belongs to
which actual LINSTOR/DRBD disk.

- Solution 2:
use pm-UUID disks everywhere. Store the VMID in the LINSTOR DB.

Pro: obvious mapping as users see the real thing

Con: requires the proposed patch; the burst cache is kind of nasty after
all

- Solution 3:

kind of a mix. Use pm-$UUID as LINSTOR/DRBD disks, but use
pm-$UUID_$VMID as "virtual" PVE names. Store the VMID in the LINSTOR DB.

Pro: we can keep things as they are, as we can still regex-parse the
VMID from the PVE names. Mapping PVE names to actual LINSTOR/DRBD names
is trivial in the plugin, just cut off the "_$VMID". It is also
obvious/trivial for actual users to map what they see in the GUI to the
actual devices.

Also I only take the first 8 chars of a UUID, it does not have to be
unique globaly, it just does not have to exist in LINSTOR already, so
the actual names are rather short and handy.

Once more, thanks for all of your input, I wrote the draft for solution
3 yesterday, if things work out as expected I think we can drop the
proposed patch while thinking it would have made sense in general.

Regards, rck