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