[pve-devel] [PATCH storage 1/2] Fix: 1542 - show storage utilization per pool, not per global

[Thomas Lamprecht]

comments inline.

On 11/15/2017 03:47 PM, Alwin Antreich wrote:
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index decfbf5..14386c4 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -7,6 +7,7 @@ use Net::IP;
>  use PVE::Tools qw(run_command trim);
>  use PVE::Storage::Plugin;
>  use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RADOS;
>  
>  use base qw(PVE::Storage::Plugin);
>  
> @@ -90,6 +91,15 @@ my $rados_cmd = sub {
>      return $build_cmd->('/usr/bin/rados', $scfg, $storeid, $op, @options);
>  };
>  
> +my $rados_mon_cmd = sub {
> +    my ($cmd) = @_;
> +
> +    my $rados = PVE::RADOS->new();
> +    my $res = $rados->mon_command({ prefix => $cmd, format => 'json' });

not sure if this is a good idea, there's quite a bit overhead involved
when looking at PVE::RADOS->new(), so starting a new connection each time
we want a command result defeats the libaries paradigm that multiple
methods/requests can be made with one instance of this class.

If we pass the connection to this method there is only one relevant line
left over, so maybe just omit this method, make one connection down below
and use the $rados->methods directly there, saves lines of code and is
faster.

> +
> +    return $res;
> +};
> +
>  # needed for volumes created using ceph jewel (or higher)
>  my $krdb_feature_disable = sub {
>      my ($scfg, $storeid, $name) = @_;
> @@ -527,32 +537,20 @@ sub list_images {
>  sub status {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
> -    my $cmd = &$rados_cmd($scfg, $storeid, 'df');
> -
> -    my $stats = {};
> -
> -    my $parser = sub {
> -	my $line = shift;
> -	if ($line =~ m/^\s*total(?:\s|_)(\S+)\s+(\d+)(k|M|G|T)?/) {
> -	    $stats->{$1} = $2;
> -	    # luminous has units here..
> -	    if ($3) {
> -		$stats->{$1} *= $rbd_unittobytes->{$3}/1024;
> -	    }
> -	}
> -    };
> -
> -    eval {
> -	run_rbd_command($cmd, errmsg => "rados error", errfunc => sub {}, outfunc => $parser);
> -    };
> +    my $df_cmd = &$rados_mon_cmd('df');

_cmd suggests that the variable contains a commando to be executed, not a result.
Maybe just use,
$df = ...
$pg_dump = ...

As we're here in the RBD plugin this is specific enough to see what happens.


> +    my $pg_cmd = &$rados_mon_cmd('pg dump');

You do not use the result from the 'pg dump' mon command in this patch,
but in the next one - so it should be also added there (makes reviewing
easier and the git history more useful)

>  
> -    my $total = $stats->{space} ? $stats->{space}*1024 : 0;
> -    my $free = $stats->{avail} ? $stats->{avail}*1024 : 0;
> -    my $used = $stats->{used} ? $stats->{used}*1024: 0;
> +    foreach my $d (@{$df_cmd->{pools}}) {
> +	next if ($d->{name} ne $scfg->{pool});
> +	my $s_df = $d->{stats};
> +	my $free = $s_df->{max_avail};
> +	my $used = $s_df->{bytes_used};
> +	my $total = $used + $free;
>  	my $active = 1;
>  
>  	return ($total, $free, $used, $active);
>      }
> +}
>  
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
> 
<