[pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Dominik Csapak]

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.

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 quite like this one, since i think a single config for all
profiles should be enough?  We could still do that later if we want
and the current way is impractical)

The minor issues:

* Is the priv path ok? /mapping/profiles/<id> feels a bit weird
* We should probably introduce a 'meta' property for ct like we have for
  vms? we could record the profile there and also e.g. the template used
  to set the container up.
* I did not restrict to any options of the config and i don't believe
  this should make any issues (the critical ones are filtered out anyway)
  but should we maybe have some kind of whitelist? if yes, which one
  should be on there?
* there is still an issue with the docs generation, i'll have to look
  into it
* I'm not quite sure how the UI for creating/editing profiles should
  look like. For creating i could imagine a 'create profile from guest'
  type of thing (thanks @mira for the idea), but for editing or creating
  from scratch I'm a bit conflicted. We could simply show the profiles
  in the tree, and reuse the vm hw/options panels for that, but does
  that seem overkill? We could also leave that API only for now, and
  make the gui later? (Using it in the wizard would also not be trivial,
  but there the constraints are rather straight forward)

pve-cluster:

Dominik Csapak (1):
  add profiles.cfg to cluster fs

 src/PVE/Cluster.pm  | 1 +
 src/pmxcfs/status.c | 1 +
 2 files changed, 2 insertions(+)

pve-guest-common:

Dominik Csapak (1):
  add profiles section config plugin

 src/Makefile               |  2 ++
 src/PVE/Profiles/Plugin.pm | 72 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 src/PVE/Profiles/Plugin.pm

qemu-server:

Dominik Csapak (4):
  meta info: allow additional properties to be given
  add the VM profiles plugin
  api: add profile option to create vm api call
  qm: register and init the profiles plugins

 PVE/API2/Qemu.pm      | 27 ++++++++++++++++++++++++++-
 PVE/CLI/qm.pm         |  6 ++++++
 PVE/Makefile          |  1 +
 PVE/Profiles/Makefile |  5 +++++
 PVE/Profiles/VM.pm    | 37 +++++++++++++++++++++++++++++++++++++
 PVE/QemuServer.pm     | 29 +++++++++++++++++++++--------
 6 files changed, 96 insertions(+), 9 deletions(-)
 create mode 100644 PVE/Profiles/Makefile
 create mode 100644 PVE/Profiles/VM.pm

pve-container:

Dominik Csapak (3):
  add the CT profiles plugin
  api: add profile option to create ct api call
  pct: register and init the profiles plugins

 src/PVE/API2/LXC.pm       | 24 ++++++++++++++++++++++++
 src/PVE/CLI/pct.pm        |  6 ++++++
 src/PVE/Makefile          |  1 +
 src/PVE/Profiles/CT.pm    | 37 +++++++++++++++++++++++++++++++++++++
 src/PVE/Profiles/Makefile |  4 ++++
 5 files changed, 72 insertions(+)
 create mode 100644 src/PVE/Profiles/CT.pm
 create mode 100644 src/PVE/Profiles/Makefile

pve-manager:

Dominik Csapak (1):
  api: add guest profile api endpoint

 PVE/API2/Cluster.pm          |   6 +
 PVE/API2/Cluster/Makefile    |   1 +
 PVE/API2/Cluster/Profiles.pm | 230 +++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 PVE/API2/Cluster/Profiles.pm

-- 
2.30.2

[pve-devel] [RFC PATCH cluster 1/1] add profiles.cfg to cluster fs

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Cluster.pm  | 1 +
 src/pmxcfs/status.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index cfa2583..c01bf89 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -80,6 +80,7 @@ my $observed = {
     'sdn/dns.cfg' => 1,
     'sdn/.running-config' => 1,
     'virtual-guest/cpu-models.conf' => 1,
+    'virtual-guest/profiles.cfg' => 1,
     'mapping/pci.cfg' => 1,
     'mapping/usb.cfg' => 1,
 };
diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index c8094ac..bcec079 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -109,6 +109,7 @@ static memdb_change_t memdb_change_array[] = {
 	{ .path = "sdn/dns.cfg" },
 	{ .path = "sdn/.running-config" },
 	{ .path = "virtual-guest/cpu-models.conf" },
+	{ .path = "virtual-guest/profiles.cfg" },
 	{ .path = "firewall/cluster.fw" },
 	{ .path = "mapping/pci.cfg" },
 	{ .path = "mapping/usb.cfg" },
-- 
2.30.2

[pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[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'll prefix the options with the type, so that there is not a
  conflict, (e.g. net0 on ct vs net0 on vm has a different format)
  but we resolve that transparently on parse and write, so that
  the consumer doesn't have to do it themselves

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/Makefile               |  2 ++
 src/PVE/Profiles/Plugin.pm | 72 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 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..8ec5054
--- /dev/null
+++ b/src/PVE/Profiles/Plugin.pm
@@ -0,0 +1,72 @@
+package PVE::Profiles::Plugin;
+
+use strict;
+use warnings;
+
+use PVE::Cluster qw(cfs_register_file);
+use PVE::SectionConfig;
+
+use base qw(PVE::SectionConfig);
+
+cfs_register_file ('virtual-guest/profiles.cfg',
+		   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',
+	},
+    },
+};
+
+sub private {
+    return $defaultData;
+}
+
+
+# on parse/write we map the configs from {type}_{prop} to {prop}
+# so that the consumers can simply use it but we can
+# have e.g. a ct_net0 and vm_net0
+sub parse_config {
+    my ($class, $filename, $raw, $allow_unknown) = @_;
+
+    # always allow unknown, so that qemu-server/pct-container
+    # can parse the file without loading the other plugin type
+    my $cfg = $class->SUPER::parse_config($filename, $raw, 1);
+    for my $id (keys $cfg->{ids}->%*) {
+	my $entry = $cfg->{ids}->{$id};
+	my $type = $entry->{type};
+	for my $opt (keys $entry->%*) {
+	    next if $opt eq 'type';
+	    if ($opt =~ m/^${type}_(.*)$/) {
+		my $newopt = $1;
+		$entry->{$newopt} = delete $entry->{$opt};
+	    }
+	}
+    }
+
+    return $cfg;
+}
+
+sub write_config {
+    my ($class, $filename, $cfg, $allow_unknown) = @_;
+
+    for my $id (keys $cfg->{ids}->%*) {
+	my $entry = $cfg->{ids}->{$id};
+	my $type = $entry->{type};
+	for my $opt (keys $entry->%*) {
+	    next if $opt eq 'type';
+	    next if $opt =~ m/^${type}_/; # skip already prefixed options
+	    my $newopt = "${type}_${opt}";
+	    $entry->{$newopt} = delete $entry->{$opt};
+	}
+    }
+
+    return $class->SUPER::write_config($filename, $cfg, $allow_unknown);
+}
+
+1;
-- 
2.30.2

[pve-devel] [RFC PATCH qemu-server 1/4] meta info: allow additional properties to be given

[Dominik Csapak]

we'll need this since we want to record the profile used for creation,
which we get from outside

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/QemuServer.pm | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0568b2bc..c22b1544 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2125,15 +2125,22 @@ sub parse_meta_info {
 }
 
 sub new_meta_info_string {
-    my () = @_; # for now do not allow to override any value
+    my ($additional_props) = @_;
 
-    return PVE::JSONSchema::print_property_string(
-	{
-	    'creation-qemu' => kvm_user_version(),
-	    ctime => "". int(time()),
-	},
-	$meta_info_fmt
-    );
+    $additional_props //= {};
+
+    # for now do not allow to override any value
+    delete $additional_props->{'creation-qemu'};
+    delete $additional_props->{'ctime'};
+
+    my $opts = {
+	'creation-qemu' => kvm_user_version(),
+	ctime => "". int(time()),
+    };
+
+    $opts->{$_} = $additional_props->{$_} for keys $additional_props->%*;
+
+    return PVE::JSONSchema::print_property_string($opts, $meta_info_fmt);
 }
 
 sub qemu_created_version_fixups {
-- 
2.30.2

[pve-devel] [RFC PATCH qemu-server 2/4] add the VM profiles plugin

[Dominik Csapak]

simply uses the json_config_properties for the vm config and maps them
to "vm_${opt}"

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/Makefile          |  1 +
 PVE/Profiles/Makefile |  5 +++++
 PVE/Profiles/VM.pm    | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 PVE/Profiles/Makefile
 create mode 100644 PVE/Profiles/VM.pm

diff --git a/PVE/Makefile b/PVE/Makefile
index dc173681..d09ca98d 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -12,3 +12,4 @@ install:
 	$(MAKE) -C API2 install
 	$(MAKE) -C CLI install
 	$(MAKE) -C QemuServer install
+	$(MAKE) -C Profiles install
diff --git a/PVE/Profiles/Makefile b/PVE/Profiles/Makefile
new file mode 100644
index 00000000..e5f56833
--- /dev/null
+++ b/PVE/Profiles/Makefile
@@ -0,0 +1,5 @@
+SOURCES=VM.pm
+
+.PHONY: install
+install: ${SOURCES}
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Profiles/$$i; done
diff --git a/PVE/Profiles/VM.pm b/PVE/Profiles/VM.pm
new file mode 100644
index 00000000..c7ff241a
--- /dev/null
+++ b/PVE/Profiles/VM.pm
@@ -0,0 +1,37 @@
+package PVE::Profiles::VM;
+
+use strict;
+use warnings;
+
+use PVE::Profiles::Plugin;
+use PVE::QemuServer;
+
+use base qw(PVE::Profiles::Plugin);
+
+my $type = "vm";
+
+sub type {
+    return $type;
+}
+
+sub properties {
+    my $props = PVE::QemuServer::json_config_properties();
+    for my $opt (keys $props->%*) {
+	my $newopt = "${type}_${opt}";
+	$props->{$newopt} = delete $props->{$opt};
+    }
+
+    return $props;
+}
+
+sub options {
+    my $props = PVE::QemuServer::json_config_properties();
+    my $opts = {};
+    for my $opt (keys $props->%*) {
+	my $newopt = "${type}_${opt}";
+	$opts->{$newopt} = { optional => 1 };
+    }
+    return $opts;
+}
+
+1;
-- 
2.30.2

[pve-devel] [RFC PATCH qemu-server 3/4] api: add profile option to create vm api call

[Dominik Csapak]

we use the the profile cfg as the 'param' hash, but overwrite the values
with the ones from the api call, so one can overwrite options from the
profile easily

also we add the used profile to the meta info in the config, since
it might be interesting which one was used

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Qemu.pm  | 27 ++++++++++++++++++++++++++-
 PVE/QemuServer.pm |  6 ++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 38bdaabd..ca18c796 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -49,6 +49,9 @@ use PVE::SSHInfo;
 use PVE::Replication;
 use PVE::StorageTunnel;
 
+use PVE::Profiles::Plugin;
+use PVE::Profiles::VM;
+
 BEGIN {
     if (!$ENV{PVE_GENERATING_DOCS}) {
 	require PVE::HA::Env::PVE2;
@@ -837,6 +840,11 @@ __PACKAGE__->register_method({
 		    default => 0,
 		    description => "Start VM after it was created successfully.",
 		},
+		profile => {
+		    optional => 1,
+		    type => 'string',
+		    description => "The profile to use as base config.",
+		},
 	    },
 	    1, # with_disk_alloc
 	),
@@ -850,6 +858,23 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $authuser = $rpcenv->get_user();
 
+	my $profile = extract_param($param, 'profile');
+	my $meta_opts = {};
+	if (defined($profile)) {
+	    $rpcenv->check_full($authuser, "/mapping/profiles/${profile}", ['Mapping.Use']);
+	    $meta_opts->{profile} = $profile;
+	    my $profile_cfgs = cfs_read_file('virtual-guest/profiles.cfg');
+	    my $profile_cfg = $profile_cfgs->{ids}->{$profile};
+	    die "no such profile '$profile'\n" if !defined($profile_cfg);
+	    die "wrong type '$profile_cfg->{type}'\n" if $profile_cfg->{type} ne 'vm';
+
+	    for my $opt (keys $param->%*) {
+		$profile_cfg->{$opt} = $param->{$opt};
+	    }
+	    delete $profile_cfg->{type};
+	    $param = $profile_cfg;
+	}
+
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
@@ -1013,7 +1038,7 @@ __PACKAGE__->register_method({
 		my $conf = $param;
 		my $arch = PVE::QemuServer::get_vm_arch($conf);
 
-		$conf->{meta} = PVE::QemuServer::new_meta_info_string();
+		$conf->{meta} = PVE::QemuServer::new_meta_info_string($meta_opts);
 
 		my $vollist = [];
 		eval {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index c22b1544..490bad9d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -290,6 +290,12 @@ my $meta_info_fmt = {
 	pattern => '\d+(\.\d+)+',
 	optional => 1,
     },
+    'profile' => {
+	type => 'string',
+	description => 'The Profile used during creation.',
+	format => 'pve-configid',
+	optional => 1,
+    },
 };
 
 my $confdesc = {
-- 
2.30.2

[pve-devel] [RFC PATCH qemu-server 4/4] qm: register and init the profiles plugins

[Dominik Csapak]

we have to that here, so the properties/options are correctly configured
when using that feature on the cli

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/CLI/qm.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index b17b4fe2..82240ba3 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -37,6 +37,12 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::OVF;
 use PVE::QemuServer;
 
+use PVE::Profiles::Plugin;
+use PVE::Profiles::VM;
+
+PVE::Profiles::VM->register();
+PVE::Profiles::Plugin->init();
+
 use PVE::CLIHandler;
 use base qw(PVE::CLIHandler);
 
-- 
2.30.2

[pve-devel] [RFC PATCH container 1/3] add the CT profiles plugin

[Dominik Csapak]

simply uses the json_config_properties for the ct config and maps them
to "ct_${opt}"

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/Makefile          |  1 +
 src/PVE/Profiles/CT.pm    | 37 +++++++++++++++++++++++++++++++++++++
 src/PVE/Profiles/Makefile |  4 ++++
 3 files changed, 42 insertions(+)
 create mode 100644 src/PVE/Profiles/CT.pm
 create mode 100644 src/PVE/Profiles/Makefile

diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index 40742e4..e9ad9a3 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -8,5 +8,6 @@ install: ${SOURCES}
 	make -C LXC install
 	make -C VZDump install
 	make -C CLI install
+	make -C Profiles install
 
 
diff --git a/src/PVE/Profiles/CT.pm b/src/PVE/Profiles/CT.pm
new file mode 100644
index 0000000..db2360e
--- /dev/null
+++ b/src/PVE/Profiles/CT.pm
@@ -0,0 +1,37 @@
+package PVE::Profiles::CT;
+
+use strict;
+use warnings;
+
+use PVE::Profiles::Plugin;
+use PVE::LXC::Config;
+
+use base qw(PVE::Profiles::Plugin);
+
+my $type = "ct";
+
+sub type {
+    return $type;
+}
+
+sub properties {
+    my $props = PVE::LXC::Config::json_config_properties();
+    for my $opt (keys $props->%*) {
+	my $newopt = "${type}_${opt}";
+	$props->{$newopt} = delete $props->{$opt};
+    }
+
+    return $props;
+}
+
+sub options {
+    my $props = PVE::LXC::Config::json_config_properties();
+    my $opts = {};
+    for my $opt (keys $props->%*) {
+	my $newopt = "${type}_${opt}";
+	$opts->{$newopt} = { optional => 1 };
+    }
+    return $opts;
+}
+
+1;
diff --git a/src/PVE/Profiles/Makefile b/src/PVE/Profiles/Makefile
new file mode 100644
index 0000000..e63a9f2
--- /dev/null
+++ b/src/PVE/Profiles/Makefile
@@ -0,0 +1,4 @@
+
+.PHONY: install
+install:
+	install -D -m 0644 CT.pm ${PERLDIR}/PVE/Profiles/CT.pm
-- 
2.30.2

[pve-devel] [RFC PATCH container 2/3] api: add profile option to create ct api call

[Dominik Csapak]

we use the profile cfg as the 'param' hash, but overwrite the values
with the ones from the api call, so one can overwrite options from
the profile easily

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/API2/LXC.pm | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 28d14de..e35f859 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -27,6 +27,10 @@ use PVE::API2::LXC::Config;
 use PVE::API2::LXC::Status;
 use PVE::API2::LXC::Snapshot;
 use PVE::JSONSchema qw(get_standard_option);
+
+use PVE::Profiles::Plugin;
+use PVE::Profiles::CT;
+
 use base qw(PVE::RESTHandler);
 
 BEGIN {
@@ -196,6 +200,11 @@ __PACKAGE__->register_method({
 		default => 0,
 		description => "Start the CT after its creation finished successfully.",
 	    },
+	    profile => {
+		optional => 1,
+		type => 'string',
+		description => "The profile to use as base config.",
+	    },
 	}),
     },
     returns => {
@@ -209,6 +218,21 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $authuser = $rpcenv->get_user();
 
+	my $profile = extract_param($param, 'profile');
+	if (defined($profile)) {
+	    $rpcenv->check_full($authuser, "/mapping/profiles/${profile}", ['Mapping.Use']);
+	    my $profile_cfgs = cfs_read_file('virtual-guest/profiles.cfg');
+	    my $profile_cfg = $profile_cfgs->{ids}->{$profile};
+	    die "no such profile '$profile'\n" if !defined($profile_cfg);
+	    die "wrong type '$profile_cfg->{type}'\n" if $profile_cfg->{type} ne 'ct';
+
+	    for my $opt (keys $param->%*) {
+		$profile_cfg->{$opt} = $param->{$opt};
+	    }
+	    delete $profile_cfg->{type};
+	    $param = $profile_cfg;
+	}
+
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 	my $ignore_unpack_errors = extract_param($param, 'ignore-unpack-errors');
-- 
2.30.2

[pve-devel] [RFC PATCH container 3/3] pct: register and init the profiles plugins

[Dominik Csapak]

we have to that here, so the properties/options are correctly configured
when using that feature on the cli

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/CLI/pct.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index a0b9bce..e579b7a 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -24,6 +24,12 @@ use PVE::API2::LXC::Snapshot;
 use PVE::API2::LXC::Status;
 use PVE::API2::LXC;
 
+use PVE::Profiles::Plugin;
+use PVE::Profiles::CT;
+
+PVE::Profiles::CT->register();
+PVE::Profiles::Plugin->init();
+
 use base qw(PVE::CLIHandler);
 
 my $nodename = PVE::INotify::nodename();
-- 
2.30.2

[pve-devel] [RFC PATCH manager 1/1] api: add guest profile api endpoint

[Dominik Csapak]

basic CRUD for the profile section config

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Cluster.pm          |   6 +
 PVE/API2/Cluster/Makefile    |   1 +
 PVE/API2/Cluster/Profiles.pm | 230 +++++++++++++++++++++++++++++++++++
 3 files changed, 237 insertions(+)
 create mode 100644 PVE/API2/Cluster/Profiles.pm

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 04387ab4..6e7775dd 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -30,6 +30,7 @@ use PVE::API2::Cluster::Mapping;
 use PVE::API2::Cluster::Jobs;
 use PVE::API2::Cluster::MetricServer;
 use PVE::API2::Cluster::Notifications;
+use PVE::API2::Cluster::Profiles;
 use PVE::API2::ClusterConfig;
 use PVE::API2::Firewall::Cluster;
 use PVE::API2::HAConfig;
@@ -103,6 +104,11 @@ __PACKAGE__->register_method ({
     path => 'mapping',
 });
 
+__PACKAGE__->register_method ({
+    subclass => "PVE::API2::Cluster::Profiles",
+    path => 'profiles',
+});
+
 if ($have_sdn) {
     __PACKAGE__->register_method ({
        subclass => "PVE::API2::Network::SDN",
diff --git a/PVE/API2/Cluster/Makefile b/PVE/API2/Cluster/Makefile
index b109e5cb..35a3f871 100644
--- a/PVE/API2/Cluster/Makefile
+++ b/PVE/API2/Cluster/Makefile
@@ -9,6 +9,7 @@ PERLSOURCE= 			\
 	MetricServer.pm		\
 	Mapping.pm		\
 	Notifications.pm		\
+	Profiles.pm		\
 	Jobs.pm			\
 	Ceph.pm
 
diff --git a/PVE/API2/Cluster/Profiles.pm b/PVE/API2/Cluster/Profiles.pm
new file mode 100644
index 00000000..198f6cc7
--- /dev/null
+++ b/PVE/API2/Cluster/Profiles.pm
@@ -0,0 +1,230 @@
+package PVE::API2::Cluster::Profiles;
+
+use warnings;
+use strict;
+
+use PVE::Tools qw(extract_param extract_sensitive_params);
+use PVE::Exception qw(raise_perm_exc raise_param_exc);
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::RPCEnvironment;
+
+use PVE::Profiles::Plugin;
+use PVE::Profiles::VM;
+use PVE::Profiles::CT;
+
+PVE::Profiles::VM->register();
+PVE::Profiles::CT->register();
+PVE::Profiles::Plugin->init();
+
+use PVE::RESTHandler;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    name => 'profile_index',
+    path => '',
+    method => 'GET',
+    description => "List configured profiles.",
+    permissions => {
+	user => 'all',
+	description => "Only lists entries where you have 'Mapping.Modify', 'Mapping.Use' or".
+	    " 'Mapping.Audit' permissions on 'mapping/profiles/<id>'.",
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => {
+		id => {
+		    description => "The ID of the entry.",
+		    type => 'string'
+		},
+		type => {
+		    description => "Plugin type.",
+		    type => 'string',
+		},
+	    },
+	},
+	links => [ { rel => 'child', href => "{id}" } ],
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authuser = $rpcenv->get_user();
+
+	my $res = [];
+	my $cfg = PVE::Cluster::cfs_read_file('virtual-guest/profiles.cfg');
+	my $can_see_mapping_privs = ['Mapping.Modify', 'Mapping.Use', 'Mapping.Audit'];
+
+	for my $id (sort keys $cfg->{ids}->%*) {
+	    next if !$rpcenv->check_any($authuser, "/mapping/profiles/$id", $can_see_mapping_privs, 1);
+	    my $plugin_config = $cfg->{ids}->{$id};
+	    push @$res, {
+		id => $id,
+		type => $plugin_config->{type},
+	    };
+	}
+
+	return $res;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'read',
+    path => '{id}',
+    method => 'GET',
+    description => "Read profile configuration.",
+    permissions => {
+	check =>['or',
+	    ['perm', '/mapping/profiles/{id}', ['Mapping.Use']],
+	    ['perm', '/mapping/profiles/{id}', ['Mapping.Modify']],
+	    ['perm', '/mapping/profiles/{id}', ['Mapping.Audit']],
+	],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    id => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	},
+    },
+    returns => { type => 'object' },
+    code => sub {
+	my ($param) = @_;
+
+	my $cfg = PVE::Cluster::cfs_read_file('virtual-guest/profiles.cfg');
+	my $id = $param->{id};
+
+	if (!defined($cfg->{ids}->{$id})) {
+	    die "profile '$id' does not exist\n";
+	}
+
+	return $cfg->{ids}->{$id};
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'create',
+    path => '{id}',
+    protected => 1,
+    method => 'POST',
+    description => "Create a new profile.",
+    permissions => {
+	check => ['perm', '/mapping/profiles', ['Mapping.Modify']],
+    },
+    parameters => PVE::Profiles::Plugin->createSchema(),
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $type = extract_param($param, 'type');
+	my $plugin = PVE::Profiles::Plugin->lookup($type);
+	my $id = extract_param($param, 'id');
+
+	PVE::Cluster::cfs_lock_file('virtual-guest/profiles.cfg', undef, sub {
+	    my $cfg = PVE::Cluster::cfs_read_file('virtual-guest/profiles.cfg');
+
+	    die "Profile '$id' already exists\n"
+		if $cfg->{ids}->{$id};
+
+	    my $opts = $plugin->check_config($id, $param, 1, 1);
+
+	    $cfg->{ids}->{$id} = $opts;
+
+	    PVE::Cluster::cfs_write_file('virtual-guest/profiles.cfg', $cfg);
+	});
+	die $@ if $@;
+
+	return;
+    }});
+
+
+__PACKAGE__->register_method ({
+    name => 'update',
+    protected => 1,
+    path => '{id}',
+    method => 'PUT',
+    description => "Update profile configuration.",
+    permissions => {
+	check => ['perm', '/mapping/profiles/{id}', ['Mapping.Modify']],
+    },
+    parameters => PVE::Profiles::Plugin->updateSchema(),
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $id = extract_param($param, 'id');
+	my $digest = extract_param($param, 'digest');
+	my $delete = extract_param($param, 'delete');
+
+	if ($delete) {
+	    $delete = [PVE::Tools::split_list($delete)];
+	}
+
+	PVE::Cluster::cfs_lock_file('virtual-guest/profiles.cfg', undef, sub {
+	    my $cfg = PVE::Cluster::cfs_read_file('virtual-guest/profiles.cfg');
+
+	    PVE::SectionConfig::assert_if_modified($cfg, $digest);
+
+	    my $data = $cfg->{ids}->{$id};
+	    die "no such profile '$id'\n" if !defined($data);
+
+	    my $plugin = PVE::Profiles::Plugin->lookup($data->{type});
+	    my $opts = $plugin->check_config($id, $param, 0, 1);
+
+	    my $options = $plugin->private()->{options}->{$data->{type}};
+	    PVE::SectionConfig::delete_from_config($data, $options, $opts, $delete);
+
+	    $data->{$_} = $opts->{$_} for keys $opts->%*;
+
+	    PVE::Cluster::cfs_write_file('virtual-guest/profiles.cfg', $cfg);
+	});
+	die $@ if $@;
+
+	return;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'delete',
+    protected => 1,
+    path => '{id}',
+    method => 'DELETE',
+    description => "Remove profile.",
+    permissions => {
+	check => [ 'perm', '/mapping/profiles', ['Mapping.Modify']],
+    },
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    id => {
+		type => 'string',
+		format => 'pve-configid',
+	    },
+	}
+    },
+    returns => { type => 'null' },
+    code => sub {
+	my ($param) = @_;
+
+	my $id = $param->{id};
+
+	PVE::Cluster::cfs_lock_file('virtual-guest/profiles.cfg', undef, sub {
+	    my $cfg = PVE::Cluster::cfs_read_file('virtual-guest/profiles.cfg');
+
+	    if ($cfg->{ids}->{$id}) {
+		delete $cfg->{ids}->{$id};
+	    }
+
+	    PVE::Cluster::cfs_write_file('virtual-guest/profiles.cfg', $cfg);
+	});
+	die $@ if $@;
+
+	return;
+    }});
+
+1;
-- 
2.30.2

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Thomas Lamprecht]

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.

> 
> 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.

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 don't quite like this one, since i think a single config for all
> profiles should be enough?  We could still do that later if we want
> and the current way is impractical)

We can always go from this to the other, or vice versa, if *really*
needed, we should try hard to avoid either such move, though..

> 
> The minor issues:
> 
> * Is the priv path ok? /mapping/profiles/<id> feels a bit weird

What feels weird? s/profile/guest-profile/ might be a bit more telling
though.

> * We should probably introduce a 'meta' property for ct like we have for
>   vms? we could record the profile there and also e.g. the template used
>   to set the container up.

I assume you mean in the CT config, IIRC there is even an older patch
floating around for adding that for recording the ctime.

But it shouldn't be needed, as meta stuff should be only informational,
profile names can change (delete/re-add) or get modified, so saving the
ID has no use, logging it to the task-log is enough for starters, I think.

> * I did not restrict to any options of the config and i don't believe
>   this should make any issues (the critical ones are filtered out anyway)
>   but should we maybe have some kind of whitelist? if yes, which one
>   should be on there?

I didn't do any evaluation of all properties, but things like force-machine
and running-state properties make no sense,  so at least disallowing those
would be good.

> * there is still an issue with the docs generation, i'll have to look
>   into it
> * I'm not quite sure how the UI for creating/editing profiles should
>   look like. For creating i could imagine a 'create profile from guest'
>   type of thing (thanks @mira for the idea), but for editing or creating
>   from scratch I'm a bit conflicted. We could simply show the profiles
>   in the tree, and reuse the vm hw/options panels for that, but does
>   that seem overkill? We could also leave that API only for now, and
>   make the gui later? (Using it in the wizard would also not be trivial,
>   but there the constraints are rather straight forward)

- view: simple grid with profile ID, profile type and the (unavoidable)
  profile comment as the tree columns (for starters). The admins can
  set a descriptive ID and comment to know what a profile is intended
  for. Showing all profile values is IMO bad UX, as it becomes crowded
  super fast for more than a few trivial profiles.

- profile create & edit:
  - a per-type add, i.e., "Add VM Profile" and "Add CT Profile"
  - each provides the id and comment at the top and then has a
    panel with an add button to add a new property.
  - Most properties are simple single-fields, for storage we can add
    more complex widgets too, idealy recycling (parts of) the forms
    and/or panels we already use other where
  - I'd start out with defining good widgets for the most sensible
    properties first, that is naturally a bit subjective, but e.g.,
    memory, max memory, cpu cores, cpu model, storage (mpX, scsiX,
    virtioX), network, including (host)name, DNS (for CTs).
    Anything else can then be a "raw" field for now, and we can add
    further "first-class" widgets for properties that are deemed
    relevant due to feedback from other devs and users.

IMO a create wizard is relatively much work, but not necessarily better,
as seeing all in one view has it's advantage.

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[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

> 
>> (I don't quite like this one, since i think a single config for all
>> profiles should be enough?  We could still do that later if we want
>> and the current way is impractical)
> 
> We can always go from this to the other, or vice versa, if *really*
> needed, we should try hard to avoid either such move, though..

yes of course

> 
>>
>> The minor issues:
>>
>> * Is the priv path ok? /mapping/profiles/<id> feels a bit weird
> 
> What feels weird? s/profile/guest-profile/ might be a bit more telling
> though.

weird that it's putting in the /mapping/ dir, but if that's not an
issue for you then i'll use /mapping/guest-profile(s)/<id>
(idk if we should prefer singular or plural for that...)

> 
>> * We should probably introduce a 'meta' property for ct like we have for
>>    vms? we could record the profile there and also e.g. the template used
>>    to set the container up.
> 
> I assume you mean in the CT config, IIRC there is even an older patch
> floating around for adding that for recording the ctime.
> 
> But it shouldn't be needed, as meta stuff should be only informational,
> profile names can change (delete/re-add) or get modified, so saving the
> ID has no use, logging it to the task-log is enough for starters, I think.
> 

makes sense, i just thought maybe recording the template name + ctime, would
make sense anyway.

should i just log it for vms too, or is it ok to put it in the meta option
there?

>> * I did not restrict to any options of the config and i don't believe
>>    this should make any issues (the critical ones are filtered out anyway)
>>    but should we maybe have some kind of whitelist? if yes, which one
>>    should be on there?
> 
> I didn't do any evaluation of all properties, but things like force-machine
> and running-state properties make no sense,  so at least disallowing those
> would be good.

those are filtered out already or only present in the respective api calls.
the json_config_properties only returns the values one can set via api to
the config, so that should be ok

> 
>> * there is still an issue with the docs generation, i'll have to look
>>    into it
>> * I'm not quite sure how the UI for creating/editing profiles should
>>    look like. For creating i could imagine a 'create profile from guest'
>>    type of thing (thanks @mira for the idea), but for editing or creating
>>    from scratch I'm a bit conflicted. We could simply show the profiles
>>    in the tree, and reuse the vm hw/options panels for that, but does
>>    that seem overkill? We could also leave that API only for now, and
>>    make the gui later? (Using it in the wizard would also not be trivial,
>>    but there the constraints are rather straight forward)
> 
> - view: simple grid with profile ID, profile type and the (unavoidable)
>    profile comment as the tree columns (for starters). The admins can
>    set a descriptive ID and comment to know what a profile is intended
>    for. Showing all profile values is IMO bad UX, as it becomes crowded
>    super fast for more than a few trivial profiles.

maybe an additional button that simply shows the config, akin to the
'show configuration' button for backups?

might be overkill if we have an edit panel though..

> 
> - profile create & edit:
>    - a per-type add, i.e., "Add VM Profile" and "Add CT Profile"
>    - each provides the id and comment at the top and then has a
>      panel with an add button to add a new property.
>    - Most properties are simple single-fields, for storage we can add
>      more complex widgets too, idealy recycling (parts of) the forms
>      and/or panels we already use other where
>    - I'd start out with defining good widgets for the most sensible
>      properties first, that is naturally a bit subjective, but e.g.,
>      memory, max memory, cpu cores, cpu model, storage (mpX, scsiX,
>      virtioX), network, including (host)name, DNS (for CTs).
>      Anything else can then be a "raw" field for now, and we can add
>      further "first-class" widgets for properties that are deemed
>      relevant due to feedback from other devs and users.
> 
> IMO a create wizard is relatively much work, but not necessarily better,
> as seeing all in one view has it's advantage.
> 

yeah i'll try that and see how it goes

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Dominik Csapak]

small correction inline:

On 11/6/23 09:17, Dominik Csapak wrote:
[snip]
>>> 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

i don't mean ofc that the read/write itself is implemented in pmxcfs, but
is special handled in pve-cluster (e.g. by registering '/qemu-server/'
as the file). The parser/writer is still perl..

sry for the bad wording

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Thomas Lamprecht]

Am 06/11/2023 um 09:17 schrieb Dominik Csapak:
> On 11/4/23 09:34, Thomas Lamprecht wrote:
>> On 03/11/2023 12:53, Dominik Csapak wrote:
>>> 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)

