[pve-devel] [PATCH storage 2/2] Addition to #1542, get precent_used from RBDPlugin - add librados2-perl for rados commands - get RBD storage status through librados - calculate %USED if not available through librados (pre Kraken)

[Thomas Lamprecht]

Your commit message heading is way to long, try to stay below 60 chars
(to allow space for tags) with 80 as upper hard limit.

On 11/15/2017 03:47 PM, Alwin Antreich wrote:
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
>  PVE/CLI/pvesm.pm         |  1 +
>  PVE/Storage.pm           |  6 ++++--
>  PVE/Storage/RBDPlugin.pm | 14 +++++++++++++-
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
> index 9455595..006f97f 100755
> --- a/PVE/CLI/pvesm.pm
> +++ b/PVE/CLI/pvesm.pm
> @@ -144,6 +144,7 @@ my $print_status = sub {
>  	my $active = $res->{active} ? 'active' : 'inactive';
>  	my ($per, $per_fmt) = (0, '% 7.2f%%');
>  	$per = ($res->{used}*100)/$res->{total} if $res->{total} > 0;
> +	$per = $res->{percent_used} if $res->{percent_used};

$per = $res->{percent_used} if defined($res->{percent_used});
else you throw away 0% if the pool is empty.

>  
>  	if (!$res->{enabled}) {
>  	    $per = 'N/A';
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 73b21e1..ada9baf 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1063,14 +1063,16 @@ sub storage_info {
>  	    next;
>  	}
>  
> -	my ($total, $avail, $used, $active);
> -	eval { ($total, $avail, $used, $active) = $plugin->status($storeid, $scfg, $cache); };
> +	my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});

$plugin was already declared and instantiated exactly the same way
a bit above, line #1051, why do you redo that here?

> +	my ($total, $avail, $used, $active, $percent_used);
> +	eval { ($total, $avail, $used, $active, $percent_used) = $plugin->status($storeid, $scfg, $cache); };

not directly related to your changes, but as we never check for
the perl return context (wantarray) in $plugin->status this could be:
my ($total, $avail, $used, $active, $percent_used) = eval { $plugin->status($storeid, $scfg, $cache) };

>  	warn $@ if $@;
>  	next if !$active;
>  	$info->{$storeid}->{total} = int($total);
>  	$info->{$storeid}->{avail} = int($avail);
>  	$info->{$storeid}->{used} = int($used);
>  	$info->{$storeid}->{active} = $active;
> +	$info->{$storeid}->{percent_used} = $percent_used;
>      }
>  
>      return $info;
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 14386c4..74d19a8 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -540,6 +540,8 @@ sub status {
>      my $df_cmd = &$rados_mon_cmd('df');
>      my $pg_cmd = &$rados_mon_cmd('pg dump');
>  
> +    my ($free, $used, $total, $percent_used) = 0;

FYI, only $free got assigned zero here, all other variables are
still undefined.

You could use:
my ($free, $used, $total, $percent_used) = (0) x 4;

but to be honest, I do not really like that, and we use this
almost never in our code base.

> +
>      foreach my $d (@{$df_cmd->{pools}}) {
>  	next if ($d->{name} ne $scfg->{pool});
>  	my $s_df = $d->{stats};
> @@ -547,8 +549,18 @@ sub status {
>  	my $used = $s_df->{bytes_used};
>  	my $total = $used + $free;
>  	my $active = 1;
> +	my $calculation = 0;

$calculation is to generic for a variable name here.
maybe $pool_used?


> +
> +	foreach my $e (@{$pg_cmd->{pool_stats}}) {
> +	    next if ($d->{id} ne $e->{poolid});

this foreach pattern is a bit confusing, IMO, a) you always cycle
through all elements, even if you'd find the one you search in
the first loop round and further, b) the calculations in the loop
suggest that they are incremental calculations (esp. with a) in mind)
but then $calculation just gets overwritten.

Maybe it would be nicer to first find the element, and then do the
math/assignment outside of the loop making it more  clear that the
loop is only for finding the element - not for the calculation itself.

The finding could be move to a private helper method or even a closure.


> +	    my $s_pg = $e->{stat_sum};
> +	    $calculation = $used * (($s_pg->{num_object_copies} - $s_pg->{num_objects_degraded}) / $s_pg->{num_object_copies});

Describing why you do this here would be nice, small comment would
be enough (and maybe a note in the commit message too)

AFAIT, you take the percentage of non-degraded objects and multiply
it with the total used amount (are degraded objects not taking space
at all?). In the case where nothing is degraded this would just be
$used.


> 
> num_objects_degraded
> 
>     number of objects replicated fewer times than they should be (but found on at least one OSD)
-- [1]

So this does not mean that it takes no space, but that not all copies are there,
e.g. under replication 3 and object which was only replicated 2 times until now
would count towards this and you assume that it takes no space, but it takes 2/3
of the final space. So $pool_used (i call $calculation this way, as else its just
to confusing for me)

> +	    $calculation /= ($calculation + $free);

This could be written as:
$pool_used = $pool_used / ($pool_used + $total_free);

It looks OK for the case of no degradation and no pool quotas, but this is
completely wrong when pool quotas are set [2], as you assume that the free space
is always shared equally between all pool in a first come first serve manner.


> +	}
> +
> +	$percent_used = $s_df->{percent_used} ? $s_df->{percent_used} : ($calculation * 100);

0 percent_used not possible, maybe use:

$percent_used = $s_df->{percent_used} // ($calculation * 100);

>  
> -	return ($total, $free, $used, $active);
> +	return ($total, $free, $used, $active, $percent_used);
>      }
>  }
>  
> 

[1] http://docs.ceph.com/docs/giant/rados/api/librados/#num_objects_degraded
[2] http://docs.ceph.com/docs/jewel/rados/operations/pools/#set-pool-quotas