[pve-devel] [PATCH pve-storage] fix #1611: implement import of base-images for LVM-thin Storage

[pve-devel] [PATCH pve-storage] fix #1611: implement import of base-images for LVM-thin Storage

[Hannes Duerr]

if a base-image is to be migrated to a lvm-thin storage, a new
vm-image is allocated on the target side, then the data is written
and afterwards the image is converted to a base-image


Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---

In the bugtracker wolfgang suggested two different approaches. In my
opinion this approach is the cleaner one, but please let me know what
you think

 src/PVE/Storage/LvmThinPlugin.pm | 65 ++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1d2e37c..4579d47 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -383,6 +383,71 @@ 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);
+
+    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 $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";
+	$name = undef;
+    }
+
+    my ($size) = PVE::Storage::Plugin::read_common_header($fh);
+    $size = int($size/1024);
+
+    # Request new vm-name which is needed for the import
+    if ($isBase) {
+	my $newvmname = $class->find_free_diskname($storeid, $scfg, $vmid);
+	$name = $newvmname;
+	$volname = $newvmname;
+    }
+
+    eval {
+	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $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";
+
+	$class->volume_import_write($fh, $file);
+    };
+    if (my $err = $@) {
+	my $cleanup_worker = eval { $class->free_image($storeid, $scfg, $volname, 0) };
+	warn $@ if $@;
+
+	if ($cleanup_worker) {
+	    my $rpcenv = PVE::RPCEnvironment::get();
+	    my $authuser = $rpcenv->get_user();
+
+	    $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
+	}
+
+	die $err;
+    }
+
+    if ($isBase) {
+	my $newbasename = $class->create_base($storeid, $scfg, $volname);
+	$volname=$newbasename;
+    }
+
+    return "$storeid:$volname";
+}
+
 # used in LVMPlugin->volume_import
 sub volume_import_write {
     my ($class, $input_fh, $output_file) = @_;
-- 
2.39.2

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

[Fiona Ebner]

Am 16.10.23 um 13:59 schrieb Hannes Duerr:
> In the bugtracker wolfgang suggested two different approaches. In my
> opinion this approach is the cleaner one, but please let me know what
> you think
> 

I slightly prefer this approach, because here, the base image conversion
is done after import which sets certain LV flags including the read-only
flag. With the "allow allocation of base image directly" approach, you'd
need to defer setting that flag until after import and then why not just
avoid that and do the full conversion to base there explicitly.

>  src/PVE/Storage/LvmThinPlugin.pm | 65 ++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
> index 1d2e37c..4579d47 100644
> --- a/src/PVE/Storage/LvmThinPlugin.pm
> +++ b/src/PVE/Storage/LvmThinPlugin.pm
> @@ -383,6 +383,71 @@ sub volume_has_feature {
>      return undef;
>  }
>  

This essentially duplicates most of the same function in the parent
plugin, i.e. LVMPlugin. What you can do to avoid it, is introduce new
helper functions for the parts that are different, call those in
LVMPlugin's implementation and overwrite the helpers appropriately in
LvmThinPlugin. Then call the parent plugin's function here and do the
base conversion at the end.

> +sub volume_import {

(...)

> +
> +    # Request new vm-name which is needed for the import
> +    if ($isBase) {
> +	my $newvmname = $class->find_free_diskname($storeid, $scfg, $vmid);
> +	$name = $newvmname;
> +	$volname = $newvmname;
> +    }

So this is one of the parts that needs to be different.

You could also just set the name to undef and alloc_image will call
find_free_diskname itself. Might be slightly nicer and avoids a new
find_free_diskname call. We could also set the name to vm-XYZ-disk-N
instead of base-XYZ-disk-N for the allocation I think. Or the whole
function could even be:

1. check if base
2. check if already exists/rename allowed
3. call parent plugin's volume_import function passing along
vm-XYZ-disk-N (or undef if it should be renamed) instead of base-XYZ-disk-N
4. if it was a base image, handle the conversion to base image

Then there's no need for new helpers either. But this is just a sketch
of course, didn't think through all details. What do you think?

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

[Hannes Dürr]

On 11/3/23 11:39, Fiona Ebner wrote:
[...]
> This essentially duplicates most of the same function in the parent
> plugin, i.e. LVMPlugin. What you can do to avoid it, is introduce new
> helper functions for the parts that are different, call those in
> LVMPlugin's implementation and overwrite the helpers appropriately in
> LvmThinPlugin. Then call the parent plugin's function here and do the
> base conversion at the end.
yes makes sense to work with the LVMPlugin implementation
rather then duplicating all the code, good point.
>> +sub volume_import {
> (...)
>
>> +
>> +    # Request new vm-name which is needed for the import
>> +    if ($isBase) {
>> +	my $newvmname = $class->find_free_diskname($storeid, $scfg, $vmid);
>> +	$name = $newvmname;
>> +	$volname = $newvmname;
>> +    }
> So this is one of the parts that needs to be different.
>
> You could also just set the name to undef and alloc_image will call
> find_free_diskname itself. Might be slightly nicer and avoids a new
> find_free_diskname call. We could also set the name to vm-XYZ-disk-N
> instead of base-XYZ-disk-N for the allocation I think. Or the whole
> function could even be:
>
> 1. check if base
> 2. check if already exists/rename allowed
> 3. call parent plugin's volume_import function passing along
> vm-XYZ-disk-N (or undef if it should be renamed) instead of base-XYZ-disk-N
> 4. if it was a base image, handle the conversion to base image
>
> Then there's no need for new helpers either. But this is just a sketch
> of course, didn't think through all details. What do you think?
that sounds good so far, but you can't get around the 
"find_free_diskspace()" call, can you ?
Not specifying a name leads to an error in the volume_import of the LVM 
plugin,
because without a name no plugin can be parsed and to get a new name in 
the style
of "vm-XYZ-disk-N" you need "find_free_diskspace()" to make sure that 
the name does not yet exist,
or am I missing something ?

So I'd propose:

1. check if base
2. check if already exists/rename allowed
3. call parent plugin's volume_import function passing along
vm-XYZ-disk-N generated with "find_free_diskspace(), instead of 
base-XYZ-disk-N
4. if it was a base image, handle the conversion to base image

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

[Fiona Ebner]

Am 13.11.23 um 14:13 schrieb Hannes Dürr:
> 
> On 11/3/23 11:39, Fiona Ebner wrote:
>> 1. check if base
>> 2. check if already exists/rename allowed
>> 3. call parent plugin's volume_import function passing along
>> vm-XYZ-disk-N (or undef if it should be renamed) instead of
>> base-XYZ-disk-N
>> 4. if it was a base image, handle the conversion to base image
>>
>> Then there's no need for new helpers either. But this is just a sketch
>> of course, didn't think through all details. What do you think?
> that sounds good so far, but you can't get around the
> "find_free_diskspace()" call, can you ?
> Not specifying a name leads to an error in the volume_import of the LVM
> plugin,

Oh, I see.

> because without a name no plugin can be parsed and to get a new name in
> the style
> of "vm-XYZ-disk-N" you need "find_free_diskspace()" to make sure that
> the name does not yet exist,
> or am I missing something ?
> > So I'd propose:
> 
> 1. check if base
> 2. check if already exists/rename allowed
> 3. call parent plugin's volume_import function passing along
> vm-XYZ-disk-N generated with "find_free_diskspace(), instead of
> base-XYZ-disk-N

Yes. And while not super important, we might want to preserve the N in
the suffix (or custom disk names). This can be done by only calling
find_free_diskname() if an image with the name already exists and the
image can be renamed. Otherwise, we can just pass the name with "base-"
replaced by "vm-" instead of invoking find_free_diskname().

> 4. if it was a base image, handle the conversion to base image
>