for some definition of "nice", as I find the resulting docs, and APIs
not really nice.

I do not want to block a per-type config out-right, it just feels wrong
to me to have section configs with types, and then not be able to use
that for different types of the (in spirit) same stuff, so I'd much
rather see section config adapted to be able to actually handle
different types better through the whole stack.

> 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

For JSON schema this could be mapped with oneOf support, i.e., for
properties that exist for multiple types as different schema's one
would create a oneOf with two sub-schemas.

https://json-schema.org/understanding-json-schema/reference/combining#oneOf

That would be clean from an API POV and could be reused for the storage
API then, where we already have this 1:1, and while not perfect it
actually uses section configs for what they got made for and works
well from an API and user POV – it would be of much more use to
improve it's rough edges compared to just go the easy way (for the
dev, not users) again.

> 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?)

API split is not required, so this is obsolete. Besides, a single
config makes it easier to ensure that IDs are unique (to avoid
confusion), so even if, there would be still some advantages.


>>> 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')


Yeah, as you've followed-up, ser/de happens still in perl.
So if that route would be chosen, it'd avoid touching CFS at first, we
can still add it to the tracked files for improved versioning & caching, 
there later one, if  ever needed, removing it is a bit harder.

>> 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
> 

From my gut feeling I'd like that a bit more if we'd start out newly,
and I guess the VMID guest configs can be seen as prior art here, but
those are always a bit special (as they map to instances, not something
that we just access or provides some profile), and I would like to avoid
adding to many special cases – i.e., ideally we got one way with which
we can handle almost all of our needs, as otherwise this mental cost for
devs and users alike, will just grow.


>>> * Is the priv path ok? /mapping/profiles/<id> feels a bit weird
>>
>> What feels weird? s/profile/guest-profile/ might be a bit more telling
>> though.
> 
> weird that it's putting in the /mapping/ dir, but if that's not an
> issue for you then i'll use /mapping/guest-profile(s)/<id>
> (idk if we should prefer singular or plural for that...)

didn't we had the same discussion for mapping itself (i.e., map vs. maps
vs. mapping vs. mappings)

IIRC singular was chosen then due to be used more often and to avoid
spending to much painting that bike shed, so would do the same here and
go for singular.

> 
>>
>>> * We should probably introduce a 'meta' property for ct like we have for
>>>    vms? we could record the profile there and also e.g. the template used
>>>    to set the container up.
>>
>> I assume you mean in the CT config, IIRC there is even an older patch
>> floating around for adding that for recording the ctime.
>>
>> But it shouldn't be needed, as meta stuff should be only informational,
>> profile names can change (delete/re-add) or get modified, so saving the
>> ID has no use, logging it to the task-log is enough for starters, I think.
>>
> 
> makes sense, i just thought maybe recording the template name + ctime, would
> make sense anyway.
> 
> should i just log it for vms too, or is it ok to put it in the meta option
> there?

I'd start out with just logging it, IMO this is mostly relevant for the
creation event, as it's only applied there (i.e., profile updates do
nothing for already instantiated guests), so adding it to the config, even
if just in the meta-data part, feels wrong to me – and here again, adding
that later, e.g., after user demand shows up, is way easier than removing
it again.

