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

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 11/8/21 16:53, Thomas Lamprecht wrote:
> 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..

IIRC (it's been a while...) we discussed this and I think agreed that for now in the context of moving disks to other guests, it is okay to define the target config key, e.g. scsi4, virtio3, mp2 since this way the users knows which one in the VM config it will be. With the plan to make it possible to actually give the disk images more customizable names, but that means touching other areas as well. From what I discussed with Fabian yesterday, that is (live)migration, restores and a few other places.

> 
>>
>> 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.
>>
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>