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

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

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?