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

[Fiona Ebner]

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.

>>
>>> +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.