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

[Fabian Grünbichler]

On Wed, Nov 15, 2017 at 03:47:27PM +0100, 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;

this means a new Build-Depends and Depends in debian/control!

>  
>  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' });

this (implicitly) assumes an /etc/ceph/ceph.conf because you don't pass
any parameters to new(), but the RBDPlugin is also used for external
clusters where we only have a storage.cfg (or a storage.cfg and a
minimal client ceph.conf with no monitor info). see rados/rbd/build_cmd
for a potential way to handle this.

I would also suggest factoring out the connection creation so that you
can re-use a RADOS connection when doing multiple commands in a row
(like below in status).

> +
> +    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');
> +    my $pg_cmd = &$rados_mon_cmd('pg dump');
>  
> -    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);

you should probably check for defined-ness to catch potential errors,
and move the variables and return out of the loop (you could also use
map to get the desired stats hash from the returned list instead of a
loop).

>      }
> +}
>  
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel