Re: [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 2, 2021 4:03 pm, Aaron Lauterer wrote:
> This new method exposes the functionality to request a new, not yet
> used, volname for a storage.
> 
> The default implementation will return the result from
> 'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
> Otherwise it will only return the result from 'find_free_diskname'.
> 
> If the format suffix is added also depends on the existence of
> $scfg->{path}.
> 
> $scfg->{path} will be present for directory based storage types.
> 
> Should a storage need to return different volname, it needs to override
> the 'find_free_volname' method.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>

as discussed off-list (I am sorry I didn't read through the full 
discussion on-list of all 10 versions of this and the previous series 
;)) - IMHO this is not a backwards compatible addition since it's not 
guarded by any feature flag, so external plugins fall into two 
categories:
- derived from Plugin.pm, so automatically implement it via 
  find_free_diskname -> get_next_vm_diskname, which might or might not 
  be correct for that plugin
- not derived from Plugin.pm, don't implement it, no safeguard to handle 
  that

RIGHT NOW, the only callers are guarded by the rename feature so there 
can't be any problems in practice - but there is no guarantee that this 
will be remembered down the line, and it also breaks the 
modularization/layering of the code to rely on this fact ;)

while we could argue about whether we really want to be that strict, 
another point against exposing this method for me is that it's also 
prone to mis-use, since it encourages code patterns like in this 
series with:

get_free_volname
do_something_that_allocates_that_volname

without holding a storage lock over both calls, since the storage lock 
is only available on the plugin level and not exposed in PVE::Storage 
itself.

an interface like
$plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)

with at least $new_owner or $new_name set would also cover both cases 
(reassign with just $new_owner, rename with just $new_name) and can be 
feature-guarded while keeping the locking in PVE::Storage for the full 
operation including finding a free slot if needed.

alternatively, just implementing

$plugin->reassign_volume($scfg, $storeid, $source, $new_owner)

for now (guarded by a 'reassign' feature), and postponing the rename 
feature for a future series that handles 'custom suffix/volname' 
across the board of our stack would also be an option.

> ---
> 
> v3 -> v4:
> * fix style nit
> 
> v2 -> v3:
> * dropped exists() check
> * fixed base image handling
> * fixed smaller code style issues
> 
> v1 -> v2:
> * many small fixes and improvements
> * rename_volume now accepts $source_volname instead of $source_volid
>     helps us to avoid to parse the volid a second time
> 
> rfc -> v1:
> * reduced number of parameters to minimum needed, plugins infer needed
>   information themselves
> * added storage locking and checking if volume already exists
> * parse target_volname prior to renaming to check if valid
> 
> old dedicated API endpoint -> rfc:
> only do rename now but the rename function handles templates and returns
> the new volid as this can be differently handled on some storages.
>  
>  PVE/Storage.pm        | 9 +++++++++
>  PVE/Storage/Plugin.pm | 8 ++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 71d6ad7..4fbeaea 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -203,6 +203,15 @@ sub storage_can_replicate {
>      return $plugin->storage_can_replicate($scfg, $storeid, $format);
>  }
>  
> +sub find_free_volname {
> +    my ($cfg, $storeid, $vmid, $fmt) = @_;
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    return $plugin->find_free_volname($storeid, $scfg, $vmid, $fmt);
> +}
> +
>  sub storage_ids {
>      my ($cfg) = @_;
>  
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index aeb4fff..d21c874 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -732,6 +732,14 @@ sub find_free_diskname {
>      return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
>  }
>  
> +sub find_free_volname {
> +    my ($class, $storeid, $scfg, $vmid, $fmt) = @_;
> +
> +    my $diskname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, $scfg->{path});
> +
> +    return $scfg->{path} ? "${vmid}/$diskname" : $diskname;
> +}
> +
>  sub clone_image {
>      my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>