Re: [pve-devel] [PATCH manager 3/5] api ceph osd: add volume details endpoint

[Aaron Lauterer <a.lauterer@proxmox.com>]

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 :)
[...]