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

Re: [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@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@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@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

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

[Fiona Ebner]

Am 10.11.23 um 11:54 schrieb Markus Ebner:
>> 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?
> 
> Use-Case:
> Our VM disks are tagged with UUIDs so we can reliably match them to the
> hardware configuration in our database and don't delete any disks just
> because they were in the wrong slot. (e.g. because someone manually
> fiddled with the config).
> We also have VM templates prepared with Cloud-Init installations that
> use the default disk naming scheme. Now if we want to reinstall a VM,
> we delete the vm's root disk volume, clone the template and move the clone's
> disk volume to the target VM. At that step, we want to rename from <default
> naming scheme> to <uuid naming scheme> with the correct UUID that's
> already specified for the vm's rootdisk in our database.
> So unfortunately, I don't think that would solve our use-case.
> 

Okay, then it won't work. I thought you might have created the disks
with the custom naming scheme via the storage layer.

>> Exposing the rename functionality as part of the storage API instead
>> might fit better with the status quo?
> 
> Yesterday in: https://bugzilla.proxmox.com/show_bug.cgi?id=5038[1] you
> wrote that the storage layer has no access to the guest layer.
> So if renaming is instead implemented here, renaming an attached disk
> would break the vm similar to how deleting a volume breaks it, right?
> 

Yes, that's true. I was thinking about doing:

1. remove config entry for disk for VM A
2. rename volume on storage layer
3. add config entry for disk for VM B

The issue is that step 1 is currently not possible AFAIK, because
removing the config entry will rather detach the disk (if attached) or
remove the disk in the process. So I think a guest-level approach can be
fine after all.

> Our use-case would then probably have to look something like this:
> - clone cloud-init template
> - unattach rootdisk in clone
> - rename vm<cloneId>-disk-0 to vm-<cloneId>-<targetVM.UUID>
> - request clone's /config/current to search for uuid in unused disks
> - move_disk from clone.unused<N> to targetVM.virtio0
> 
> 
> Yet another suggestion:
> - Do it like you said in move_disk and preserve the appendix; just changing
> the vmid.
> - Add a rename_disk endpoint at the guest level that is aware of the vm config
> and patches it accordingly so nothing breaks.
> 
> What do you think about that?
> 

I'd rather go with your current approach than a new endpoint. Having
said that, another issue is that custom naming is not a first-class
feature currently, e.g. live-migration of a local disk or moving a disk
to a different storage or restoring from a backup will just throw away
the custom name. If we add more explicit support for custom names here,
then it's natural that users expect it to "just work" in all scenarios.

So I feel like it's a question of whether we want to better support
custom names in general.

There also is an open feature request to support comment fields for
disks (and other hardware items):
https://bugzilla.proxmox.com/show_bug.cgi?id=2672 and IMHO that would be
a cleaner solution than encoding custom information in the disk name itself.

Opinions from other devs?

Best Regards,
Fiona

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

[Fabian Grünbichler]

On November 10, 2023 2:07 pm, Fiona Ebner wrote:
> I'd rather go with your current approach than a new endpoint. Having
> said that, another issue is that custom naming is not a first-class
> feature currently, e.g. live-migration of a local disk or moving a disk
> to a different storage or restoring from a backup will just throw away
> the custom name. If we add more explicit support for custom names here,
> then it's natural that users expect it to "just work" in all scenarios.
> 
> So I feel like it's a question of whether we want to better support
> custom names in general.
> 
> There also is an open feature request to support comment fields for
> disks (and other hardware items):
> https://bugzilla.proxmox.com/show_bug.cgi?id=2672 and IMHO that would be
> a cleaner solution than encoding custom information in the disk name itself.
> 
> Opinions from other devs?

there have been similar requests in the past, my preference would be:
- expose "human readable" custom name more prominently in places where new
  volumes are created, not just on the CLI/API only storage layer
- keep the name on operations that copy/move a volume

if a conflict arises for the second part, we could simply bump the
number-suffix until we find a free "slot", just like we do now for
vm-XXX-disk-N. this would require a coordinated change through the
storage plugins, although it would be possible to fall back to the old
behaviour ("custom name" is lost) in PVE::Storage for (external) plugins
that don't support this feature/API (yet).

for doing a strict match/integration into external tooling, some sort of
comment or attribute would be better, as that could be easily preserved
on "move" type operations, and the external tooling would need to ensure
it is set for "create" type operations, and updated for "copy" type
operations if that is required (e.g., if it is supposed to be unique).

one could argue that a comment is all that is needed for human readable
labelling as well, but there are lots of people that *really* want

vm-XXX-system-0.raw and vm-XXX-data-0.raw (or 'db', or ..)

encoded directly in the volume (and thus file/..) name, since that is
then also visible on the storage layer (in PVE, but also directly when
doing disaster recovery/..), and I kind of see value in that as well.

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

[Thomas Lamprecht]

Am 13/11/2023 um 10:38 schrieb Fabian Grünbichler:
> On November 10, 2023 2:07 pm, Fiona Ebner wrote:
>> I'd rather go with your current approach than a new endpoint. Having
>> said that, another issue is that custom naming is not a first-class
>> feature currently, e.g. live-migration of a local disk or moving a disk
>> to a different storage or restoring from a backup will just throw away
>> the custom name. If we add more explicit support for custom names here,
>> then it's natural that users expect it to "just work" in all scenarios.
>>
>> So I feel like it's a question of whether we want to better support
>> custom names in general.
>>
>> There also is an open feature request to support comment fields for
>> disks (and other hardware items):
>> https://bugzilla.proxmox.com/show_bug.cgi?id=2672 and IMHO that would be
>> a cleaner solution than encoding custom information in the disk name itself.
>>
>> Opinions from other devs?
> 
> there have been similar requests in the past, my preference would be:
> - expose "human readable" custom name more prominently in places where new
>   volumes are created, not just on the CLI/API only storage layer
> - keep the name on operations that copy/move a volume
> 
> if a conflict arises for the second part, we could simply bump the
> number-suffix until we find a free "slot", just like we do now for
> vm-XXX-disk-N. this would require a coordinated change through the
> storage plugins, although it would be possible to fall back to the old
> behaviour ("custom name" is lost) in PVE::Storage for (external) plugins
> that don't support this feature/API (yet).
> 
> for doing a strict match/integration into external tooling, some sort of
> comment or attribute would be better, as that could be easily preserved
> on "move" type operations, and the external tooling would need to ensure
> it is set for "create" type operations, and updated for "copy" type
> operations if that is required (e.g., if it is supposed to be unique).
> 
> one could argue that a comment is all that is needed for human readable
> labelling as well, but there are lots of people that *really* want
> 
> vm-XXX-system-0.raw and vm-XXX-data-0.raw (or 'db', or ..)
> 
> encoded directly in the volume (and thus file/..) name, since that is
> then also visible on the storage layer (in PVE, but also directly when
> doing disaster recovery/..), and I kind of see value in that as well.

I.e., two things

- a comment that is displayed only and has no duplication checks and a
  relatively flexible format (maxLength 64 and /^[\S ]+/) saved as, e.g.,
  URL-encoded. This is realtively easy to do, and someone already tried
  but went IMO way overboard and added comment fields for everything,
  which just does not makes sense for most options but disks and nics.

- a alias, that is checked unique per VMID (via config) and is actually
  inside the volume name. This is a bit harder to get nicely right,
  especially if one wants to do queries on that information.
  Storage move's would need some handling for (non-recommended) sharing
  of storage between clusters, or just left-over volumes. Similar for
  disk-movements to other guests, but as the counter would stay it wouldn't
  be that hard.
  My question here is is duch a thing is really worth it, as having that
  encoded is one thing, but the user actually needs to have the foresight
  to set the correct alias so that they have some value from it listing
  volumes at the storage-layer in the disaster case. And having the config
  around, they could to that mapping too if they used comments.

So, I'd not go for the alias/description/... in volume name for now,
rather do the per-disk comment first, which is way simpler to maintain
forever and provides not that less benefits, while being quite a bit
less work.