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 8B49D7AAFB for ; Tue, 5 Jul 2022 16:19:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8180B16EF for ; Tue, 5 Jul 2022 16:19:39 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 5 Jul 2022 16:19:37 +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 BC25E406C5 for ; Tue, 5 Jul 2022 16:19:37 +0200 (CEST) Message-ID: <7e833330-c2bc-d392-2690-fc828e600adb@proxmox.com> Date: Tue, 5 Jul 2022 16:19:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.0 To: Thomas Lamprecht , Proxmox VE development discussion References: <20220701141642.2743824-1-a.lauterer@proxmox.com> <20220701141642.2743824-4-a.lauterer@proxmox.com> <1bc425f0-5b8c-833b-6f8f-e1583357fc1d@proxmox.com> Content-Language: en-US From: Aaron Lauterer In-Reply-To: <1bc425f0-5b8c-833b-6f8f-e1583357fc1d@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.017 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 - 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 14:19:39 -0000 On 7/5/22 11:58, Thomas Lamprecht wrote: > 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. I can send a V2 with the suggested changes. [...] >> >> +__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. > Thanks for the explanation. Will change the API to the latter variant, plus the index endpoint for {osdid} itself. >> + 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) Okay, so IIUC, drop the Datastore.Audit and only keep the Sys.Audit? > [...] >> + 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. Will do. > [...] >> + 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 $@? Being unsure ;) Looking closer at run_command, it will die if there is some issue running the specified command, not found or returning an error. So this would only be needed if I would like to catch the error to handle it in some way, right? > >> + >> + 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} } }; yeah... missed that when cleaning up the code > >> + 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" will change that > >> + >> + $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) }; > true :) [...]