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

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

On 08.11.21 16:21, Fabian Grünbichler wrote:
> 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.

for the record, that's not a problem in the sense of consistency or the like,
the alloc is the important thing and that is locked, it can be a nuisance as
one can gets their rename rejected, but that's not uncommon in our API in
general.

IIRC my idea was that the user (has to) get a free name proposed (i.e., the
client can request a free name for $owner and submits that, so that they are
in the loop about the new name already, if the API call returns OK then all
worked out, else, well not -> check error.

That way we avoid that the user has no idea about what the new volid will be
and they won't have to parse some (task) logs or guess around..

> 
> 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.
>