[pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s

[pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s

[Aaron Lauterer]

Printing a lot of very detailed JSON output on the CLI is not very
useful.

Printing the `ceph -s` overview is much more suited to give an overview
of the ceph cluster status.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

@Thomas: I hope this is what you hand in mind when we did discuss this
off list.

 PVE/CLI/pveceph.pm | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
index 3d7bf2b1..e12a9338 100755
--- a/PVE/CLI/pveceph.pm
+++ b/PVE/CLI/pveceph.pm
@@ -175,6 +175,20 @@ __PACKAGE__->register_method ({
 	return undef;
     }});
 
+__PACKAGE__->register_method ({
+    name => 'status',
+    path => 'status',
+    method => 'GET',
+    description => "Get Ceph Status.",
+    parameters => {
+	additionalProperties => 0,
+    },
+    returns => { type => 'null' },
+    code => sub {
+	eval { run_command(['ceph', '-s'], outfunc => sub { print "$_[0]\n" }, errfunc => sub { print STDERR "$_[0]\n" }) };
+	return undef;
+    }});
+
 our $cmddef = {
     init => [ 'PVE::API2::Ceph', 'init', [], { node => $nodename } ],
     pool => {
@@ -229,11 +243,7 @@ our $cmddef = {
     stop => [ 'PVE::API2::Ceph', 'stop', [], { node => $nodename }, $upid_exit],
     install => [ __PACKAGE__, 'install', [] ],
     purge => [  __PACKAGE__, 'purge', [] ],
-    status => [ 'PVE::API2::Ceph', 'status', [], { node => $nodename }, sub {
-	my $res = shift;
-	my $json = JSON->new->allow_nonref;
-	print $json->pretty->encode($res) . "\n";
-    }],
+    status => [ __PACKAGE__, 'status', []],
 };
 
 1;
-- 
2.20.1

Re: [pve-devel] [PATCH manager] pveceph: change status from long JSON to ceph -s

[Thomas Lamprecht]

On 18/12/2020 17:00, Aaron Lauterer wrote:
> Printing a lot of very detailed JSON output on the CLI is not very
> useful.
> 
> Printing the `ceph -s` overview is much more suited to give an overview
> of the ceph cluster status.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> @Thomas: I hope this is what you hand in mind when we did discuss this
> off list.
> 

yeah, this looks fine in general, a code style nit and a comment/questions about
the use of eval below.


>  PVE/CLI/pveceph.pm | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/CLI/pveceph.pm b/PVE/CLI/pveceph.pm
> index 3d7bf2b1..e12a9338 100755
> --- a/PVE/CLI/pveceph.pm
> +++ b/PVE/CLI/pveceph.pm
> @@ -175,6 +175,20 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'status',
> +    path => 'status',
> +    method => 'GET',
> +    description => "Get Ceph Status.",
> +    parameters => {
> +	additionalProperties => 0,
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +	eval { run_command(['ceph', '-s'], outfunc => sub { print "$_[0]\n" }, errfunc => sub { print STDERR "$_[0]\n" }) };

is the eval really required? silencing execution errors may not be ideal,
I'd either drop it completely or if deemed as required do a `warn $@ if $@;`

And this is a bit long for a single line, we adapted rules from perl to be
closer to the ones from rust, see 80 as soft-limit (i.e., not good if you reach that
(branch) indentation, but totally fine for messages or improved readability) and 100
as hard one.
https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings

So rather do:

run_command(
    [ceph', '-s'],
    outfunc => sub { print "$_[0]\n" },
    errfunc => sub { print STDERR "$_[0]\n" },
);

(with and without eval)

> +	return undef;
> +    }});
> +
>  our $cmddef = {
>      init => [ 'PVE::API2::Ceph', 'init', [], { node => $nodename } ],
>      pool => {
> @@ -229,11 +243,7 @@ our $cmddef = {
>      stop => [ 'PVE::API2::Ceph', 'stop', [], { node => $nodename }, $upid_exit],
>      install => [ __PACKAGE__, 'install', [] ],
>      purge => [  __PACKAGE__, 'purge', [] ],
> -    status => [ 'PVE::API2::Ceph', 'status', [], { node => $nodename }, sub {
> -	my $res = shift;
> -	my $json = JSON->new->allow_nonref;
> -	print $json->pretty->encode($res) . "\n";
> -    }],
> +    status => [ __PACKAGE__, 'status', []],
>  };
>  
>  1;
>