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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Extend volume import functionality to support 'iso', 'snippets',
> 'vztmpl', and 'import' types, in addition to the existing support for
> 'images' and 'rootdir'. This is a prerequisite for the ability to move
> ISOs, snippets and container templates between nodes.
> 
> Existing behavior for importing VM disks and container volumes remains
> unchanged.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Storage/Plugin.pm | 72 ++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 65cf43f..b682362 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1738,6 +1738,8 @@ sub volume_export_formats {
>  	my $format = ($class->parse_volname($volname))[6];
>  	my $size = file_size_info($file, undef, $format);
>  
> +	return ('raw+size') if !defined($format);

Can we rather make this explicitly check for vtype being one of the ones
we care about? That reflects more precisely what we want to test for.
I'm also thinking whether we want to explicitly include the vtype in the
new export format name to avoid potential type confusion down the line.
I.e. if a storage wants to export an iso, it should not be importable as
a snippet on the other side. We can keep the current formats for
backwards-compat, but the formats for the newly supported vtypes could
then be $vtype+meta, similar to the backup+size type you add. This would
be more explicit and less likely to be(come) exploitable. And we could
write the vtype into the header of the stream itself for extra safety.
Maybe in a way that allows for some forwards-compatibility, e.g. what if
ISO files get notes too at some point.

Just a crude outline what the stream could be from the top of my head:
number of meta-properties
for each meta-property: length of property name, property name, length
of property data, property data
length of volume, volume data

For compatibility/extensibility, the stream consumer can then handle all
meta properties it knows and skip the rest (with some informational
logging about what was skipped). Or we can add a marker to each property
whether it is required or optional and mark all important properties
that should rather lead to failure than skipping. Then outdated nodes
might not be able to receive certain streams, but we have the
flexibility to decide for each property.

I'm sorry, it would expand the scope of the series a bit more, but I do
think this will save us some headaches and give us more flexibility in
the future. And it seems to be a natural generalization of what you
already do for backup+size.

@other devs: opinions?

> +
>  	if ($with_snapshots) {
>  	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
>  	    return ();
> @@ -1766,14 +1768,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')

Style nit: multiline ifs are written differently:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If

> +    ) {
> +	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;
>      }
> @@ -1781,29 +1787,44 @@ sub volume_import {
>      my ($size) = read_common_header($fh);
>      $size = PVE::Storage::Common::align_size_up($size, 1024) / 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 (grep { $vtype eq $_ } qw(import iso snippets vztmpl)) {
> +	eval {
> +	    run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));

Please use 'conv=excl' to avoid the race of somebody/something else
creating the file between the existence check above and here.

> +	};
> +	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";
> @@ -1813,6 +1834,9 @@ sub volume_import_formats {
>      my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
>      if ($scfg->{path} && !defined($base_snapshot)) {
>  	my $format = ($class->parse_volname($volname))[6];
> +
> +	return ('raw+size') if !defined($format);

Similar here, I'd like to be more precise and also encode the vtype in
the transport format to avoid shenanigans with potential for importing
with a different vtype.

> +
>  	if ($with_snapshots) {
>  	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
>  	    return ();



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel