[pve-devel] [PATCH widget-toolkit/manager 0/5] Ceph OSD: add detail infos

[pve-devel] [PATCH widget-toolkit/manager 0/5] Ceph OSD: add detail infos

[Aaron Lauterer]

This patch series adds 2 new API endpoints for OSDs to fetch more
detailed information about a single OSD. One for overall information and
one for a single volume (block, db, wal).

Because the api endpoint for the volume infos can take a few moments, we
most likely want to show that loading state. For this, a small patch in
the widget-toolkit->ObjectGrid is added that makes it possible to enable
this behavior.

More in the actual patches.

widget-toolkit: Aaron Lauterer (1):
  ObjectGrid: optionally show loading on reload

 src/grid/ObjectGrid.js | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

manager: Aaron Lauterer (4):
  api ceph osd: add OSD details endpoint
  api ceph osd: add volume details endpoint
  ui utils: add renderer for ceph osd addresses
  ui: osd: add details window

 PVE/API2/Ceph/OSD.pm            | 290 ++++++++++++++++++++++++++++++++
 www/manager6/Makefile           |   1 +
 www/manager6/Utils.js           |  12 ++
 www/manager6/ceph/OSD.js        |  26 +++
 www/manager6/ceph/OSDDetails.js | 289 +++++++++++++++++++++++++++++++
 5 files changed, 618 insertions(+)
 create mode 100644 www/manager6/ceph/OSDDetails.js

-- 
2.30.2

