From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 1418F1FF15E
for <inbox@lore.proxmox.com>; 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 <d.kral@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Filip Schauer <f.schauer@proxmox.com>
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 <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>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 9/18/24 16:49, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
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 <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel