[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:58 schrieb Dominik Csapak:
> On 4/18/24 11:55, Fiona Ebner wrote:
>>
>>
>> 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?
>>
> 
> sorry i was a step further along in my mind ^^
> 
> the reason i put it here was that we got an absolute path back, which
> would have been complicated when i'd have put it in the branch
> 
> so with my next patch i'll return a volid again and we can safely put
> it there as you suggested
> 

Ah, I see :)

>>>>
>>>>>            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.
> 
> of course we have to clean up for the other case, i just meant
> accidentally left over images can be more easily found and deleted
> 
> sorry for the confusion!
> 

Sorry for not understanding ;) I see what you mean now, thanks for the
explanations!