Am 21.02.25 um 09:30 schrieb Daniel Kral:
> 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.k...@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.
Right.
>
> 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).
I don't see a reason to distinguish based on the media=cdrom flag being
set or not. There are two cases to consider:
1. checking access for an existing cloudinit image
This goes to the "else" branch and we should not change that.
2. checking access for allocating a new cloudinit image
The $check_storage_access helper currently returns early in this case. I
think it's fine to have this also take the new disk branch, because
that's what it is. But this will break allocating cloudinit images for
users without Datastore.AllocateSpace on the storage. In the past, we
had the cloudinit UI do a two step, remove volume, allocate new one, but
this was changed a while ago if you remember ;) There could be other API
users that rely on no such permission being required for allocating a
cloudinit drive. If we want to be really careful, we should wait until
PVE 9 with this change and note that the cloudinit_update endpoint
should be used by everybody. Or we could take the stance that no
Datastore.AllocateSpace permission means no Datastore.AllocateSpace
permission even in this edge case and not wait.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
