Re: [pve-devel] [PATCH v2 qemu-server pve-storage 2/2] fix #1611: implement import of base-images for LVM-thin Storage

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

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) = @_;