[pve-devel] [PATCH qemu-server 1/1] fix #5033: api: Add target-filename option to move_disk reassign

[Fiona Ebner]

Am 08.11.23 um 17:52 schrieb Markus Ebner:
> When the move_disk endpoint is used to reassign a disk image from one
> vm to another, the target-filename of the image is typically chosen
> automatically with the known naming schema.
> 
> This patch adds the optional parameter target-filename, allowing
> to manually specify a filename for the disk image when doing a vm
> to vm reassignment. It's not currently implemented for storage to
> storage moving.
> 

I'm not fully convinced we want this at the guest level. For example,
when allocating a volume via the special "qm set 123 --scsi0
storeid:size-in-GiB" syntax, we also don't allow specifying a name.
Exposing the rename functionality as part of the storage API instead
might fit better with the status quo?

But I do have another suggestion too: Should we rather automatically
preserve the current volume name (just replacing the VM ID) if there is
no other volume with that name and choose a new name if there is? For
offline storage migration, we also do it like that (sans replacing the
VM ID). Then we could get away without needing a new option and users
specifying it. Would that be enough to cover your use case?

If we choose a guest-level approach, it should also be implemented for
containers for consistency.


Regarding your approach:

It'd make sense to support using the same VM ID if target-filename is
specified.

root at pve8a1 ~ # qm disk move 120 scsi1 --target-vmid 120
--target-filename foobar

This currently fails with

400 Parameter verification failed.
target-vmid: must be different than source VMID to reassign disk

Also, there is an issue when the file name doesn't match the usual
naming convention:

https://pve.proxmox.com/pve-docs/chapter-pvesm.html#_file_naming_conventions

For example, with

root at pve8a1 ~ # qm disk move 120 scsi1 --target-vmid 125
--target-filename foobar

moving disk 'scsi1' from VM '120' to '125'
removing disk 'scsi1' from VM '120' config
unable to parse volume filename 'foobar'

you end up with a situation where the volume is in neither config and
rescan won't find it, but it still got renamed:

root at pve8a1 ~ # ls /mnt/pve/nfs/images/125
foobar

This issue is pre-existing, because it is the storage plugin's job to
check for that, but your patch exposes it. The rename_volume()
implementations should verify that the target filename is valid (i.e.
can be parsed) before actually doing the rename. Still, it'd be best to
also do an early check in the API function here.

Best Regards,
Fiona