Re: [pve-devel] [PATCH manager v2 2/4] api ceph osd: add OSD index, metadata and lv-info

[Dominik Csapak <d.csapak@proxmox.com>]

LGTM, some comments inline (mostly nitpicks/qestions)

On 7/6/22 15:01, Aaron Lauterer wrote:
> To get more details for a single OSD, we add two new endpoints:
> * nodes/{node}/ceph/osd/{osdid}/metadata
> * nodes/{node}/ceph/osd/{osdid}/lv-info
> 
> The {osdid} endpoint itself gets a new GET handler to return the index.
> 
> The metadata one provides various metadata regarding the OSD.
> 
> Such as
> * process id
> * memory usage
> * info about devices used (bdev/block, db, wal)
>      * size
>      * disks used (sdX)
>      ...
> * network addresses and ports used
> ...
> 
> Memory usage and PID are retrieved from systemd while the rest can be
> retrieved from the metadata provided by Ceph.
> 
> The second one (lv-info) returns the following infos for a logical
> 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>
> ---
> changes since v1:
> - squashed all API commits into one
> - moved all new API endpoints into sub endpoints to {osdid}
> - {osdid} itself returns the necessary index
> - incorporated other code improvements
> 
>   PVE/API2/Ceph/OSD.pm | 315 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 315 insertions(+)
> 
> diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
> index 93433b3a..170c4fd3 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;
> @@ -516,6 +517,320 @@ __PACKAGE__->register_method ({
>   	return $rpcenv->fork_worker('cephcreateosd', $devs->{dev}->{name},  $authuser, $worker);
>       }});
>   
> +my $OSD_DEV_RETURN_PROPS = {
> +    dev_node => {
> +	type => 'string',
> +	description => 'Device node',
> +    },
> +    devices => {
> +	type => 'string',
> +	description => 'Physical disks used',
> +    },
> +    size => {
> +	type => 'integer',
> +	description => 'Size in bytes',
> +    },
> +    support_discard => {
> +	type => 'boolean',
> +	description => 'Discard support of the physical device',
> +    },
> +    type => {
> +	type => 'string',
> +	description => 'Type of device. For example, hdd or ssd',
> +    },
> +};
> +
> +__PACKAGE__->register_method ({
> +    name => 'osdindex',
> +    path => '{osdid}',
> +    method => 'GET',
> +    permissions => { user => 'all' },
> +    description => "OSD index.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    osdid => {
> +		description => 'OSD ID',
> +		type => 'integer',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {},
> +	},
> +	links => [ { rel => 'child', href => "{name}" } ],
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $result = [
> +	    { name => 'metadata' },
> +	    { name => 'lv-info' },
> +	];
> +
> +	return $result;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'osddetails',
> +    path => '{osdid}/metadata',
> +    method => 'GET',
> +    description => "Get OSD details",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit' ], any => 1],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    osdid => {
> +		description => 'OSD ID',
> +		type => 'integer',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +	properties => {
> +	    osd => {
> +		type => 'object',
> +		description => 'General information about the OSD',
> +		properties => {
> +		    hostname => {
> +			type => 'string',
> +			description => 'Name of the host containing the OSD.',
> +		    },
> +		    id => {
> +			type => 'integer',
> +			description => 'ID of the OSD.',
> +		    },
> +		    mem_usage => {
> +			type => 'integer',
> +			description => 'Memory usage of the OSD service.',
> +		    },
> +		    osd_data => {
> +			type => 'string',
> +			description => "Path to the OSD's data directory.",
> +		    },
> +		    osd_objectstore => {
> +			type => 'string',
> +			description => 'The type of object store used.',
> +		    },
> +		    pid => {
> +			type => 'integer',
> +			description => 'OSD process ID.',
> +		    },
> +		    version => {
> +			type => 'string',
> +			description => 'Ceph version of the OSD service.',
> +		    },
> +		    front_addr => {
> +			type => 'string',
> +			description => 'Address and port used to talk to clients and monitors.',
> +		    },
> +		    back_addr => {
> +			type => 'string',
> +			description => 'Address and port used to talk to other OSDs.',
> +		    },
> +		    hb_front_addr => {
> +			type => 'string',
> +			description => 'Heartbeat address and port for clients and monitors.',
> +		    },
> +		    hb_back_addr => {
> +			type => 'string',
> +			description => 'Heartbeat address and port for other OSDs.',
> +		    },
> +		},
> +	    },
> +	    bdev => {
> +		type => 'object',
> +		description => 'Data about the OSD block device',
> +		properties => $OSD_DEV_RETURN_PROPS,
> +	    },
> +	    db => {
> +		type => 'object',
> +		description => 'Data about the DB device (optional)',
> +		properties => $OSD_DEV_RETURN_PROPS,
> +		optional => 1,
> +	    },
> +	    wal => {
> +		type => 'object',
> +		description => 'Data about the WAL device (optional)',
> +		properties => $OSD_DEV_RETURN_PROPS,
> +		optional => 1,
> +	    },
> +	}
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	PVE::Ceph::Tools::check_ceph_inited();
> +
> +	my $osdid = $param->{osdid};
> +	my $rados = PVE::RADOS->new();
> +	my $metadata = $rados->mon_command({ prefix => 'osd metadata', id => int($osdid) });
> +
> +	die "OSD '${osdid}' does not exists on host '${nodename}'\n"
> +	    if $nodename ne $metadata->{hostname};
> +
> +	my $raw = '';
> +	my $pid;
> +	my $memory;
> +	my $parser = sub { $raw .= shift };
> +	my $cmd = [
> +	    '/bin/systemctl',
> +	    'show',
> +	    "ceph-osd\@${osdid}.service",
> +	    '--property=MainPID,MemoryCurrent'
> +	];

nit: personally i'd prefer to have the value of the parameter on it's own
i.e. :

'--property', 'MainPID,MemoryCurrent'

but it's not really a blocker for me


> +	run_command($cmd, errmsg => 'systemctl show error', outfunc => $parser);
> +
> +	if ($raw =~ m/^MainPID=([0-9]*)MemoryCurrent=([0-9]*|\[not set\])$/s) { #untaint
> +	    $pid = $1;
> +	    $memory = $2 eq "[not set]" ? 0 : $2;
> +	} else {
> +	    die "got unexpected data from systemctl: '${raw}'\n";
> +	}

it works, but wouldn't it also be possible to parse those two
things on their own line in the parser? that way you'd not need the intermediate
$raw and would (IMHO) make the code a bit cleaner

> +
> +	my $data = {
> +	    osd => {
> +		hostname => $metadata->{hostname},
> +		id => $metadata->{id},
> +		mem_usage => int($memory),
> +		osd_data => $metadata->{osd_data},
> +		osd_objectstore => $metadata->{osd_objectstore},
> +		pid => int($pid),
> +		version => "$metadata->{ceph_version_short} ($metadata->{ceph_release})",
> +		front_addr => $metadata->{front_addr},
> +		back_addr => $metadata->{back_addr},
> +		hb_front_addr => $metadata->{hb_front_addr},
> +		hb_back_addr => $metadata->{hb_back_addr},
> +	    },
> +	};
> +
> +	my $get_data = sub {
> +	    my ($dev, $prefix) = @_;
> +	    $data->{$dev} = {
> +		dev_node => $metadata->{"${prefix}_${dev}_dev_node"},
> +		devices => $metadata->{"${prefix}_${dev}_devices"},
> +		size => int($metadata->{"${prefix}_${dev}_size"}),
> +		support_discard => int($metadata->{"${prefix}_${dev}_support_discard"}),
> +		type => $metadata->{"${prefix}_${dev}_type"},
> +	    };
> +	};
> +
> +	$get_data->("bdev", "bluestore");
> +	$get_data->("db", "bluefs") if $metadata->{bluefs_dedicated_db};
> +	$get_data->("wal", "bluefs") if $metadata->{bluefs_dedicated_wal};
> +
> +	return $data;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'osdvolume',
> +    path => '{osdid}/lv-info',
> +    method => 'GET',
> +    description => "Get OSD volume details",
> +    proxyto => 'node',
> +    protected => 1,
> +    permissions => {
> +	check => ['perm', '/', [ 'Sys.Audit' ], any => 1],
> +    },
> +    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`.",
> +	    },
> +	    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'];
> +	run_command($cmd, errmsg => 'ceph volume error', outfunc => $parser);
> +
> +	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 $lv_data = { map { $_->{type} => $_ } @{$result->{$osdid}} };
> +	my $volume = $lv_data->{$type} || die "volume type '${type}' not found for OSD ${osdid}\n";
> +
> +	$raw = '';
> +	$cmd = ['/sbin/lvs', $volume->{lv_path}, '--reportformat', 'json', '-o', 'lv_time'];
> +	run_command($cmd, errmsg => 'lvs error', outfunc => $parser);
> +
> +	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 = { 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 {