From: Fiona Ebner <f.ebner@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Dominik Csapak <d.csapak@proxmox.com>,
Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support
Date: Mon, 6 Nov 2023 10:21:44 +0100 [thread overview]
Message-ID: <b23b91b1-0ed9-490c-a4ef-d55562121493@proxmox.com> (raw)
In-Reply-To: <a9be4557-ce92-4bda-a6f0-b9e151ceddde@proxmox.com>
Am 06.11.23 um 09:17 schrieb Dominik Csapak:
> thx for the answer! i'll see that i send a next version soon
>
> some more comment from me inline
>
> On 11/4/23 09:34, Thomas Lamprecht wrote:
>> On 03/11/2023 12:53, Dominik Csapak wrote:
>>> This series aims to provide profile support when creating guests (ct/vm)
>>> so that users can reuse options without having to specify them every
>>> time.
>>>
>>> Sending as RFC because I don't quite like some things with the current
>>> implementation and I'm not quite sure in which direction I should take
>>> this. Also the GUI part isn't done yet and i wanted to see if the
>>> direction is OK. (Sorry for the wall of text)
>>>
>>> The major issues:
>>>
>>> Using a single section config for both VMs and CTs make handling the
>>> properties a bit weird. For now i prefix the options with "$type_"
>>> so vm options are e.g. 'vm_ostype', 'vm_name', and so on while container
>>> options are 'ct_ostype', 'ct_hostname', etc.
>>
>> That properties are shared by different sections by default is IMO not
>> really ideal in general, it's also making the storage API and its docs
>> rather hard to understand & work with.
>>
>> One option could be to opt-into a newer behavior, e.g., via some
>> property, or getter, that the section config implementation needs to
>> set, or override and return true, which makes then all properties
>> isolated if not explicitly marked as shared (via another new property),
>> that way we could use it now, and move over existing ones where it
>> makes sense, without risking wide breakage.
>>
>
> ok, so if i'm reading this right, having a config per type is not really
> an option for you? (that would be nice and easy, without having
> to extend the section config at all, but still get most of the
> upsides)
>
> hard part of extending the section config is how to handle the api
> since when we want to have a 'clean' api per type, we then have
> to also split the api per type? (like e.g. we do with the mapping
> of pci/usb) and then whats the gain of having both types in a single
> config (besides a single file instead of two?)
>
>
>>>
>>> Using the same config/plugin system also makes using it a bit weird.
>>> We have to register/init them in the api where they're used, but for the
>>> cli we have to register only the available type and then init. This
>>> makes it necessary to always set 'allow_unknown' while parsing the
>>> config so that 'qm' doesn't trip over the container profiles...
>>>
>>> A fix for both could be to separate the ct/vm configs into two files
>>> (similar to how we did pci/usb mapping configs), that would fix the
>>> prefixing, as well as the register/init issue (We have to have
>>> separate APIs then ofc).
>>>
>>> Another fix would be to extend the section config to allow different
>>> properties of the same name for different types. Should be possible,
>>> although we have to be careful to not break existing ones, and also
>>> the API interface has to be separate for each type then (cannot really
>>> have confiflicting api parameter schemas for the same name?)
>>>
>>> We could also go in a completely different direction and create a config
>>> per profile? (like we handle vm configs). Downside of that is, that the
>>> current guest config handling part is partly in pmxcfs, so we'd have to
>>> make that either more generic, or duplicate it for profiles.
>>
>> I don't see how this would have anything to do with pmxcfs and VMIDs,
>> that map to an actual guest instance and thus needs special treatment
>> compared to just some profile that can, e.g., live in a
>> /etc/pve/guest-profiles/ as <id>.profile (the extension just an
>> example), and be handled only via perl – profiles are only used in
>> the profile management API, where it's ok to fully parse one, and
>> in guest creation POST calls, so even cfs_register* would be probably
>> overkill.
>
> you're right, i did not explain right what i meant. We currently do
> somethings in pmxcfs for vm configs (e.g. read/write/list) and
> we'd have to use the filesystem layer for the profiles if done in perl
> (which i meant with 'duplicating')
>
>>
>> Having a file per profile makes some things relatively easy, but doesn't
>> fits the commonly used section config, let's see if others have input
>> (i.e., actively ask one/some of fabian/wolfgang/fiona.
>
> i agree, just mentioned it for completeness sake, but yes, lets wait
> for another opinion
>
Yes, then it's just two different section config files with a single
type, which is rather unusual. Compared to that, having a dedicated
config file per profile akin to templates would seem more natural to me.
Don't get me wrong, I have no problem with the section config approach.
Ideally, we would be able to avoid the type prefixing of properties and
still have it all in one config. And having a way to do per-type
declaration of properties should solve that.
Do we have the same issue in our PBS/Rust section configs? Or how is it
solved there? And what is the situation for third-party storage plugins?
Are they already "broken" in that regard, i.e. if a third-party plugin
declares fancy-new-property and we later add fancy-new-property to our
schema with a different format, will it clash?
prev parent reply other threads:[~2023-11-06 9:22 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-03 11:53 Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH cluster 1/1] add profiles.cfg to cluster fs Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin Dominik Csapak
2023-11-06 9:22 ` Fiona Ebner
2023-11-06 9:34 ` Dominik Csapak
2023-11-06 10:12 ` Fiona Ebner
2023-11-06 10:32 ` Fiona Ebner
2023-11-06 10:38 ` Dominik Csapak
2023-11-06 11:38 ` Fiona Ebner
2023-11-03 11:53 ` [pve-devel] [RFC PATCH qemu-server 1/4] meta info: allow additional properties to be given Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH qemu-server 2/4] add the VM profiles plugin Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH qemu-server 3/4] api: add profile option to create vm api call Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH qemu-server 4/4] qm: register and init the profiles plugins Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH container 1/3] add the CT profiles plugin Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH container 2/3] api: add profile option to create ct api call Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH container 3/3] pct: register and init the profiles plugins Dominik Csapak
2023-11-03 11:53 ` [pve-devel] [RFC PATCH manager 1/1] api: add guest profile api endpoint Dominik Csapak
2023-11-04 8:34 ` [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support Thomas Lamprecht
2023-11-06 8:17 ` Dominik Csapak
2023-11-06 8:30 ` Dominik Csapak
2023-11-06 8:53 ` Thomas Lamprecht
2023-11-06 9:48 ` Dominik Csapak
2023-11-06 9:21 ` Fiona Ebner [this message]
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=b23b91b1-0ed9-490c-a4ef-d55562121493@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox