From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <f.ebner@proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 9102EA0EE4 for <pve-devel@lists.proxmox.com>; Fri, 10 Nov 2023 10:45:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6A0F21DD95 for <pve-devel@lists.proxmox.com>; Fri, 10 Nov 2023 10:45:09 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for <pve-devel@lists.proxmox.com>; Fri, 10 Nov 2023 10:45:06 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A147147A81; Fri, 10 Nov 2023 10:45:06 +0100 (CET) Message-ID: <aeacd878-8bba-4b96-a3d3-412498ede6cb@proxmox.com> Date: Fri, 10 Nov 2023 10:45:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Markus Ebner <info@ebner-markus.de>, pve-devel@lists.proxmox.com References: <20231108165239.22145-1-info@ebner-markus.de> <20231108165239.22145-2-info@ebner-markus.de> Content-Language: en-US From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20231108165239.22145-2-info@ebner-markus.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.080 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH qemu-server 1/1] fix #5033: api: Add target-filename option to move_disk reassign X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Fri, 10 Nov 2023 09:45:39 -0000 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