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 9BD8BBA7AA
for <pve-devel@lists.proxmox.com>; Thu, 14 Dec 2023 15:24:13 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 769D617E0D
for <pve-devel@lists.proxmox.com>; Thu, 14 Dec 2023 15:23:43 +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>; Thu, 14 Dec 2023 15:23:42 +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 9DCEF4761D
for <pve-devel@lists.proxmox.com>; Thu, 14 Dec 2023 15:23:42 +0100 (CET)
Message-ID: <bc965cdd-4124-4d11-8d33-e9843cb9613b@proxmox.com>
Date: Thu, 14 Dec 2023 15:23:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Hannes Duerr <h.duerr@proxmox.com>
References: <20231207091203.87763-1-h.duerr@proxmox.com>
<20231207091203.87763-3-h.duerr@proxmox.com>
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20231207091203.87763-3-h.duerr@proxmox.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.076 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 -
Subject: Re: [pve-devel] [PATCH v2 qemu-server pve-storage 2/2] 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 <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: Thu, 14 Dec 2023 14:24:13 -0000
Am 07.12.23 um 10:12 schrieb Hannes Duerr:
> for base images we call the volume_import of the parent plugin and pass
> it as vm-image instead of base-image, then convert it back as base-image
>
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
> src/PVE/Storage/LvmThinPlugin.pm | 60 ++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..6c95919 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,66 @@ sub volume_has_feature {
> return undef;
> }
>
> +sub volume_import {
> + my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
> + die "volume import format $format not available for $class\n"
> + if $format ne 'raw+size';
> + die "cannot import volumes together with their snapshots in $class\n"
> + if $with_snapshots;
> + die "cannot import an incremental stream in $class\n" if defined($base_snapshot);
> +
Nit: Not really hurting, but since we use the parent's import method, I
think we can be fine relying on the checks there. If the parent's method
gains support for more in the future and can drop a check, we'd just
need to remove the check here too.
> + my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> + $class->parse_volname($volname);
> + die "cannot import format $format into a file of format $file_format\n"
> + if $file_format ne 'raw';
> +
> + my $oldbasename;
Nit: This variable is not used in the if branch, so declaration could be
moved into the else branch.
> + if (!$isBase) {
> + ($storeid, $volname) = split (/:/, $class->SUPER::volume_import(
Could also just be
return $class->SUPER...
instead of doing the split and re-assembling for return below. Also
avoids overwriting the passed-in params $storeid and $volname.
> + $scfg,
> + $storeid,
> + $fh,
> + $name,
I'd still pass $volname just to be more future-proof
> + $format,
> + $snapshot,
> + $base_snapshot,
> + $with_snapshots,
> + $allow_rename
> + ));
> + } else {
> + my $vg = $scfg->{vgname};
> + my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
> + if ($lvs->{$vg}->{$volname}) {
> + die "volume $vg/$volname already exists\n" if !$allow_rename;
> + warn "volume $vg/$volname already exists - importing with a different name\n";
> +
> + $volname = $class->find_free_diskname($storeid, $scfg, $vmid);> + } else {
> + $oldbasename = $volname;
> + $volname =~ s/base/vm/;
> + }
Nit: I'd prefer to have new variables for both the temporary volname we
pass to the parent's volume_import and the new volname. Since they serve
different purposes.
> +
> + ($storeid, $volname) = split (/:/, $class->SUPER::volume_import(
Using parse_volid() would be more robust than using an ad-hoc split.
> + $scfg,
> + $storeid,
> + $fh,
> + $volname,
> + $format,
> + $snapshot,
> + $base_snapshot,
> + $with_snapshots,
> + $allow_rename
> + ));
> +
> + $volname = $class->create_base($storeid, $scfg, $volname);
> + if ($oldbasename) {
> + $volname= $oldbasename;
> + }
create_base() will tell you the actual name, you should not override it
with the old one. We expect it to match, but then there's no need for
the assignment. If it doesn't match, you'd be returning something wrong.
Or what am I missing?
> + }
> +
> + return "$storeid:$volname";
> +}
> +
> # used in LVMPlugin->volume_import
> sub volume_import_write {
> my ($class, $input_fh, $output_file) = @_;