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 5C1C77A7D6 for ; Tue, 5 Jul 2022 11:59:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 494852D4B5 for ; Tue, 5 Jul 2022 11:58:42 +0200 (CEST) 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 ; Tue, 5 Jul 2022 11:58:40 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 669AF40F90 for ; Tue, 5 Jul 2022 11:58:40 +0200 (CEST) Message-ID: <1bc425f0-5b8c-833b-6f8f-e1583357fc1d@proxmox.com> Date: Tue, 5 Jul 2022 11:58:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Thunderbird/103.0 Content-Language: en-GB To: Proxmox VE development discussion , Aaron Lauterer References: <20220701141642.2743824-1-a.lauterer@proxmox.com> <20220701141642.2743824-4-a.lauterer@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220701141642.2743824-4-a.lauterer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.004 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [osd.pm] Subject: Re: [pve-devel] [PATCH manager 3/5] api ceph osd: add volume details endpoint 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: Tue, 05 Jul 2022 09:59:12 -0000 Replying here albeit a few things also apply on the 2/5 patch. Note that I didn't checked the series as a whole (meaning, full UI and backend integration) but noticed a API issues here that has to be fixed anyway, so that plus some drive by reviews inline. On 01/07/2022 16:16, Aaron Lauterer wrote: > This endpoint returns the following infos for an volume: > * creation time > * lv name > * lv path > * lv size > * lv uuid > * vg name > > Possible volumes are: > * block (default value if not provided) > * db > * wal > > 'ceph-volume' is used to gather the infos, except for the creation time > of the LV which is retrieved via 'lvs'. > > Signed-off-by: Aaron Lauterer > --- > > While Ceph itself does not store any information about the creation time > of an OSD, I think we can get close to it by using the information > stored in the LVs. > > PVE/API2/Ceph/OSD.pm | 109 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 109 insertions(+) > > diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm > index 078a8c81..c9ec3222 100644 > --- a/PVE/API2/Ceph/OSD.pm > +++ b/PVE/API2/Ceph/OSD.pm > @@ -5,6 +5,7 @@ use warnings; > > use Cwd qw(abs_path); > use IO::File; > +use JSON; > use UUID; > > use PVE::Ceph::Tools; > @@ -697,6 +698,114 @@ __PACKAGE__->register_method ({ > return $data; > }}); > > +__PACKAGE__->register_method ({ > + name => 'osdvolume', > + path => '{osdid}/volume', adding a API path that isn't returned by any index breaks API schema generation, and the UX is also somewhat affected, NAK. For this to work the `{osdid}` API call would need to return the sub-paths too and set the href property to ensure the API stacks links them and allows to do the common completion in pvesh and correct display in the API viewer, among other things. But, that'd be naturally ugly too because you need to mix some generated data specific to the OSD with some static API path router/schema stuff. Instead, please transform {osdid} to a plain GET index call returning only the sub routes, and then add a separate sub-route in there for the details endpoint from patch 2/5, iow. the end result would be: {osdid}/ {osdid}/details {osdid}/volume Or with slightly changed route path names {osdid}/ {osdid}/metadata {osdid}/lv-info That would also allow to adding further routes in there in the future. > + method => 'GET', > + description => "Get OSD volume details", > + proxyto => 'node', > + protected => 1, > + permissions => { > + check => ['perm', '/', [ 'Sys.Audit', 'Datastore.Audit' ], any => 1], I know we use Datastore.Audit for some other ceph API calls, but that is mostly for higher level info about Proxmox VE registered storage clients, to be specific, the docs declare it as "view/browse a datastore". So I'd like to avoid expanding that usage further, especially for such detailed API calls that return a lot of specific system information. (same applies for patch 2/3) > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + node => get_standard_option('pve-node'), > + osdid => { > + description => 'OSD ID', > + type => 'integer', > + }, > + type => { > + description => 'OSD device type', > + type => 'string', > + enum => ['block', 'db', 'wal'], > + default => 'block', > + optional => 1, > + }, > + }, > + }, > + returns => { > + type => 'object', > + properties => { > + creation_time => { > + type => 'string', > + description => "Creation time as reported by 'lvs'.", I'd use backticks for CLI tools, as this is interpreted by asciidoc too for manpage generation. > + }, > + lv_name => { > + type => 'string', > + description => 'Name of the logical volume (LV).', > + }, > + lv_path => { > + type => 'string', > + description => 'Path to the logical volume (LV).', > + }, > + lv_size => { > + type => 'integer', > + description => 'Size of the logical volume (LV).', > + }, > + lv_uuid => { > + type => 'string', > + description => 'UUID of the logical volume (LV).', > + }, > + vg_name => { > + type => 'string', > + description => 'Name of the volume group (VG).', > + }, > + }, > + }, > + code => sub { > + my ($param) = @_; > + > + PVE::Ceph::Tools::check_ceph_inited(); > + > + my $osdid = $param->{osdid}; > + my $type = $param->{type} // 'block'; > + > + my $raw = ''; > + my $parser = sub { $raw .= shift }; > + my $cmd = ['/usr/sbin/ceph-volume', 'lvm', 'list', '--format', 'json']; > + eval { run_command($cmd, errmsg => 'ceph volume error', outfunc => $parser) }; > + die $@ if $@; what's the point in eval'ing if you do a plain die then anyway on $@? > + > + my $result; > + if ($raw =~ m/^(\{.*\})$/s) { #untaint > + $result = JSON::decode_json($1); > + } else { > + die "got unexpected data from ceph-volume: '${raw}'\n"; > + } > + die "OSD '${osdid}' not found in 'ceph-volume lvm list' on node '${nodename}'\n" > + if !$result->{$osdid}; > + > + my $volume_data = {}; > + %{$volume_data} = map { $_->{type} => $_ } @{$result->{$osdid}}; huh? why not just my $volume_data = { map { $_->{type} => $_ } @{$result->{$osdid} } }; > + die "volume type '${type}' not found for OSD ${osdid}\n" if !$volume_data->{$type}; > + > + my $volume = $volume_data->{$type}; just fyi, we can do a conditional die on variable declaration just fine, compared to the always bad conditionally `my $foo = "bar" if $condition` declaration. The former isn't touching the conditionality of the declaration (i.e., code will never be executed with a unsound value in there), while the latter means that there is an unsound value used when the condition evaluates to false. E.g., can be fine and sometimes shorter (albeit no hard feelings, I just like to combine extraction and error in one, like rust can do much more nicely) my $volume = $volume_data->{$type} || die "volume type '${type}' not found for OSD ${osdid}\n" > + > + $raw = ''; > + $cmd = ['/sbin/lvs', $volume->{lv_path}, '--reportformat', 'json', '-o', 'lv_time']; > + eval { run_command($cmd, errmsg => 'lvs error', outfunc => $parser) }; > + die $@ if $@; same as above wrt. probably useless eval > + > + if ($raw =~ m/(\{.*\})$/s) { #untaint, lvs has whitespace at beginning > + $result = JSON::decode_json($1); > + } else { > + die "got unexpected data from lvs: '${raw}'\n"; > + } > + my $data = {}; > + > + my $keys = [ 'lv_name', 'lv_path', 'lv_uuid', 'vg_name' ]; > + for my $key (@$keys) { > + $data->{$key} = $volume->{$key}; > + } could avoid about four lines with: my $data = { map { $_ => $volume->{$_} } qw(lv_name lv_path lv_uuid vg_name) }; > + $data->{lv_size} = int($volume->{lv_size}); > + > + $data->{creation_time} = @{$result->{report}}[0]->{lv}[0]->{lv_time}; > + > + return $data; > + }}); > + > # Check if $osdid belongs to $nodename > # $tree ... rados osd tree (passing the tree makes it easy to test) > sub osd_belongs_to_node {