From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 6666C1FF176
for <inbox@lore.proxmox.com>; Fri, 21 Feb 2025 11:58:39 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id C30252B4E9;
Fri, 21 Feb 2025 09:30:32 +0100 (CET)
Message-ID: <b1b86f25-8ecf-4956-9dcc-5b40bbf535a9@proxmox.com>
Date: Fri, 21 Feb 2025 09:30:11 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Daniel Kral <d.kral@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250211160825.254167-1-d.kral@proxmox.com>
<20250211160825.254167-12-d.kral@proxmox.com>
<9d27e6fb-8861-4ad5-b3ac-e594899def19@proxmox.com>
Content-Language: en-US
In-Reply-To: <9d27e6fb-8861-4ad5-b3ac-e594899def19@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.009 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api:
update-vm: assert content type support for cloudinit images
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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