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

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

[Fiona Ebner]

Am 23.02.24 um 10:24 schrieb Roland Kammerer:
> This passes the well known $scfg to parse_volname and bumps the API
> versions accordingly. This allows plugins to access their configuration
> if necessary.
> 

Hi,
can you please describe your use case in more detail? Why does parsing
the volume name depend on something in the storage configuration?

Best Regards,
Fiona

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

[Dietmar Maurer]

> On 29.2.2024 16:09 CET Roland Kammerer via pve-devel <pve-devel@lists.proxmox.com> wrote:
> All in all, yes, this is specific for our use case, otherwise
> parse_volname would already have that additional parameter as all the
> other plugin functions, but I don't see where this would hurt existing
> code, and it certainly helps us to enable reassigning disks to VMs.
> Passing in a param all other functions already get access to also does
> not sound too terrible to me.
> 
> If there are further questions please feel free to ask.

Are you aware that parse_volname() is sometimes called for
all volumes, i.e inside volume_is_base_and_used().

Would that be fast enough? IMHO its a bad idea to make a network query for each volume there...

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

[Fabian Grünbichler]

Roland Kammerer wrote:
> On Thu, Feb 29, 2024 at 02:29:51PM +0100, Fiona Ebner wrote:
> > Am 23.02.24 um 10:24 schrieb Roland Kammerer:
> > > This passes the well known $scfg to parse_volname and bumps the API
> > > versions accordingly. This allows plugins to access their configuration
> > > if necessary.
> > > 
> > 
> > Hi,
> > can you please describe your use case in more detail? Why does parsing
> > the volume name depend on something in the storage configuration?
> 
> Hi,
> 
> I can try, hopefully not only repeating what I wrote in the cover
> letter:
> 
> - We want to implement reassigning disks to different VMs.
> - this calls the plugin's rename_volume.
> 
> Usually this renames a disk from vm-100-disk-1 to something like
> vm-101-disk-1. Great, no changes needed to parse_volume, the (new) VM ID
> can be parsed from the name with a regex and the actual disk.
> 
> For us this would mean that we would need to rename the resource in
> LINSTOR, the DRBD devices on all nodes, the backing device for the DRBD
> resources on all nodes,... For various reasons that is terribly hard,
> especially considering cluster wide rollbacks. If you want me to, I can
> fill some pages about that :).
> 
> So I came up with this:
> 
> - Let's not store the VM ID in the name of the resource/device, but
>   let's generate resource/"disk" names like pm-$UUID. The LINSTOR
>   resource is named pm-$UUID, the DRBD device is named pm-$UUID, the
>   backing devices on LVM/ZFS are named /dev/vg/pm-$UUID...
> - Great, we abstracted away the VM IDs. So where do we then store the VM
>   ID that for example parse_volname needs to return the actual VM ID?
> - We can store that in meta data that is associated with the
>   LINSTOR/DRBD resource. We have a central component, the LINSTOR
>   controller, the "brain of the SDS", which is needed anyways and it's
>   IP is already in the storage configuration. Just think of it as a
>   "data base".
> - So in that "data base" we store that pm-123... belongs to VM ID 100.
> - Reassigning a disk to a new VM (ID) is then trivial, we just update
>   the "data base" entry for pm-$UUID, now pointing to VM ID 101 (or
>   whatever). That is exactly what I do in rename_volume. I don't
>   actually rename the LINSTOR/DRBD/LVM objects/disks, I just update the
>   mapping. When getting called for "reassing disks to a new VM", the
>   plugin is free to generate a name. I return the old/existing name, but
>   the new VM ID. Yes, that is a bit if cheating, but works perfectly
>   fine for that scenario of reassinging disks to VMs.
> - Whenever we need the VM ID, we don't parse it from the pm-$UUID name,
>   we can't, but we ask our "data base".
> - In oder to ask the "data base", we need to know its IP. And for that
>   we need a pointer to the storage configuration that (already) contains
>   that IP.
> - all plugin functions already get a parameter to $scfg. With one
>   exception: parse_volname.
> 
> All in all, yes, this is specific for our use case, otherwise
> parse_volname would already have that additional parameter as all the
> other plugin functions, but I don't see where this would hurt existing
> code, and it certainly helps us to enable reassigning disks to VMs.
> Passing in a param all other functions already get access to also does
> not sound too terrible to me.
> 
> If there are further questions please feel free to ask.

thanks for spelling out your rationale!

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

that way, the interface can remain as it is. renaming is just a metadata update (change volname on the PVE side, update mapping in linstore), no need to change the internal DRBD identifier or any related entities. there is no chance of accidentally triggering a storm of requests or a return of outdated information just by calls to parse_volname. you only need to query the mapping between PVE volname and DRBD identifier when you actually need to do stuff with the volume on the DRBD end (e.g., things that already have $scfg at the moment).

the reason parse_volname doesn't get the storage config entry at the moment is exactly that we want to force plugins to encode the basic information in the volname, so that we can rely on it being cheap to call everywhere.

basically, instead of your proposed mapping of volname to "information usually encoded in volname" via linstore, the approach lined out above would be a mapping of volname "with all the basic information" to DRBD-internal, real volume ID via linstore. there wouldn't be a fundamental difference to other storages that also have to adapt the "logical" volname into full information via some means (e.g., extend it with the actual base dir, or add (lib)rbd related information, or translating it to a block device path, or ..), except that maybe it's a tad more expensive than usual (although, with storage access, all bets are off anyway ;)).

