Re: [pve-devel] [PATCH v4 storage 1/6] plugin: allow volume import of iso, snippets and vztmpl

[Daniel Kral <d.kral@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