>>> * there is still an issue with the docs generation, i'll have to look
>>>    into it
>>> * I'm not quite sure how the UI for creating/editing profiles should
>>>    look like. For creating i could imagine a 'create profile from guest'
>>>    type of thing (thanks @mira for the idea), but for editing or creating
>>>    from scratch I'm a bit conflicted. We could simply show the profiles
>>>    in the tree, and reuse the vm hw/options panels for that, but does
>>>    that seem overkill? We could also leave that API only for now, and
>>>    make the gui later? (Using it in the wizard would also not be trivial,
>>>    but there the constraints are rather straight forward)
>>
>> - view: simple grid with profile ID, profile type and the (unavoidable)
>>    profile comment as the tree columns (for starters). The admins can
>>    set a descriptive ID and comment to know what a profile is intended
>>    for. Showing all profile values is IMO bad UX, as it becomes crowded
>>    super fast for more than a few trivial profiles.
> 
> maybe an additional button that simply shows the config, akin to the
> 'show configuration' button for backups?
> 
> might be overkill if we have an edit panel though..

could be done, but yes, a bit redundant if there's a edit panel, as unlike
with backup jobs there's no subject that can influence the job too, like
the per-disk opt-out of backups for guests, which was IIRC one of the main
reason for the dedicated job-view there (as that information is simply not
present in the edit panel of backup jobs).

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Fiona Ebner]

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?

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Fiona Ebner]

Am 03.11.23 um 12:53 schrieb Dominik Csapak:
> +my $defaultData = {
> +    propertyList => {
> +	type => { description => 'Profile type.' },
> +	id => {
> +	    type => 'string',
> +	    description => "The ID of the profile.",
> +	    format => 'pve-configid',
> +	},

The ID is usually not a property AFAIK. Doesn't this lead to duplication
when writing the section config, i.e.

type: <ID>
	id <ID>

? Do we gain anything by having it be a property?

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Dominik Csapak]

On 11/6/23 10:22, Fiona Ebner wrote:
> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>> +my $defaultData = {
>> +    propertyList => {
>> +	type => { description => 'Profile type.' },
>> +	id => {
>> +	    type => 'string',
>> +	    description => "The ID of the profile.",
>> +	    format => 'pve-configid',
>> +	},
> 
> The ID is usually not a property AFAIK. Doesn't this lead to duplication
> when writing the section config, i.e.
> 
> type: <ID>
> 	id <ID>
> 
> ? Do we gain anything by having it be a property?

mhm? the id has to be part of the properties, otherwise
the generated api with 'createSchema' etc. would not include it.

(it isn't always named id, e.g. in the storage plugins
it's 'storage')

Re: [pve-devel] [RFC PATCH cluster/guest-common/qemu-server/container/manager] add backend profile support

[Dominik Csapak]

On 11/6/23 09:53, Thomas Lamprecht wrote:
> Am 06/11/2023 um 09:17 schrieb Dominik Csapak:
>> On 11/4/23 09:34, Thomas Lamprecht wrote:
>>> On 03/11/2023 12:53, Dominik Csapak wrote:
>>>> 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)
> 
> for some definition of "nice", as I find the resulting docs, and APIs
> not really nice.
> 
> I do not want to block a per-type config out-right, it just feels wrong
> to me to have section configs with types, and then not be able to use
> that for different types of the (in spirit) same stuff, so I'd much
> rather see section config adapted to be able to actually handle
> different types better through the whole stack.

makes sense

> 
>> 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
> 
> For JSON schema this could be mapped with oneOf support, i.e., for
> properties that exist for multiple types as different schema's one
> would create a oneOf with two sub-schemas.
> 
> https://json-schema.org/understanding-json-schema/reference/combining#oneOf
> 
> That would be clean from an API POV and could be reused for the storage
> API then, where we already have this 1:1, and while not perfect it
> actually uses section configs for what they got made for and works
> well from an API and user POV – it would be of much more use to
> improve it's rough edges compared to just go the easy way (for the
> dev, not users) again.

ok, but AFAICS we don't have support for that yet, so we'd have
to implement that first... if we want to go in that direction
i'll implement that first ofc

> 
>> 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?)
> 
> API split is not required, so this is obsolete. Besides, a single
> config makes it easier to ensure that IDs are unique (to avoid
> confusion), so even if, there would be still some advantages.
> 
> 
>>>> 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')
> 
> 
> Yeah, as you've followed-up, ser/de happens still in perl.
> So if that route would be chosen, it'd avoid touching CFS at first, we
> can still add it to the tracked files for improved versioning & caching,
> there later one, if  ever needed, removing it is a bit harder.

makes sense

> 
>>> 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
>>
> 
>  From my gut feeling I'd like that a bit more if we'd start out newly,
> and I guess the VMID guest configs can be seen as prior art here, but
> those are always a bit special (as they map to instances, not something
> that we just access or provides some profile), and I would like to avoid
> adding to many special cases – i.e., ideally we got one way with which
> we can handle almost all of our needs, as otherwise this mental cost for
> devs and users alike, will just grow.
> 
> 
>>>> * Is the priv path ok? /mapping/profiles/<id> feels a bit weird
>>>
>>> What feels weird? s/profile/guest-profile/ might be a bit more telling
>>> though.
>>
>> weird that it's putting in the /mapping/ dir, but if that's not an
>> issue for you then i'll use /mapping/guest-profile(s)/<id>
>> (idk if we should prefer singular or plural for that...)
> 
> didn't we had the same discussion for mapping itself (i.e., map vs. maps
> vs. mapping vs. mappings)
> 
> IIRC singular was chosen then due to be used more often and to avoid
> spending to much painting that bike shed, so would do the same here and
> go for singular.
> 

true, i remember now ;)

>>
>>>
>>>> * We should probably introduce a 'meta' property for ct like we have for
>>>>     vms? we could record the profile there and also e.g. the template used
>>>>     to set the container up.
>>>
>>> I assume you mean in the CT config, IIRC there is even an older patch
>>> floating around for adding that for recording the ctime.
>>>
>>> But it shouldn't be needed, as meta stuff should be only informational,
>>> profile names can change (delete/re-add) or get modified, so saving the
>>> ID has no use, logging it to the task-log is enough for starters, I think.
>>>
>>
>> makes sense, i just thought maybe recording the template name + ctime, would
>> make sense anyway.
>>
>> should i just log it for vms too, or is it ok to put it in the meta option
>> there?
> 
> I'd start out with just logging it, IMO this is mostly relevant for the
> creation event, as it's only applied there (i.e., profile updates do
> nothing for already instantiated guests), so adding it to the config, even
> if just in the meta-data part, feels wrong to me – and here again, adding
> that later, e.g., after user demand shows up, is way easier than removing
> it again.

ok, fine with me

> 
>>>> * there is still an issue with the docs generation, i'll have to look
>>>>     into it
>>>> * I'm not quite sure how the UI for creating/editing profiles should
>>>>     look like. For creating i could imagine a 'create profile from guest'
>>>>     type of thing (thanks @mira for the idea), but for editing or creating
>>>>     from scratch I'm a bit conflicted. We could simply show the profiles
>>>>     in the tree, and reuse the vm hw/options panels for that, but does
>>>>     that seem overkill? We could also leave that API only for now, and
>>>>     make the gui later? (Using it in the wizard would also not be trivial,
>>>>     but there the constraints are rather straight forward)
>>>
>>> - view: simple grid with profile ID, profile type and the (unavoidable)
>>>     profile comment as the tree columns (for starters). The admins can
>>>     set a descriptive ID and comment to know what a profile is intended
>>>     for. Showing all profile values is IMO bad UX, as it becomes crowded
>>>     super fast for more than a few trivial profiles.
>>
>> maybe an additional button that simply shows the config, akin to the
>> 'show configuration' button for backups?
>>
>> might be overkill if we have an edit panel though..
> 
> could be done, but yes, a bit redundant if there's a edit panel, as unlike
> with backup jobs there's no subject that can influence the job too, like
> the per-disk opt-out of backups for guests, which was IIRC one of the main
> reason for the dedicated job-view there (as that information is simply not
> present in the edit panel of backup jobs).

IIUC we mean different things, i meant the 'Show configuration' button of
individual backups that get read from the archive/pbs snapshot

but yes i agree that if we have an edit panel it does not provide
any additional info (also we could do it later if there is demand)

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Fiona Ebner]