if there is no way to make the above work for DRBD, extending parse_volname is of course still an option like Thomas said in his last reply, but such a change should be a last resort IMHO.

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

[Thomas Lamprecht]

Am 23/02/2024 um 10:24 schrieb Roland Kammerer:
> This passes the well known $scfg to parse_volname and bumps the API
> versions accordingly. This allows plugins to access their configuration
> if necessary.

We discussed this another time here and effectively it can be fine, while
the need for it seems like a slight smell from our architecture POV, as
it basically always means that the VMID -> volume mapping is not encoded
in the name any more (worse UX), it still does not hurt our, or other
external, existing plug-ins.

So fine to add, but please also the parameter also to the base
"parse_volname" method including a comment that mentions that this
is in general not used and only required if the storage cannot
provide all required information in the volume name.
 > Signed-off-by: Roland Kammerer <roland.kammerer@linbit.com>
> ---
>  ApiChangeLog       |  7 +++++++
>  src/PVE/Storage.pm | 18 +++++++++---------
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 98b5893..042e13b 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -6,6 +6,13 @@ without breaking anything unaware of it.)
>  
>  Future changes should be documented in here.
>  
> +##  Version 11:
> +
> +* `parse_volname` got additional `$scfg` parameter
> +
> +  This allows plugins to use information from their configuration.
> +  As this is the last, additional parameter APIAGE is not reset.
> +
>  ##  Version 10:
>  
>  * a new `rename_volume` method has been added
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 0beabbc..3ac4f1e 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -41,11 +41,11 @@ use PVE::Storage::PBSPlugin;
>  use PVE::Storage::BTRFSPlugin;
>  
>  # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> +use constant APIVER => 11;
>  # Age is the number of versions we're backward compatible with.
>  # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>  # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 1;
> +use constant APIAGE => 2;
>  

unrelated, but wondering if we could rework the approach how we warn about
to old plugins somewhat, as currently this is IMO rather to noisy, especially
for a change like this one.

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

[Wolfgang Bumiller]

On Tue, Mar 05, 2024 at 12:13:05PM +0100, Thomas Lamprecht wrote:
> Am 23/02/2024 um 10:24 schrieb Roland Kammerer:
> > This passes the well known $scfg to parse_volname and bumps the API
> > versions accordingly. This allows plugins to access their configuration
> > if necessary.
> 
> We discussed this another time here and effectively it can be fine, while
> the need for it seems like a slight smell from our architecture POV, as
> it basically always means that the VMID -> volume mapping is not encoded
> in the name any more (worse UX), it still does not hurt our, or other
> external, existing plug-ins.
> 
> So fine to add, but please also the parameter also to the base
> "parse_volname" method including a comment that mentions that this
> is in general not used and only required if the storage cannot
> provide all required information in the volume name.

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) 

./API2/LXC/Config.pm:199:                           $format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
^ update_vm call, attaching volumes - needs storage access anyway
./API2/LXC.pm:2345:             my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/LXC.pm:2368:                 my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in 'move_volume', storage access already expected
./API2/Qemu.pm:155:             my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
^ check_storage_access called from create_vm - creation is already needs
to access the storage anyway and can take different amounts of time
based on storages and is not a hot path anyway
./API2/Qemu.pm:441:             my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
^ disk creation... also already expensive and not a hot path...
./API2/Qemu.pm:1640:        $format = (PVE::Storage::parse_volname($storecfg, $volid))[6];
^ update_vm call...
./API2/Qemu.pm:4143:            my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/Qemu.pm:4166:                my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in move_disk... duh :)
./API2/Qemu.pm-4866-        my (undef, undef, undef, undef, undef, undef, $format) =
                                PVE::Storage::parse_volname($storecfg, $drive->{file});
^ resize_vm - also fine...
./CLI/pct.pm-247-               my (undef, undef, undef, undef, undef, undef, $format) =
                                    PVE::Storage::parse_volname($storage_cfg, $volid);
^ in 'pct fsck', fine

Vs the rest:

./API2/LXC.pm-1966-         my (undef, undef, $owner, undef, undef, undef, $format) =
                                PVE::Storage::parse_volname($storage_cfg, $volid);
^ a check in a resize operation (not necessarily cheap anyway)

./API2/Qemu.pm:164:             (my $vtype, undef, $src_vmid) = PVE::Storage::parse_volname($storecfg, $src_image);
./API2/Qemu.pm:242:    my $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_volid))[2];
^ both are part of the import-disk code - certainly not cheap to begin
with

./API2/VZDump.pm:295:	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
./CLI/pvesm.pm:181:	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
^ in both cases: permission check in extracting a config from a backup,
so only affects path based storages and isn't particularly cheap anyway


./CLI/pvesr.pm:41:    my ($vtype, undef, $ownervm) = PVE::Storage::parse_volname($storecfg, $volid);
^ replication...
./API2/Storage/FileRestore.pm:201:      my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
^ PBS specific...
./CLI/pve7to8.pm:895:	    ($vtype) = eval { PVE::Storage::parse_volname($storage_cfg, $volid); };
^ happens only on major debian releases...

While within the storage implementations a lot of times the need for it
is storage dependent (eg. path based storages need it because the files
are in vmid-named subdirectories), and besides, it's usually within the
context of doing actual storage operations.

Also, the `volume_is_base_and_used()` call Dietmar mentioned only
happens when destroying or migrating VMs and so I'm not all that worried
about storage access (especially when there's a burst-cache anyway) for
that kind of access anyway...