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 3365A9EBE9 for ; Fri, 3 Nov 2023 11:39:42 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0C12E1B85D for ; Fri, 3 Nov 2023 11:39:42 +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 ; Fri, 3 Nov 2023 11:39:38 +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 B4150441FD for ; Fri, 3 Nov 2023 11:39:37 +0100 (CET) Message-ID: <5f42156e-a80c-411b-9ce9-782fa72ffb61@proxmox.com> Date: Fri, 3 Nov 2023 11:39:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Hannes Duerr References: <20231016115920.95859-1-h.duerr@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20231016115920.95859-1-h.duerr@proxmox.com> 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. [lvmthinplugin.pm] Subject: Re: [pve-devel] [PATCH pve-storage] fix #1611: implement import of base-images for LVM-thin Storage 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: Fri, 03 Nov 2023 10:39:42 -0000 Am 16.10.23 um 13:59 schrieb Hannes Duerr: > In the bugtracker wolfgang suggested two different approaches. In my > opinion this approach is the cleaner one, but please let me know what > you think > I slightly prefer this approach, because here, the base image conversion is done after import which sets certain LV flags including the read-only flag. With the "allow allocation of base image directly" approach, you'd need to defer setting that flag until after import and then why not just avoid that and do the full conversion to base there explicitly. > src/PVE/Storage/LvmThinPlugin.pm | 65 ++++++++++++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm > index 1d2e37c..4579d47 100644 > --- a/src/PVE/Storage/LvmThinPlugin.pm > +++ b/src/PVE/Storage/LvmThinPlugin.pm > @@ -383,6 +383,71 @@ sub volume_has_feature { > return undef; > } > This essentially duplicates most of the same function in the parent plugin, i.e. LVMPlugin. What you can do to avoid it, is introduce new helper functions for the parts that are different, call those in LVMPlugin's implementation and overwrite the helpers appropriately in LvmThinPlugin. Then call the parent plugin's function here and do the base conversion at the end. > +sub volume_import { (...) > + > + # Request new vm-name which is needed for the import > + if ($isBase) { > + my $newvmname = $class->find_free_diskname($storeid, $scfg, $vmid); > + $name = $newvmname; > + $volname = $newvmname; > + } So this is one of the parts that needs to be different. You could also just set the name to undef and alloc_image will call find_free_diskname itself. Might be slightly nicer and avoids a new find_free_diskname call. We could also set the name to vm-XYZ-disk-N instead of base-XYZ-disk-N for the allocation I think. Or the whole function could even be: 1. check if base 2. check if already exists/rename allowed 3. call parent plugin's volume_import function passing along vm-XYZ-disk-N (or undef if it should be renamed) instead of base-XYZ-disk-N 4. if it was a base image, handle the conversion to base image Then there's no need for new helpers either. But this is just a sketch of course, didn't think through all details. What do you think?