From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A7A9691E4 for ; Wed, 8 Mar 2023 13:14:53 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8547920175 for ; Wed, 8 Mar 2023 13:14:53 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 8 Mar 2023 13:14:52 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 53CD145E50 for ; Wed, 8 Mar 2023 13:14:52 +0100 (CET) Message-ID: <0d08ee63-9f1e-2218-e8e7-358196d795c3@proxmox.com> Date: Wed, 8 Mar 2023 13:14:50 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Content-Language: en-US To: Proxmox VE development discussion , Aaron Lauterer References: <20230113150930.857270-1-a.lauterer@proxmox.com> <20230113150930.857270-2-a.lauterer@proxmox.com> From: Dominik Csapak In-Reply-To: <20230113150930.857270-2-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.061 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [ceph.pm] Subject: Re: [pve-devel] [PATCH manager 1/3] api: ceph: add endpoint to fetch config keys X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Mar 2023 12:14:53 -0000 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 > "
:" 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 > --- > > 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 => "
:[;
:]", > + 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
: 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',