[pve-devel] [PATCH storage 3/9] plugin: dir: handle ova files for import

[Fabian Grünbichler]

On April 18, 2024 9:22 am, Fiona Ebner wrote:
> Am 17.04.24 um 15:07 schrieb Dominik Csapak:
>> On 4/17/24 12:52, Fiona Ebner wrote:
>>> Am 16.04.24 um 15:18 schrieb Dominik Csapak:
>>>>
>>>>    we currently extract into the import storage in a directory named:
>>>>    `.tmp_<pid>_<targetvmid>` which should not clash with concurrent
>>>>    operations (though we do extract it multiple times then)
>>>>
>>>
>>> Could we do "extract upon upload", "tar upon download" instead? Sure
>>> some people surely want to drop the ova manually, but we could tell them
>>> they need to extract it first too. Depending on the amount of headache
>>> this would save us, it might be worth it.
>> 
>> we could, but this opens a whole other can of worms, namely
>> what to do with conflicting filenames for different ovas?
>> 
>> we'd then either have to magically match the paths from the ovfs
>> to some subdir that don't overlap
>> 
>> or we'd have to abort everytime we encounter identical disk names
>> 
>> IMHO this would be less practical than just extract on demand...
>> 
> 
> Yes, I was thinking about just having a subdir named based on the ova
> file (e.g. just strip the extension).
> 
>>>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>>>> index f8ea93d..bc073ef 100755
>>>> --- a/src/PVE/Storage.pm
>>>> +++ b/src/PVE/Storage.pm
>>>> @@ -2189,4 +2189,63 @@ sub get_import_metadata {
>>>>       return $plugin->get_import_metadata($scfg, $volname, $storeid);
>>>>   }
>>>>   
>>>
>>> Shouldn't the following three functions call into plugin methods
>>> instead? That'd seem much more future-proof to me.
>> 
>> could be, i just did not want to extend the plugin api for that
>> but as fabian wrote, maybe we should put them in qemu-server
>> altogether for now?
>> 
>> (after thinking about it a bit, i'd be in favor of putting it in
>> qemu-server, because mainly i don't want to add to the plugin api further)
>> 
>> what do you think @fiona @fabian?
>> 
> 
> Doesn't that kinda defeat the purpose to move OVF here? Ideally
> qemu-server just uses the import storage API without any knowledge about
> how the import content is organized by the storage layer. I mean we
> could potentially avoid extending the plugin API by doing the "extract
> upon upload". I'd prefer to extend the plugin API, because other future
> plugins might also want to offer archive-based import, but if we really
> don't want to do it for now, fine by me too.

I am not convinved of that - the import sits in a weird place between
storage and qemu-server, it's basically a layering violation already ;)
and we have lots of other places where qemu-server second-guesses the
storage layer/does custom things..

>>>> +sub copy_needs_extraction {
>>>> +    my ($volid) = @_;
>>>> +    my ($storeid, $volname) = parse_volume_id($volid);
>>>> +    my $cfg = config();
>>>> +    my $scfg = storage_config($cfg, $storeid);
>>>> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>> +
>>>> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase,
>>>> $file_format) =
>>>> +    $plugin->parse_volname($volname);
>>>> +
>>>> +    return $vtype eq 'import' && defined($file_format);
>>>
>>> E.g this seems rather hacky, and puts a weird coupling on a future
>>> import plugin's parse_volname() function (presence of $file_format).
>> 
>> would it be better to check the volid again for '.ova/something$' ?
>> or do you have a better idea?
>> (especially if we want to have this maybe in qemu-server)
>> 
> 
> IMHO, it's the plugin's job to decide this. The plugin should know how
> the import content is organized and nobody else needs to know.

I'd dislike moving it into the plugin API for the same reason I dislike
it being in PVE::Storage. it should live in some import-specific module
(whether that lives in pve-storage or qemu-server).