[pve-devel] [PATCH widget-toolkit 1/5] ObjectGrid: optionally show loading on reload

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 src/grid/ObjectGrid.js | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/grid/ObjectGrid.js b/src/grid/ObjectGrid.js
index b355d6d..3c01851 100644
--- a/src/grid/ObjectGrid.js
+++ b/src/grid/ObjectGrid.js
@@ -48,6 +48,8 @@ Ext.define('Proxmox.grid.ObjectGrid', {
     // see top-level doc-comment above for details/example
     gridRows: [],
 
+    showReloading: false,
+
     disabled: false,
     hideHeaders: true,
 
@@ -221,7 +223,16 @@ Ext.define('Proxmox.grid.ObjectGrid', {
 
     reload: function() {
 	let me = this;
-	me.rstore.load();
+	let param;
+	if (me.showReloading) {
+	    me.setLoading();
+	    param = {
+		callback: function() {
+		    me.setLoading(false);
+		},
+	    };
+	}
+	me.rstore.load(param);
     },
 
     getObjectValue: function(key, defaultValue) {
-- 
2.30.2

Re: [pve-devel] [PATCH widget-toolkit 1/5] ObjectGrid: optionally show loading on reload

[Thomas Lamprecht]

looks functional, two higher level comments inline.

On 01/07/2022 16:16, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  src/grid/ObjectGrid.js | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/grid/ObjectGrid.js b/src/grid/ObjectGrid.js
> index b355d6d..3c01851 100644
> --- a/src/grid/ObjectGrid.js
> +++ b/src/grid/ObjectGrid.js
> @@ -48,6 +48,8 @@ Ext.define('Proxmox.grid.ObjectGrid', {
>      // see top-level doc-comment above for details/example
>      gridRows: [],
>  
> +    showReloading: false,

reloading is a bit of a misnormer and the existing `reload` fn name doesn't
help, as we actually only do loads, we don't care if its the first one or
a successive (re-)load one. Besides that, "showing reload" is a bit to generic,
as that could also mean some loading indicator other than masking the whole
component.

Maybe `maskOnLoad` could be a more fitting config name for this (open for
better proposals with above in mind).

> +
>      disabled: false,
>      hideHeaders: true,
>  
> @@ -221,7 +223,16 @@ Ext.define('Proxmox.grid.ObjectGrid', {
>  
>      reload: function() {
>  	let me = this;
> -	me.rstore.load();
> +	let param;
> +	if (me.showReloading) {
> +	    me.setLoading();
> +	    param = {
> +		callback: function() {
> +		    me.setLoading(false);
> +		},
> +	    };

I'd avoid the param variable and just to an if/else with passing the config
object directly. Using an arrow fn makes it also a bit shorter, such a simple
object can even be placed in a single line (but no hard feelings on that
one), e.g.:

me.rstore.load({ callback: () => me.setLoading(false) });

> +	}
> +	me.rstore.load(param);
>      },
>  
>      getObjectValue: function(key, defaultValue) {

[pve-devel] [PATCH manager 2/5] api ceph osd: add OSD details endpoint

[Aaron Lauterer]

Adding a GET endpoint to .../ceph/osd/<osdid> that returns 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.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/API2/Ceph/OSD.pm | 181 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 181 insertions(+)

diff --git a/PVE/API2/Ceph/OSD.pm b/PVE/API2/Ceph/OSD.pm
index 93433b3a..33b3fdb6 100644
--- a/PVE/API2/Ceph/OSD.pm
+++ b/PVE/API2/Ceph/OSD.pm
@@ -516,6 +516,187 @@ __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 => 'osddetails',
+    path => '{osdid}',
+    method => 'GET',
+    description => "Get OSD details",
+    proxyto => 'node',
+    protected => 1,
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Datastore.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'
+	];
+	eval { run_command($cmd, errmsg => 'systemctl show error', outfunc => $parser) };
+	die $@ if $@;
+
+	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";
+	}
+
+	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;
+    }});
+
 # Check if $osdid belongs to $nodename
 # $tree ... rados osd tree (passing the tree makes it easy to test)
 sub osd_belongs_to_node {
-- 
2.30.2

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

[Aaron Lauterer]

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',
+    method => 'GET',
+    description => "Get OSD volume details",
+    proxyto => 'node',
+    protected => 1,
+    permissions => {
+	check => ['perm', '/', [ 'Sys.Audit', 'Datastore.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'];
+	eval { run_command($cmd, errmsg => 'ceph volume error', outfunc => $parser) };
+	die $@ if $@;
+
+	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}};
+	die "volume type '${type}' not found for OSD ${osdid}\n" if !$volume_data->{$type};
+
+	my $volume = $volume_data->{$type};
+
+	$raw = '';
+	$cmd = ['/sbin/lvs', $volume->{lv_path}, '--reportformat', 'json', '-o', 'lv_time'];
+	eval { run_command($cmd, errmsg => 'lvs error', outfunc => $parser) };
+	die $@ if $@;
+
+	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};
+	}
+	$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 {
-- 
2.30.2

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

[Thomas Lamprecht]

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 {

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

[Aaron Lauterer]

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

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

[Thomas Lamprecht]

On 05/07/2022 16:19, Aaron Lauterer wrote:
> Okay, so IIUC, drop the Datastore.Audit and only keep the Sys.Audit?

yes.

> 
> 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 $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?

Most of the time an eval with a single (!) function call, for example like
`eval { foo() }; die $@ if $@;`, is identical with '`foo();`, I say most of the
time because a function may avoid die directly but only set $@ manually, and in
that case the eval can help to ensure it was an error from inside the function,
as `eval {}` will reset $@ to undef.

IIRC we only do such "set $@ manually without die" in the PVE::Tools
lock_file_full (and thus the depending lock_file) method though, so just
mentioning for completeness sake.

Otherwise you're right, doing eval { .. } + die if $@ is basically done when
we require to transform the error or have additional condition for when to
actually die, e.g., a $noerr parameter.

[pve-devel] [PATCH manager 4/5] ui utils: add renderer for ceph osd addresses

[Aaron Lauterer]

Render the OSD listening addresses a bit nicer and one per line.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/Utils.js | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 7ca6a271..6499712f 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1278,6 +1278,18 @@ Ext.define('PVE.Utils', {
 	return Ext.htmlEncode(first + " " + last);
     },
 
+    // expecting the following format:
+    // [v2:10.10.10.1:6802/2008,v1:10.10.10.1:6803/2008]
+    render_ceph_osd_addr: function(value) {
+	value = value.match(/\[(.*)\]/)[1];
+	value = value.replaceAll(',', '\n');
+	let retVal = '';
+	for (const i of value.matchAll(/^(v[0-9]):(.*):([0-9]*)\/([0-9]*)$/gm)) {
+	    retVal += `${i[1]}: ${i[2]}:${i[3]}<br>`;
+	}
+	return retVal;
+    },
+
     windowHostname: function() {
 	return window.location.hostname.replace(Proxmox.Utils.IP6_bracket_match,
             function(m, addr, offset, original) { return addr; });
-- 
2.30.2

[pve-devel] [PATCH manager 5/5] ui: osd: add details window

[Aaron Lauterer]

This new windows provides more detailes about an OSD such as:
* PID
* Memory usage
* various metadata that could be of interest
* list of phyiscal disks used for the main disk, db and wal with
  additional infos about the volumes for each

A new 'Details' button is added to the OSD overview and a double click
on an OSD will also open this new window.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
I decided to add a new .js file for the details window because OSD.js is
already close to 900 lines long.

 www/manager6/Makefile           |   1 +
 www/manager6/ceph/OSD.js        |  26 +++
 www/manager6/ceph/OSDDetails.js | 286 ++++++++++++++++++++++++++++++++
 3 files changed, 313 insertions(+)
 create mode 100644 www/manager6/ceph/OSDDetails.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index d16770b1..2535aaea 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -179,6 +179,7 @@ JSSRC= 							\
 	ceph/Log.js					\
 	ceph/Monitor.js					\
 	ceph/OSD.js					\
+	ceph/OSDDetails.js				\
 	ceph/Pool.js					\
 	ceph/ServiceList.js				\
 	ceph/Services.js				\
diff --git a/www/manager6/ceph/OSD.js b/www/manager6/ceph/OSD.js
index 78f226ff..75855f95 100644
--- a/www/manager6/ceph/OSD.js
+++ b/www/manager6/ceph/OSD.js
@@ -481,6 +481,20 @@ Ext.define('PVE.node.CephOsdTree', {
 	    });
 	},
 
+	run_details: function(view, rec) {
+	    if (rec.data.host && rec.data.type === 'osd' && rec.data.id >= 0) {
+		this.details();
+	    }
+	},
+
+	details: function() {
+	    let vm = this.getViewModel();
+	    Ext.create('PVE.CephOsdDetails', {
+		nodename: vm.get('osdhost'),
+		osdid: vm.get('osdid'),
+	    }).show();
+	},
+
 	set_selection_status: function(tp, selection) {
 	    if (selection.length < 1) {
 		return;
@@ -593,6 +607,9 @@ Ext.define('PVE.node.CephOsdTree', {
     stateId: 'grid-ceph-osd',
     rootVisible: false,
     useArrows: true,
+    listeners: {
+	itemdblclick: 'run_details',
+    },
 
     columns: [
 	{
@@ -733,6 +750,15 @@ Ext.define('PVE.node.CephOsdTree', {
 		    '</tpl>',
 		],
 	    },
+	    {
+		text: gettext('Details'),
+		iconCls: 'fa fa-info-circle',
+		disabled: true,
+		bind: {
+		    disabled: '{!isOsd}',
+		},
+		handler: 'details',
+	    },
 	    {
 		text: gettext('Start'),
 		iconCls: 'fa fa-play',
diff --git a/www/manager6/ceph/OSDDetails.js b/www/manager6/ceph/OSDDetails.js
new file mode 100644
index 00000000..e7e4e5da
--- /dev/null
+++ b/www/manager6/ceph/OSDDetails.js
@@ -0,0 +1,286 @@
+Ext.define('pve-osd-details-devices', {
+    extend: 'Ext.data.Model',
+    fields: ['device', 'type', 'devices', 'size', 'support_discard', 'dev_node'],
+    idProperty: 'device',
+});
+
+Ext.define('PVE.CephOsdDetails', {
+    extend: 'Ext.window.Window',
+    alias: ['widget.pveCephOsdDetails'],
+
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    cbindData: function() {
+	let me = this;
+	me.url = `/nodes/${me.nodename}/ceph/osd/${me.osdid}`;
+	return {
+	    title: `${gettext('Details')}: OSD ${me.osdid}`,
+	};
+    },
+
+    viewModel: {
+	data: {
+	    device: '',
+	},
+    },
+
+    modal: true,
+    width: 650,
+    minHeight: 250,
+    resizable: true,
+    cbind: {
+	title: '{title}',
+    },
+
+    layout: {
+	type: 'vbox',
+	align: 'stretch',
+    },
+    defaults: {
+	layout: 'fit',
+	border: false,
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	reload: function() {
+	    let view = this.getView();
+
+	    Proxmox.Utils.API2Request({
+		url: view.url,
+		waitMsgTarget: view,
+		method: 'GET',
+		failure: function(response, opts) {
+		    Proxmox.Utils.setErrorMask(view, response.htmlStatus);
+		},
+		success: function(response, opts) {
+		    let d = response.result.data;
+		    let map_data = function(data) {
+			return Object.keys(data).sort().map(x => ({ key: x, value: data[x] }));
+		    };
+		    let osdData = map_data(d.osd);
+		    d.bdev.device = 'block';
+		    let devData = [d.bdev];
+		    if (d.db) {
+			d.db.device = 'db';
+			devData.push(d.db);
+		    }
+		    if (d.wal) {
+			d.wal.device = 'wal';
+			devData.push(d.wal);
+		    }
+		    view.osdStore.loadData(osdData);
+		    let devices = view.lookup('devices');
+		    let deviceStore = devices.getStore();
+		    deviceStore.loadData(devData);
+
+		    view.lookup('osdGeneral').rstore.fireEvent('load', view.osdStore, osdData, true);
+		    view.lookup('osdNetwork').rstore.fireEvent('load', view.osdStore, osdData, true);
+
+		    // select 'block' device automatically on first load
+		    if (devices.getSelection().length === 0) {
+			devices.setSelection(deviceStore.findRecord('device', 'block'));
+		    }
+		},
+	    });
+	},
+
+	showDevInfo: function(grid, selected) {
+	    let view = this.getView();
+	    if (selected[0]) {
+		let device = selected[0].data.device;
+		this.getViewModel().set('device', device);
+
+		let detailStore = view.lookup('volumeDetails');
+		detailStore.rstore.getProxy().setUrl(`api2/json${view.url}/volume`);
+		detailStore.rstore.getProxy().setExtraParams({ 'type': device });
+		detailStore.reload();
+	    }
+	},
+
+	init: function() {
+	    let me = this;
+	    let view = me.getView();
+	    view.lookup('osdGeneral').down('tableview').enableTextSelection=true;
+	    view.lookup('osdNetwork').down('tableview').enableTextSelection=true;
+	    view.lookup('volumeDetails').down('tableview').enableTextSelection=true;
+
+	    me.reload();
+	},
+
+	control: {
+	    'grid[reference=devices]': {
+		selectionchange: 'showDevInfo',
+	    },
+	},
+    },
+    tbar: [
+	{
+	    text: gettext('Reload'),
+	    iconCls: 'fa fa-refresh',
+	    handler: 'reload',
+	},
+    ],
+    initComponent: function() {
+        let me = this;
+
+	me.osdStore = Ext.create('Proxmox.data.ObjectStore');
+
+	Ext.applyIf(me, {
+	    items: [
+		{
+		    xtype: 'tabpanel',
+		    reference: 'detailsTabs',
+		    items: [
+			{
+			    xtype: 'proxmoxObjectGrid',
+			    reference: 'osdGeneral',
+			    tooltip: gettext('Various information about the OSD'),
+			    rstore: me.osdStore,
+			    title: gettext('General'),
+			    gridRows: [
+				{
+				    xtype: 'text',
+				    name: 'version',
+				    text: gettext('Version'),
+				},
+				{
+				    xtype: 'text',
+				    name: 'hostname',
+				    text: gettext('Hostname'),
+				},
+				{
+				    xtype: 'text',
+				    name: 'osd_data',
+				    text: gettext('OSD data path'),
+				},
+				{
+				    xtype: 'text',
+				    name: 'osd_objectstore',
+				    text: gettext('OSD object store'),
+				},
+				{
+				    xtype: 'text',
+				    name: 'mem_usage',
+				    text: gettext('Memory usage'),
+				    renderer: Proxmox.Utils.render_size,
+				},
+				{
+				    xtype: 'text',
+				    name: 'pid',
+				    text: `${gettext('Process ID')} (PID)`,
+				},
+			    ],
+			},
+			{
+			    xtype: 'proxmoxObjectGrid',
+			    reference: 'osdNetwork',
+			    tooltip: gettext('Addresses and ports used by the OSD service'),
+			    rstore: me.osdStore,
+			    title: gettext('Network'),
+			    gridRows: [
+				{
+				    xtype: 'text',
+				    name: 'front_addr',
+				    text: `${gettext('Front Address')}<br>(Client & Monitor)`,
+				    renderer: PVE.Utils.render_ceph_osd_addr,
+				},
+				{
+				    xtype: 'text',
+				    name: 'hb_front_addr',
+				    text: gettext('Heartbeat Front Address'),
+				    renderer: PVE.Utils.render_ceph_osd_addr,
+				},
+				{
+				    xtype: 'text',
+				    name: 'back_addr',
+				    text: `${gettext('Back Address')}<br>(OSD)`,
+				    renderer: PVE.Utils.render_ceph_osd_addr,
+				},
+				{
+				    xtype: 'text',
+				    name: 'hb_back_addr',
+				    text: gettext('Heartbeat Back Address'),
+				    renderer: PVE.Utils.render_ceph_osd_addr,
+				},
+			    ],
+			},
+			{
+			    xtype: 'panel',
+			    title: 'Devices',
+			    tooltip: gettext('Physical devices used by the OSD'),
+			    items: [
+				{
+				    xtype: 'grid',
+				    border: false,
+				    reference: 'devices',
+				    store: {
+					model: 'pve-osd-details-devices',
+				    },
+				    columns: {
+					items: [
+					    { text: gettext('Name'), dataIndex: 'device' },
+					    { text: gettext('Type'), dataIndex: 'type' },
+					    {
+						text: gettext('Physical Devices'),
+						dataIndex: 'devices',
+					    },
+					    {
+						text: gettext('Size'),
+						dataIndex: 'size',
+						renderer: Proxmox.Utils.render_size,
+					    },
+					    {
+						text: 'Discard',
+						dataIndex: 'support_discard',
+						hidden: true,
+					    },
+					    {
+						text: gettext('Device node'),
+						dataIndex: 'dev_node',
+						hidden: true,
+					    },
+					],
+					defaults: {
+					    tdCls: 'pointer',
+					    flex: 1,
+					},
+				    },
+				},
+				{
+				    xtype: 'proxmoxObjectGrid',
+				    reference: 'volumeDetails',
+				    showReloading: true,
+				    bind: {
+					title: Ext.String.format(gettext('Volume Details for {0}'), '{device}'),
+				    },
+				    rows: {
+					creation_time: {
+					    header: gettext('Creation time'),
+					},
+					lv_name: {
+					    header: gettext('LV Name'),
+					},
+					lv_path: {
+					    header: gettext('LV Path'),
+					},
+					lv_uuid: {
+					    header: gettext('LV UUID'),
+					},
+					vg_name: {
+					    header: gettext('VG Name'),
+					},
+				    },
+				    url: 'nodes/', //placeholder, will be set when device is selected
+				},
+			    ],
+			},
+		    ],
+		},
+	    ],
+	});
+
+	me.callParent();
+    },
+});
-- 
2.30.2