Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter

From: Aaron Lauterer <a.lauterer@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server v7 4/5] api: create: add 'import-working-storage' parameter
Date: Tue, 19 Nov 2024 12:36:34 +0100	[thread overview]
Message-ID: <7e8a33f4-ac31-4965-a12b-587a9e09f721@proxmox.com> (raw)
In-Reply-To: <3e792a4f-2fd3-40af-aa28-d1ce372d2937@proxmox.com>

On  2024-11-18  21:22, Thomas Lamprecht wrote:
> I now looked into your diff.
> 
> Am 18.11.24 um 18:24 schrieb Aaron Lauterer:
>> On  2024-11-18  16:29, Dominik Csapak wrote:
>>> +		    if (!$extraction_scfg->{content}->{images} || !$extraction_scfg->{path}) {
>>
>> I think the if condition here is grouped wrong.
>>
>> As it is, once if one of the items (content->images or path) is falsy,
>> we trigger it, but it should not be the case, if the other is actually true.
> 
> how so? It must be both, file based and allow the import content type?
> 
>> Consider the following diff, basically grouping the content->images ||
>> path condition, and only if the result is falsy, we want to raise the error.
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 8db443d..545fe31 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -179,7 +179,7 @@ my $check_storage_access = sub {
>>                           $scfg;
>>                       my $extraction_param = defined($extraction_storage) ? 'import-working-storage' : $ds;
>>
>> -                   if (!$extraction_scfg->{content}->{images} || !$extraction_scfg->{path}) {
>> +                   if (!($extraction_scfg->{content}->{images} || $extraction_scfg->{path})) {
> 
> Besides above, the error would now be wrong too, it states:
> 
> "storage selected for extraction does not support 'images' content type or is not file based"
> 
> (not (support 'images' content type)) or is (not (file based))
> 
> Exactly what Dominik used.

For completeness’ sake: I talked in person with Dominik and yes, my 
understanding was wrong.

AFAIU Dominik will send a few follow-up patches, one of them will change 
the error message to make it clearer that the issue is with the working 
or OVA source storage. As that would probably have helped me yesterday 
to not come to the wrong conclusion.

> 
> The fix here would add the 'images' content-type as supported one for
> CephFS.

This is something I would like to avoid unless we have a way to prevent 
using these images in guest configs. Starting a VM or CT with the disk 
image on a Ceph FS will lead to all kinds of issues. RBD is what should 
be used to store guest disk images in Ceph.

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