From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 34F087DC05 for ; Tue, 9 Nov 2021 15:42:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 316BDF253 for ; Tue, 9 Nov 2021 15:42:07 +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 id 06128F248 for ; Tue, 9 Nov 2021 15:42: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 D52D1427FE for ; Tue, 9 Nov 2021 15:42:05 +0100 (CET) Message-ID: Date: Tue, 9 Nov 2021 15:42:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Content-Language: en-US To: Proxmox VE development discussion , Thomas Lamprecht , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20211102150335.2371545-1-a.lauterer@proxmox.com> <20211102150335.2371545-2-a.lauterer@proxmox.com> <1636384211.m34yji3cck.astroid@nora.none> <96a51277-818a-43d6-0849-a311d4b3c9f3@proxmox.com> From: Aaron Lauterer In-Reply-To: <96a51277-818a-43d6-0849-a311d4b3c9f3@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.640 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -3.06 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record 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 v4 storage 1/9] storage: add new find_free_volname X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Nov 2021 14:42:07 -0000 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 "/" 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 >> >> 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 >