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

[Dominik Csapak <d.csapak@proxmox.com>]

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

>>>
>>>>            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!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel