Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images

[Daniel Kral <d.kral@proxmox.com>]

On 2/20/25 15:23, Fiona Ebner wrote:
> Am 11.02.25 um 17:08 schrieb Daniel Kral:
>> Asserts whether the target storage supports storing cloudinit images,
>> i.e. VM images, before creating a cloudinit image on the target storage.
>>
>> Without the check in place, a cloudinit image can be created on the
>> storage, which does not support VM images, but won't be able to start
>> since any attached volume must be stored on a supported storage.
>>
>> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
>> ---
>> changes since v1:
>> - new bug fix! (was indirectly fixed in rfc at commit_cloudinit_image)
>>
>>   PVE/API2/Qemu.pm | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 2a2d971e..9fadf3e5 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -142,12 +142,13 @@ my $check_storage_access = sub {
>>   
>>   	my $volid = $drive->{file};
>>   	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>> +	my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
>>   
>> -	if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
>> +	if (!$volid || $volid eq 'none') {
>>   	    # nothing to check
>>   	} elsif ($isCDROM && ($volid eq 'cdrom')) {
>>   	    $rpcenv->check($authuser, "/", ['Sys.Console']);
>> -	} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
>> +	} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE || $is_cloudinit)) {
> 
> A cloudinit drive should be a CD-ROM. Can we even reach here?

Partly, but you're correct that was bad testing on my part, sorry.

As it turns out, it is reachable for some cloudinit images...But it's 
not obvious why and I'll fix this and make the why clearer in the v3's 
patch message:

`PVE::QemuServer::drive_is_cdrom` checks whether the given drive has the 
property key-value pair "media=cdrom". But we neither add that to the 
drive string in the pve-manager (which only sends, e.g.
	"ide2": "local-lvm:cloudinit,format=qcow2"
for any format) and in the rarer case someone allocates a cloudinit 
image via `qm set`, they are likely to not append "media=cdrom" 
themselves like:
	"qm set -ide2 local-lvm:cloudinit,media=cdrom"

Therefore, the check here doesn't detect a cloudinit image as a cdrom as 
long as the "media=cdrom" is not set. But you're correct, with this 
patch applied the check would just be skipped like before if someone 
explicitly provides this setting.

If it doesn't add too much confusion here and we decide to merge the 
cloudinit && new_disk branch here (see my reply for #5), I suggest to 
make the `drive_is_cdrom` exclude cloudinit images here in v3 (so that 
$isCDROM is only 1 when media=cdrom except for cloudinit drives).


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