From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pve-devel] [PATCH guest-common v2 1/1] add profiles section config plugin
Date: Tue, 14 Nov 2023 13:41:06 +0100 [thread overview]
Message-ID: <d6ce42b7-c823-4bb2-af42-008f9e9334f0@proxmox.com> (raw)
In-Reply-To: <20231114103545.2878054-3-d.csapak@proxmox.com>
Am 14/11/2023 um 11:35 schrieb Dominik Csapak:
> this is intended to house custom profiles which can be used
> on guest creation instead of manually needing to specify every option.
>
> we do special things here:
> * we always set 'allow_unknown' to 1, because when using the guest
> specific parts in the cli, we cannot depend on the other one, else
> we'd get a cyclic dependency, and without that we need to ignore
> unknown sections
>
> * we pack the type and id in the global options, so that when using
> it with a seperated section config they get included in the
> create/updateSchema
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> src/Makefile | 2 ++
> src/PVE/Profiles/Plugin.pm | 74 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
> create mode 100644 src/PVE/Profiles/Plugin.pm
>
> diff --git a/src/Makefile b/src/Makefile
> index cbc40c1..d99645c 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -17,6 +17,8 @@ install: PVE
> install -d ${PERL5DIR}/PVE/Mapping
> install -m 0644 PVE/Mapping/PCI.pm ${PERL5DIR}/PVE/Mapping/
> install -m 0644 PVE/Mapping/USB.pm ${PERL5DIR}/PVE/Mapping/
> + install -d ${PERL5DIR}/PVE/Profiles
> + install -m 0644 PVE/Profiles/Plugin.pm ${PERL5DIR}/PVE/Profiles/
> install -d ${PERL5DIR}/PVE/VZDump
> install -m 0644 PVE/VZDump/Plugin.pm ${PERL5DIR}/PVE/VZDump/
> install -m 0644 PVE/VZDump/Common.pm ${PERL5DIR}/PVE/VZDump/
> diff --git a/src/PVE/Profiles/Plugin.pm b/src/PVE/Profiles/Plugin.pm
> new file mode 100644
> index 0000000..6ea2b5f
> --- /dev/null
> +++ b/src/PVE/Profiles/Plugin.pm
> @@ -0,0 +1,74 @@
> +package PVE::Profiles::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Cluster qw(cfs_register_file);
> +use PVE::SectionConfig;
> +
> +use base qw(PVE::SectionConfig);
> +
> +my $CFG_PATH = 'virtual-guest/profiles.cfg';
> +
> +cfs_register_file ($CFG_PATH,
extra space before opening parenthesis.
> + sub { __PACKAGE__->parse_config(@_); },
> + sub { __PACKAGE__->write_config(@_); });
let's use some more modern indentation here
cfs_register_file(
$CFG_PATH,
sub { __PACKAGE__->parse_config(@_); },
sub { __PACKAGE__->write_config(@_); },
);
> +
> +my $defaultData = {
> + propertyList => {
> + type => { description => 'Profile type.' },
> + id => {
> + type => 'string',
> + description => "The ID of the profile.",
> + format => 'pve-configid',
> + },
> + 'profile-description' => {
> + description => "Description.",
I mean, yes, but a sentence is still nice:
description => "Use this to add a short comment about a profile.",
> + type => 'string',
> + optional => 1,
> + maxLength => 4096,
would start out with a bit less, as this is rendered in grids and the like
and doesn't needs to hold that many information than e.g., node or guest
instances might have.
> + },
> + },
> + options => {
> + type => {},
> + id => {},
> + 'profile-description' => { optional => 1 },
> + },
> +};
> +
> +sub private {
> + return $defaultData;
> +}
> +
> +sub parse_config {
> + my ($class, $filename, $raw, $allow_unknown) = @_;
> +
> + # always allow unknown, so that qemu-server/pct-container
to make it slightly easier to understand it'd IMO help to:
s/unknown/unknown section types/
> + # can parse the file without loading the other plugin type
> + return $class->SUPER::parse_config($filename, $raw, 1);
> +}
> +
> +sub write_config {
> + my ($class, $filename, $cfg, $allow_unknown) = @_;
> +
> + return $class->SUPER::write_config($filename, $cfg, 1);
> +}
> +
> +# gets, checks and prepares the guest config
> +# throws an error if it does not exist or the type is wrong
> +sub get_guest_ready_config {
This is rather covoluted name for what happens, wouldn't naming this
`load_profile` be clear enough? Or are there future changes, or overrides
in child-implementations that warrant this name (even then we probably
find something better)
> + my ($id, $type) = @_;
Otherwise I'd use at least the following variable names to clarify what
$id's we're talking about, because from the method name I initially thought
those are vmid's (was cleared up quickly when continue reading the code, but
IMO still not ideal as is)
my ($profile_id, $profile_type) = @_;
> +
> + my $cfg = PVE::Cluster::cfs_read_file($CFG_PATH);
> +
> + my $profile = $cfg->{ids}->{$id};
> + die "no such profile '$id'\n" if !defined $profile;
> + die "wrong type '$profile->{type}'\n" if $profile->{type} ne $type;
> +
> + delete $profile->{type};
> + delete $profile->{'profile-description'};
> +
> + return $profile;
> +}
> +
> +1;
next prev parent reply other threads:[~2023-11-14 12:41 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-14 10:35 [pve-devel] [PATCH cluster/guest-common/qemu-server/container/manager v2] add backend profile support Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH cluster v2 1/1] add profiles.cfg to cluster fs Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH guest-common v2 1/1] add profiles section config plugin Dominik Csapak
2023-11-14 12:41 ` Thomas Lamprecht [this message]
2023-11-14 10:35 ` [pve-devel] [PATCH qemu-server v2 1/3] add the VM profiles plugin Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH qemu-server v2 2/3] api: add profile option to create vm api call Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH qemu-server v2 3/3] qm: register and init the profiles plugins Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH container v2 1/3] add the CT profiles plugin Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH container v2 2/3] api: add profile option to create ct api call Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH container v2 3/3] pct: register and init the profiles plugins Dominik Csapak
2023-11-14 10:35 ` [pve-devel] [PATCH manager v2 1/1] api: add guest profile api endpoint Dominik Csapak
2023-11-14 13:15 ` [pve-devel] [PATCH cluster/guest-common/qemu-server/container/manager v2] add backend profile support Thomas Lamprecht
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=d6ce42b7-c823-4bb2-af42-008f9e9334f0@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=d.csapak@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox