[pve-devel] [PATCH qemu-server 3/3] api: create: implement extracting disks when needed for import-from

[Fiona Ebner]

Am 18.04.24 um 11:48 schrieb Dominik Csapak:
> On 4/18/24 11:41, Fiona Ebner wrote:
>> Am 16.04.24 um 15:19 schrieb Dominik Csapak:
>>> @@ -391,6 +392,13 @@ my sub create_disks : prototype($$$$$$$$$$) {
>>>             $needs_creation = $live_import;
>>>   +        if (PVE::Storage::copy_needs_extraction($source)) { #
>>> needs extraction beforehand
>>> +            print "extracting $source\n";
>>> +            $source =
>>> PVE::Storage::extract_disk_from_import_file($source, $vmid);
>>> +            print "finished extracting to $source\n";
>>> +            push @$delete_sources, $source;
>>> +        }
>>> +
>>
>> This breaks import from an absolute path: copy_needs_extraction()
>> expects to be called with a PVE-managed volid, so the above should be
>> moved into the if below.
> 
> true, that will be fixed in the next iteration since we then get a
> pve managed volid back after extraction
> (see my answer to the cover letter)
> 

Sorry, I don't understand. The breakage is for import from an absolute
path, because copy_needs_extraction() cannot be called on an absolute
path. Why does it matter whether extraction returns a managed volid or not?

>>
>>>           if (PVE::Storage::parse_volume_id($source, 1)) { #
>>> PVE-managed volume
>>>               if ($live_import && $ds ne 'efidisk0') {
>>>               my $path = PVE::Storage::path($storecfg, $source)
>>> @@ -514,13 +522,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
>>>           eval { PVE::Storage::vdisk_free($storecfg, $volid); };
>>>           warn $@ if $@;
>>>       }
>>> +   
>>> PVE::QemuServer::Helpers::cleanup_extracted_images($delete_sources);
>>>       die $err;
>>>       }
>>>         # don't return empty import mappings
>>>       $live_import_mapping = undef if !%$live_import_mapping;
>>>   -    return ($vollist, $res, $live_import_mapping);
>>> +    return ($vollist, $res, $live_import_mapping, $delete_sources);
>>>   };
>>>     my $check_cpu_model_access = sub {
>>
>> The second caller of create_disks(), i.e. when updating an existing VM,
>> is not updated to handle $delete_sources. (You can also do a disk
>> import-from from an OVA for an existing VM).
>>
>> When I tested that my suspicion is correct I didn't notice initially
>> that the temporary dirs were hidden, should we really make them so hard
>> to find?
> 
> see my recent answer to the cover letter, this shouldn't be an issue when
> we put the extracted image into a valid image path on the storage
> 

But we should still attempt cleanup and not just ignore the
$delete_sources for the second caller.