Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys

[Dominik Csapak <d.csapak@proxmox.com>]

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.lauterer@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',