From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 1418F1FF15E for ; Fri, 20 Sep 2024 16:26:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 22485375B9; Fri, 20 Sep 2024 16:27:09 +0200 (CEST) Message-ID: <4d572ac6-b6c9-4c26-94d8-2629db66c566@proxmox.com> Date: Fri, 20 Sep 2024 16:26:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Daniel Kral To: Proxmox VE development discussion , Filip Schauer References: <20240918144953.130780-1-f.schauer@proxmox.com> <20240918144953.130780-2-f.schauer@proxmox.com> Content-Language: en-US In-Reply-To: <20240918144953.130780-2-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.002 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm] Subject: Re: [pve-devel] [PATCH v4 storage 1/6] plugin: allow volume import of iso, snippets and vztmpl 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: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 9/18/24 16:49, Filip Schauer wrote: > Signed-off-by: Filip Schauer This would surely benefit from a more detailed description about what was changed, in what way and which API endpoints (i.e. `pvesm import`) it affects, so that other people could find and understand the context of the change more easily later on. Also it would be helpful for others to know that nothing was functionally changed for the `$vtype eq 'images' || $vtype eq 'rootdir'` eval block when they're not using `git diff --ignore-all-space`, as that is not totally clear from the raw git diff. > --- > src/PVE/Storage/Plugin.pm | 67 +++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 24 deletions(-) > > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 8cc693c..57536c6 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -1667,14 +1667,18 @@ sub volume_import { > > # XXX: Should we bother with conversion routines at this level? This won't > # happen without manual CLI usage, so for now we just error out... > - die "cannot import format $format into a file of format $file_format\n" > - if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol'); > + if (($vtype eq 'images' || $vtype eq 'rootdir') && $data_format ne $file_format && > + !($data_format eq 'tar' && $file_format eq 'subvol') > + ) { > + die "cannot import format $format into a file of format $file_format\n"; > + } > > # Check for an existing file first since interrupting alloc_image doesn't > # free it. > my $file = $class->path($scfg, $volname, $storeid); > if (-e $file) { > - die "file '$file' already exists\n" if !$allow_rename; > + die "file '$file' already exists\n" > + if !$allow_rename || ($vtype ne 'images' && $vtype ne 'rootdir'); > warn "file '$file' already exists - importing with a different name\n"; > $name = undef; > } > @@ -1682,29 +1686,44 @@ sub volume_import { > my ($size) = read_common_header($fh); > $size = int($size/1024); > > - eval { > - my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > - my $oldname = $volname; > - $volname = $allocname; > - if (defined($name) && $allocname ne $oldname) { > - die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + if ($vtype eq 'images' || $vtype eq 'rootdir') { > + eval { > + my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > + my $oldname = $volname; > + $volname = $allocname; > + if (defined($name) && $allocname ne $oldname) { > + die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + } > + my $file = $class->path($scfg, $volname, $storeid) > + or die "internal error: failed to get path to newly allocated volume $volname\n"; > + if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > + run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > + input => '<&'.fileno($fh)); > + } elsif ($data_format eq 'tar') { > + run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > + input => '<&'.fileno($fh)); > + } else { > + die "volume import format '$format' not available for $class"; > + } > + }; > + if (my $err = $@) { > + eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) }; > + warn $@ if $@; > + die $err; > } > - my $file = $class->path($scfg, $volname, $storeid) > - or die "internal error: failed to get path to newly allocated volume $volname\n"; > - if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > - run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > - input => '<&'.fileno($fh)); > - } elsif ($data_format eq 'tar') { > - run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > - input => '<&'.fileno($fh)); > - } else { > - die "volume import format '$format' not available for $class"; > + } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl') { > + eval { > + run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh)); > + }; > + if (my $err = $@) { > + if (-e $file) { > + eval { unlink($file) }; > + warn $@ if $@; > + } > + die $err; > } > - }; > - if (my $err = $@) { > - eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) }; > - warn $@ if $@; > - die $err; > + } else { > + die "importing volume of type '$vtype' not implemented\n"; > } > > return "$storeid:$volname"; I tested this API endpoint with importing ISO images, Cloudinit snippets and VM templates from local to CephFS and local to local as a copy. This worked as expected for all of those content types. --- I also tested this with importing raw VM images out of curiosity, even though it was not part of the change, but this failed for me. I haven't looked yet how this is implemented, but this works: ``` pvesm import local:108/vm-108-disk-0.raw raw+size /var/lib/vz/images/107/vm-107-disk-0.raw ``` But something like this will not: ``` pvesm import local-lvm:vm-108-disk-0 raw+size /var/lib/vz/images/107/vm-107-disk-0.raw ``` as it will report that the virtual size is zero: ``` --virtualsize may not be zero. lvcreate 'pve/vm-108-disk-0' error: Run `lvcreate --help' for more information. ``` --- Otherwise, this looks good to me as far as I can tell and the changes that should work, work for me in my tests. Tested-by: Daniel Kral Reviewed-by: Daniel Kral _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel