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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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 <a.lauterer@proxmox.com>
> ---
> 
> 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 {