From: Fiona Ebner <f.ebner@proxmox.com>
To: Daniel Kral <d.kral@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images
Date: Fri, 21 Feb 2025 10:42:50 +0100 [thread overview]
Message-ID: <94260963-fae6-45b5-8bc7-0fe02e5d07bd@proxmox.com> (raw)
In-Reply-To: <b1b86f25-8ecf-4956-9dcc-5b40bbf535a9@proxmox.com>
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.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.
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
next prev parent reply other threads:[~2025-02-21 10:08 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-11 16:07 [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 1/5] api: {upload, download}_url: factor out common parameter hash accesses Daniel Kral
2025-02-19 15:39 ` [pve-devel] applied: " Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes Daniel Kral
2025-02-19 14:54 ` Fiona Ebner
2025-02-20 9:14 ` Daniel Kral
2025-02-20 9:36 ` Fiona Ebner
2025-02-20 12:53 ` Daniel Kral
2025-02-20 12:58 ` Fiona Ebner
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 3/5] tree-wide: make use of content type assertion helper Daniel Kral
2025-02-19 15:16 ` Fiona Ebner
2025-02-20 9:31 ` Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 4/5] vdisk_alloc: factor out optional parameters in options hash argument Daniel Kral
2025-02-20 8:49 ` Fiona Ebner
2025-02-20 9:34 ` Daniel Kral
2025-02-11 16:07 ` [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type Daniel Kral
2025-02-20 9:03 ` Fiona Ebner
2025-02-20 10:40 ` Fabian Grünbichler
2025-02-20 10:48 ` Fiona Ebner
2025-02-20 12:33 ` Daniel Kral
2025-02-20 13:09 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 01/15] test: cfg2cmd: expect error for invalid volume's storage " Daniel Kral
2025-02-19 15:45 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 02/15] fix #5284: api: move-disk: assert content type support for target storage Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 03/15] fix #5284: api: clone_vm: " Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 04/15] api: remove unused size variable in check_storage_access Daniel Kral
2025-02-20 14:04 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 05/15] api: remove unusable default storage parameter " Daniel Kral
2025-02-20 14:09 ` Fiona Ebner
2025-02-21 8:27 ` Daniel Kral
2025-02-21 9:15 ` Fiona Ebner
2025-02-20 14:15 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 06/15] fix #5284: api: update-vm: assert content type support for cloudinit images Daniel Kral
2025-02-20 14:23 ` Fiona Ebner
2025-02-21 8:30 ` Daniel Kral
2025-02-21 9:42 ` Fiona Ebner [this message]
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 07/15] fix #5284: cli: importovf: assert content type support for target storage Daniel Kral
2025-02-20 14:29 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 08/15] tree-wide: update vdisk_alloc optional arguments signature Daniel Kral
2025-02-20 14:36 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 09/15] cfg2cmd: improve error message for invalid volume content type Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 10/15] api: {clone, move}_vm: use volume content type assertion helpers Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 11/15] api: {create, update}_vm: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 12/15] api: import{disk, ovf}: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 13/15] api: qmrestore: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 14/15] api: migrate_vm: " Daniel Kral
2025-02-20 14:46 ` Fiona Ebner
2025-02-20 17:50 ` Daniel Kral
2025-02-21 9:45 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH qemu-server v2 15/15] tree-wide: add todos for breaking content type assertions Daniel Kral
2025-02-20 14:47 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 01/11] migration: prepare: factor out common read-only variables Daniel Kral
2025-02-20 9:20 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 02/11] tests: add tests for expected behavior of alloc_disk wrapper Daniel Kral
2025-02-20 10:21 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 03/11] alloc_disk: fail fast if storage does not support content type rootdir Daniel Kral
2025-02-20 12:15 ` Fiona Ebner
2025-02-20 17:52 ` Daniel Kral
2025-04-15 12:27 ` Daniel Kral
2025-04-15 13:31 ` Wolfgang Bumiller
2025-04-15 14:19 ` Fabian Grünbichler
2025-04-16 8:19 ` Wolfgang Bumiller
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc Daniel Kral
2025-02-20 13:11 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments Daniel Kral
2025-02-20 13:22 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 06/11] alloc_disk: update vdisk_alloc optional arguments signature Daniel Kral
2025-02-20 13:24 ` Fiona Ebner
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 07/11] alloc_disk: use volume content type assertion helpers Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 08/11] api: create: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 09/11] migration: prepare: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 10/11] api: update_vm: " Daniel Kral
2025-02-11 16:08 ` [pve-devel] [PATCH container v2 11/11] mount: raw/iso: " Daniel Kral
2025-02-20 13:29 ` Fiona Ebner
2025-02-21 8:38 ` Daniel Kral
2025-02-21 9:50 ` [pve-devel] [PATCH container/qemu-server/storage v2 00/31] consistent assertions for volume's content types Fiona Ebner
2025-02-28 8:46 ` [pve-devel] partially-applied: " Fiona Ebner
2025-04-15 13:58 ` [pve-devel] superseded: " Daniel Kral
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=94260963-fae6-45b5-8bc7-0fe02e5d07bd@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=d.kral@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal