[pve-devel] [PATCH storage] rbd: fix #4060 show data-pool usage when configured

[pve-devel] [PATCH storage] rbd: fix #4060 show data-pool usage when configured

[Aaron Lauterer]

When a data-pool is configured, use it for status infos. The 'data-pool'
config option is used to mark the erasure coded pool while the 'pool'
will be the replicated pool holding meta data such as the omap.

This means, the 'pool' will only use a small amount of space and people
are interested how much they can store in the erasure coded pool anyway.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
I decided to check for the IMO more likely case first and check against
a configured data-pool. Afterwards check if we actually need to set the
default pool name in case the pool property is omitted in the storage
config.

 PVE/Storage/RBDPlugin.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index fb48c3f..da28e8a 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -630,7 +630,8 @@ sub status {
     my $rados = $librados_connect->($scfg, $storeid);
     my $df = $rados->mon_command({ prefix => 'df', format => 'json' });
 
-    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
+    my $pool = $scfg->{'data-pool'} ? $scfg->{'data-pool'} : $scfg->{pool};
+    $pool = 'rbd' if !$pool;
 
     my ($d) = grep { $_->{name} eq $pool } @{$df->{pools}};
 
-- 
2.30.2

Re: [pve-devel] [PATCH storage] rbd: fix #4060 show data-pool usage when configured

[Dominik Csapak]

On 5/17/22 14:58, Aaron Lauterer wrote:
> When a data-pool is configured, use it for status infos. The 'data-pool'
> config option is used to mark the erasure coded pool while the 'pool'
> will be the replicated pool holding meta data such as the omap.
> 
> This means, the 'pool' will only use a small amount of space and people
> are interested how much they can store in the erasure coded pool anyway.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> I decided to check for the IMO more likely case first and check against
> a configured data-pool. Afterwards check if we actually need to set the
> default pool name in case the pool property is omitted in the storage
> config.
> 
>   PVE/Storage/RBDPlugin.pm | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index fb48c3f..da28e8a 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -630,7 +630,8 @@ sub status {
>       my $rados = $librados_connect->($scfg, $storeid);
>       my $df = $rados->mon_command({ prefix => 'df', format => 'json' });
>   
> -    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
> +    my $pool = $scfg->{'data-pool'} ? $scfg->{'data-pool'} : $scfg->{pool};
> +    $pool = 'rbd' if !$pool;
>   
>       my ($d) = grep { $_->{name} eq $pool } @{$df->{pools}};
>   

without looking more closely at the surrounding code and how we
set/get those properties, wouldn't it also possble to do:

my $pool = $scfg->{'data-pool'} // $scfg->{pool} // 'rbd';

?

Re: [pve-devel] [PATCH storage] rbd: fix #4060 show data-pool usage when configured

[Aaron Lauterer]

On 5/17/22 16:01, Dominik Csapak wrote:
> On 5/17/22 14:58, Aaron Lauterer wrote:
>> When a data-pool is configured, use it for status infos. The 'data-pool'
>> config option is used to mark the erasure coded pool while the 'pool'
>> will be the replicated pool holding meta data such as the omap.
>>
>> This means, the 'pool' will only use a small amount of space and people
>> are interested how much they can store in the erasure coded pool anyway.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>> I decided to check for the IMO more likely case first and check against
>> a configured data-pool. Afterwards check if we actually need to set the
>> default pool name in case the pool property is omitted in the storage
>> config.
>>
>>   PVE/Storage/RBDPlugin.pm | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
>> index fb48c3f..da28e8a 100644
>> --- a/PVE/Storage/RBDPlugin.pm
>> +++ b/PVE/Storage/RBDPlugin.pm
>> @@ -630,7 +630,8 @@ sub status {
>>       my $rados = $librados_connect->($scfg, $storeid);
>>       my $df = $rados->mon_command({ prefix => 'df', format => 'json' });
>> -    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>> +    my $pool = $scfg->{'data-pool'} ? $scfg->{'data-pool'} : $scfg->{pool};
>> +    $pool = 'rbd' if !$pool;
>>       my ($d) = grep { $_->{name} eq $pool } @{$df->{pools}};
> 
> without looking more closely at the surrounding code and how we
> set/get those properties, wouldn't it also possble to do:
> 
> my $pool = $scfg->{'data-pool'} // $scfg->{pool} // 'rbd';
> 
> ?

Should work as well if it is okay stylewise. In the end, we want to get the pool 
name in that order.

> 

Re: [pve-devel] [PATCH storage] rbd: fix #4060 show data-pool usage when configured

[Thomas Lamprecht]

Am 5/17/22 um 16:07 schrieb Aaron Lauterer:
>>> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
>>> index fb48c3f..da28e8a 100644
>>> --- a/PVE/Storage/RBDPlugin.pm
>>> +++ b/PVE/Storage/RBDPlugin.pm
>>> @@ -630,7 +630,8 @@ sub status {
>>>       my $rados = $librados_connect->($scfg, $storeid);
>>>       my $df = $rados->mon_command({ prefix => 'df', format => 'json' });
>>> -    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>>> +    my $pool = $scfg->{'data-pool'} ? $scfg->{'data-pool'} : $scfg->{pool};
>>> +    $pool = 'rbd' if !$pool;
>>>       my ($d) = grep { $_->{name} eq $pool } @{$df->{pools}};
>>
>> without looking more closely at the surrounding code and how we
>> set/get those properties, wouldn't it also possble to do:
>>
>> my $pool = $scfg->{'data-pool'} // $scfg->{pool} // 'rbd';
>>
>> ?
> 
> Should work as well if it is okay stylewise. In the end, we want to get the pool name in that order.

Dominik's proposal would be fine(r) for me style wise.