public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
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?




      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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal