high level:
as you mentioned the path 'configkey' is not really optimal
i recently mentioned off-list that we could clean this up on
the next breaking major release with a breaking api change:
have a 'config' dir and a
'file'
'db'
and 'key'( or 'value') api endpoint inside
that represents the different things
for now a possible change could be to do it in 'config'
but with a new parameter, though that's also not ideal
any further ideas/suggestions @Thomas?
some general comments inline:
On 1/13/23 16:09, Aaron Lauterer wrote:
This new endpoint allows to get the values of config keys that are
either set in the config db or the ceph.conf file.
Values that are set in the ceph.conf file have priority over values set
in the conifg db via 'ceph config set'.
Expects the --config_keys parameter as a semicolon separated list of
"<section>:<config key>" where the section is a section in the ceph.conf
or config db. For example: global:osd_pool_default_size
Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
---
I kept it as general as possible for any potential future use.
As mentioned in the cover letter, suggestions for a better name are
welcome.
Currently it returns the data as named hashes. My main reasoning for
this, instead of flatter arrays is the following:
The client is already requesting specific config keys. Being able to
use them directly means the client doesn't have to build its own dict or
object structure from the return values.
If the requested key is not set, a warning will be logged. The return
value will be 'null'.
PVE/API2/Ceph.pm | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/PVE/API2/Ceph.pm b/PVE/API2/Ceph.pm
index 55220324..3e21f0c8 100644
--- a/PVE/API2/Ceph.pm
+++ b/PVE/API2/Ceph.pm
@@ -90,6 +90,7 @@ __PACKAGE__->register_method ({
{ name => 'cmd-safety' },
{ name => 'config' },
{ name => 'configdb' },
+ { name => 'configkey' },
{ name => 'crush' },
{ name => 'fs' },
{ name => 'init' },
@@ -180,6 +181,89 @@ __PACKAGE__->register_method ({
}});
+my $CONFIGKEY_RE = qr/[0-9a-z\-_\.]*:[0-9a-zA-Z\-_]*/i;
+my $CONFIGKEYS_RE = qr/^(:?${CONFIGKEY_RE})(:?[;, ]${CONFIGKEY_RE})*$/;
eh i get it, but imho those two names are too similar.
easy to confuse...
+
+__PACKAGE__->register_method ({
+ name => 'configkey',
+ path => 'configkey',
+ method => 'GET',
+ proxyto => 'node',
+ protected => 1,
+ permissions => {
+ check => ['perm', '/', [ 'Sys.Audit' ]],
+ },
+ description => "Get configured keys from either the config file or config
DB.",
+ parameters => {
+ additionalProperties => 0,
+ properties => {
+ node => get_standard_option('pve-node'),
+ config_keys => {
+ type => "string",
+ format => "string-list",
+ typetext => "<section>:<config key>[;<section>:<config key>]",
+ pattern => $CONFIGKEYS_RE,
is it really necessary to have the pattern include the [;, ] and other
ocurrences if we use 'string-list' with a pattern?
if yes, we could also omit the 'format' if it's already contained in the
pattern
also, it seems that you'd allow the parameter: ':' (empty section/name)
is that intentional?
+ description => "List of <section>:<config key> items.",
+ }
+ },
+ },
+ returns => {
+ type => 'object',
+ description => "Contains {section}->{key} children with the values",
+ },
+ code => sub {
+ my ($param) = @_;
+
+ PVE::Ceph::Tools::check_ceph_inited();
+
+ # Ceph treats - and _ the same in parameter names, stick with _
we currently try to use only kebab-case for parameters, so i'd use that
too for return values if it really does not matter for ceph...
+ my $normalize = sub {
+ my $t = shift;
+ $t =~ s/-/_/g;
+ return $t;
+ };
+
+ my $requested_keys = [];
+ for my $pair (PVE::Tools::split_list($param->{config_keys})) {
+ my ($section, $key) = split(":", $pair);
+ $section = $normalize->($section);
+ $key = $normalize->($key);
+ push(@{$requested_keys}, { section => $section, key => $key });
+ }
+
+ my $config = {};
+
+ my $rados = PVE::RADOS->new();
+ my $configdb = $rados->mon_command( { prefix => 'config dump', format
=> 'json' });
+ for my $s (@{$configdb}) {
+ my ($section, $name, $value) = $s ->@{'section', 'name', 'value'};
+ $config->{$normalize->($section)}->{$normalize->($name)} = $value;
+ }
+
+ # read ceph.conf after config db as it has priority if settings are
present in both
+ my $config_file = cfs_read_file('ceph.conf');
+ for my $section (keys %{$config_file}) {
+ for my $key (keys %{$config_file->{$section}}) {
+ $config->{$normalize->($section)}->{$normalize->($key)}
+ = $config_file->{$section}->{$key};
+ }
+ }
+
+ my $ret = {};
+
+ for my $pair (@{$requested_keys}) {
+ my ($section, $key) = $pair->@{'section', 'key'};
+ warn "section '${section}' does not exist in config" if
!defined($config->{$section});
+ warn "key '${section}:${key}' does not exist in config"
+ if !defined($config->{$section}->{$key});
+
+ $ret->{$section}->{$key} = $config->{$section}->{$key};
+ }
couldn't you directly filter the necessary section/names when iterating
over the config/db ? that would remove the extra step of filtering at
the end
+
+ return $ret;
+ }});
+
+
__PACKAGE__->register_method ({
name => 'init',
path => 'init',
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
