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

[Roland Kammerer]

On Tue, Mar 05, 2024 at 01:13:08PM +0100, Fabian Grünbichler wrote:
> I wonder whether the following wouldn't also work?
> 
> - keep the volume name on the PVE side like the other storage plugins
> (which means - encode vital information about the volume there so that
> it's possible to query *without* talking to the storage[0])
> - use a separate, unknown to PVE identifier on the DRBD side
> - store the association (either from volname to internal, or both
> ways) in linstore and potentially cache it on the PVE side if needed

Hi Fabian,

thanks for your feedback, highly appreciated, I'm pretty sure this is
exactly what I hinted in the cover letter with "completely virtualizing
the names", but did not want to go into too much details of alternative
implementations as the text was already very long.

I did a draft of that idea after running into the problem with
parse_volname initially. One can argue what the worse UX is, but giving
the users virtual vm-100-disk-1 names that have absolutely nothing to do
with the actual LINSTOR/DRBD resources did not sound to convincing
either. If things go sideways uses switch to the cli and for example
check the state of the DRBD resources. That is easier if they see the
actual pm-$UUID name in the PVE GUI already than to step through another
layer of virtual names.

If I go that route I have to store the mapping on a "global level" in
the LINSTOR controller, but if I only store the VM ID, I can store it in
the "object" associated with the LINSTOR resource/disk. If the disk gets
deleted, that is trivial, the "disk object" is deleted and so its VM ID
mapping. On the other hand if I need to store the mapping on the "global
level", then I also need some extra code to make sure that the entries
are actually consistent if things fail. Minor, but still.

What killed the idea for me was that it surprisingly complicated the
code dramatically. One needs to resolve these mappings way more often
than I expected. Everything that in the end talks to LINSTOR needs to
resolve that mapping. Also the parts that deal with snapshots became a
lot more tricky. With what I propose users (and in this case also the
plugin and LISNTOR) get what it is, pm-UUID names, the rest (except
parse_volname) then just works.

I perfectly understand that my suggested way is not perfect, it is a
trade off, but looking at both implementations this resulted in much
more reliable/maintainable code on the plugin's side, and not too much
hassle on the PVE side. Making parse_volname slightly worse vs. making
everything else in the plugin worse...

I give it one more round of thought, but most likely this will be a v2
with the changes Thomas requested if this is okay.

Regards, rck