Am 06.11.23 um 10:34 schrieb Dominik Csapak:
> On 11/6/23 10:22, Fiona Ebner wrote:
>> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>>> +my $defaultData = {
>>> +    propertyList => {
>>> +    type => { description => 'Profile type.' },
>>> +    id => {
>>> +        type => 'string',
>>> +        description => "The ID of the profile.",
>>> +        format => 'pve-configid',
>>> +    },
>>
>> The ID is usually not a property AFAIK. Doesn't this lead to duplication
>> when writing the section config, i.e.
>>
>> type: <ID>
>>     id <ID>
>>
>> ? Do we gain anything by having it be a property?
> 
> mhm? the id has to be part of the properties, otherwise
> the generated api with 'createSchema' etc. would not include it.
> 
> (it isn't always named id, e.g. in the storage plugins
> it's 'storage')
> 

I was just reminded of [0], where it could lead to that situation. Would
need to check if that patch still applies, because since then
Jobs/RealmSync.pm has been added.

But somebody needs to filter the 'storage' property, right? Isn't that
property actually superfluous?

E.g.

root@pve8a1 ~ # pvesm set pbsenc --storage foobar
root@pve8a1 ~ # pvesm add dir foo --storage bar --path /var/lib/vz
root@pve8a1 ~ # grep bar /etc/pve/storage.cfg
1 root@pve8a1 ~ #

[0]: https://lists.proxmox.com/pipermail/pve-devel/2022-November/054714.html

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Fiona Ebner]

Am 06.11.23 um 11:12 schrieb Fiona Ebner:
> Am 06.11.23 um 10:34 schrieb Dominik Csapak:
>> On 11/6/23 10:22, Fiona Ebner wrote:
>>> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>>>> +my $defaultData = {
>>>> +    propertyList => {
>>>> +    type => { description => 'Profile type.' },
>>>> +    id => {
>>>> +        type => 'string',
>>>> +        description => "The ID of the profile.",
>>>> +        format => 'pve-configid',
>>>> +    },
>>>
>>> The ID is usually not a property AFAIK. Doesn't this lead to duplication
>>> when writing the section config, i.e.
>>>
>>> type: <ID>
>>>     id <ID>
>>>
>>> ? Do we gain anything by having it be a property?
>>
>> mhm? the id has to be part of the properties, otherwise
>> the generated api with 'createSchema' etc. would not include it.
>>
>> (it isn't always named id, e.g. in the storage plugins
>> it's 'storage')
>>
> 
> I was just reminded of [0], where it could lead to that situation. Would
> need to check if that patch still applies, because since then
> Jobs/RealmSync.pm has been added.
> 
> But somebody needs to filter the 'storage' property, right? Isn't that
> property actually superfluous?
> 

Well, it seems to be needed by the current storage config API
implementation. For the backup job API, it's no issue if no 'id'
property is declared explicitly in the config schema.

> E.g.
> 
> root@pve8a1 ~ # pvesm set pbsenc --storage foobar
> root@pve8a1 ~ # pvesm add dir foo --storage bar --path /var/lib/vz
> root@pve8a1 ~ # grep bar /etc/pve/storage.cfg
> 1 root@pve8a1 ~ #
> 
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2022-November/054714.html
> 

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Dominik Csapak]

On 11/6/23 11:12, Fiona Ebner wrote:
> Am 06.11.23 um 10:34 schrieb Dominik Csapak:
>> On 11/6/23 10:22, Fiona Ebner wrote:
>>> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>>>> +my $defaultData = {
>>>> +    propertyList => {
>>>> +    type => { description => 'Profile type.' },
>>>> +    id => {
>>>> +        type => 'string',
>>>> +        description => "The ID of the profile.",
>>>> +        format => 'pve-configid',
>>>> +    },
>>>
>>> The ID is usually not a property AFAIK. Doesn't this lead to duplication
>>> when writing the section config, i.e.
>>>
>>> type: <ID>
>>>      id <ID>
>>>
>>> ? Do we gain anything by having it be a property?
>>
>> mhm? the id has to be part of the properties, otherwise
>> the generated api with 'createSchema' etc. would not include it.
>>
>> (it isn't always named id, e.g. in the storage plugins
>> it's 'storage')
>>
> 
> I was just reminded of [0], where it could lead to that situation. Would
> need to check if that patch still applies, because since then
> Jobs/RealmSync.pm has been added.
> 
> But somebody needs to filter the 'storage' property, right? Isn't that
> property actually superfluous?

well, yes and no,

in a section config, we always have a global 'propertyList'
and a per type 'options' list (which only references the propertylist)

when using the 'create/updateSchema' calls, *all* options from the propertylist
will be included (incl. the type) so the id is always there

in the api calls, we then use this id, to modify the config
but it isn't actually contained in the 'options' of any type
and since we extract it from the parameters, it does not actually
land in the config part

(ofc this is a bit error-prone, and forgetting to extract it would
lead to the situation you describe)

> 
> E.g.
> 
> root@pve8a1 ~ # pvesm set pbsenc --storage foobar

this behaves like i mentioned above

> root@pve8a1 ~ # pvesm add dir foo --storage bar --path /var/lib/vz

i think this simply sets the 'storage' parameter twice, where the second
one is just overwritten by the first (i.e. you can always give any parameter
multiple times, but in this case the first one overwrites the later ones,
in contrast to when the parameter is not positional)

> root@pve8a1 ~ # grep bar /etc/pve/storage.cfg
> 1 root@pve8a1 ~ #
> 
> [0]: https://lists.proxmox.com/pipermail/pve-devel/2022-November/054714.html

Re: [pve-devel] [RFC PATCH guest-common 1/1] add profiles section config plugin

[Fiona Ebner]

Am 06.11.23 um 11:38 schrieb Dominik Csapak:
> On 11/6/23 11:12, Fiona Ebner wrote:
>> Am 06.11.23 um 10:34 schrieb Dominik Csapak:
>>> On 11/6/23 10:22, Fiona Ebner wrote:
>>>> Am 03.11.23 um 12:53 schrieb Dominik Csapak:
>>>>> +my $defaultData = {
>>>>> +    propertyList => {
>>>>> +    type => { description => 'Profile type.' },
>>>>> +    id => {
>>>>> +        type => 'string',
>>>>> +        description => "The ID of the profile.",
>>>>> +        format => 'pve-configid',
>>>>> +    },
>>>>
>>>> The ID is usually not a property AFAIK. Doesn't this lead to
>>>> duplication
>>>> when writing the section config, i.e.
>>>>
>>>> type: <ID>
>>>>      id <ID>
>>>>
>>>> ? Do we gain anything by having it be a property?
>>>
>>> mhm? the id has to be part of the properties, otherwise
>>> the generated api with 'createSchema' etc. would not include it.
>>>
>>> (it isn't always named id, e.g. in the storage plugins
>>> it's 'storage')
>>>
>>
>> I was just reminded of [0], where it could lead to that situation. Would
>> need to check if that patch still applies, because since then
>> Jobs/RealmSync.pm has been added.
>>
>> But somebody needs to filter the 'storage' property, right? Isn't that
>> property actually superfluous?
> 
> well, yes and no,
> 
> in a section config, we always have a global 'propertyList'
> and a per type 'options' list (which only references the propertylist)
> 
> when using the 'create/updateSchema' calls, *all* options from the
> propertylist
> will be included (incl. the type) so the id is always there

Well, yes. If it was declared explicitly in the propertyList ;)

I checked some other section configs and they all have an id (not always
named id) property, so the question is what to do about the FIXME comment:

https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Job/Registry.pm;h=32e02728d629dca67bc479a0c25d3ea3aae2858e;hb=HEAD#l18

Should we just remove the comment, because it's more consistent to other
section configs with the ID rather than without?

For the other one

https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Job/Registry.pm;h=32e02728d629dca67bc479a0c25d3ea3aae2858e;hb=HEAD#l69

we can just drop the auto-injection, because neither VZDump/JobBase.pm
nor Jobs/RealmSync.pm declare 'id' in their 'options', so the if
condition is never true AFAICT.

> 
> in the api calls, we then use this id, to modify the config
> but it isn't actually contained in the 'options' of any type
> and since we extract it from the parameters, it does not actually
> land in the config part
> 
> (ofc this is a bit error-prone, and forgetting to extract it would
> lead to the situation you describe)
> 

Okay, I see.