[pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Dominik Csapak]

we incorrectly used 'total' as 100% of the to recovered objects here,
but that contains the total number of *bytes*.

rename 'toRecover' to better reflect its meaning and use that as
100% of the objects.

reported by a user:
https://forum.proxmox.com/threads/bug-ceph-recovery-bar-not-showing-percentage.91782/

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/manager6/ceph/Status.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
index e92c698b..52563605 100644
--- a/www/manager6/ceph/Status.js
+++ b/www/manager6/ceph/Status.js
@@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
 	let unhealthy = degraded + unfound + misplaced;
 	// update recovery
 	if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
-	    let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
-	    if (toRecover === 0) {
+	    let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
+	    if (totalRecovery === 0) {
 		return; // FIXME: unexpected return and leaves things possible visible when it shouldn't?
 	    }
-	    let recovered = toRecover - unhealthy || 0;
+	    let recovered = totalRecovery - unhealthy || 0;
 	    let speed = pgmap.recovering_bytes_per_sec || 0;
 
-	    let recoveryRatio = recovered / total;
+	    let recoveryRatio = recovered / totalRecovery;
 	    let txt = `${(recoveryRatio * 100).toFixed(2)}%`;
 	    if (speed > 0) {
 		let obj_per_sec = speed / (4 * 1024 * 1024); // 4 MiB per Object
-- 
2.30.2

Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Thomas Lamprecht]

On 07.07.21 10:47, Dominik Csapak wrote:
> we incorrectly used 'total' as 100% of the to recovered objects here,
> but that contains the total number of *bytes*.
> 
> rename 'toRecover' to better reflect its meaning and use that as
> 100% of the objects.
> 
> reported by a user:
> https://forum.proxmox.com/threads/bug-ceph-recovery-bar-not-showing-percentage.91782/
> 

please note if this would need to be backported too.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/ceph/Status.js | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
> index e92c698b..52563605 100644
> --- a/www/manager6/ceph/Status.js
> +++ b/www/manager6/ceph/Status.js
> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>  	let unhealthy = degraded + unfound + misplaced;
>  	// update recovery
>  	if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
> -	    let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
> -	    if (toRecover === 0) {
> +	    let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;

why change the variable name, `toRecover` was still OK? Or at least I do not see
any improvement in making it easier to understand with `totalRecovery` if byte vs.
objects where a issue of confusion why not addressing that by using `toRecoverObjects`
or the like

Also, why not adding those metrics up? If, misplaced and unfound do not have any
overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
about overlap with the other two from top of my head though.

> +	    if (totalRecovery === 0) {
>  		return; // FIXME: unexpected return and leaves things possible visible when it shouldn't?
>  	    }
> -	    let recovered = toRecover - unhealthy || 0;
> +	    let recovered = totalRecovery - unhealthy || 0;
>  	    let speed = pgmap.recovering_bytes_per_sec || 0;
>  
> -	    let recoveryRatio = recovered / total;
> +	    let recoveryRatio = recovered / totalRecovery;
>  	    let txt = `${(recoveryRatio * 100).toFixed(2)}%`;
>  	    if (speed > 0) {
>  		let obj_per_sec = speed / (4 * 1024 * 1024); // 4 MiB per Object
> 

Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Dominik Csapak]

On 7/7/21 12:19 PM, Thomas Lamprecht wrote:
> On 07.07.21 10:47, Dominik Csapak wrote:
>> we incorrectly used 'total' as 100% of the to recovered objects here,
>> but that contains the total number of *bytes*.
>>
>> rename 'toRecover' to better reflect its meaning and use that as
>> 100% of the objects.
>>
>> reported by a user:
>> https://forum.proxmox.com/threads/bug-ceph-recovery-bar-not-showing-percentage.91782/
>>
> 
> please note if this would need to be backported too.

yes, i think this would be good to backport

> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   www/manager6/ceph/Status.js | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>> index e92c698b..52563605 100644
>> --- a/www/manager6/ceph/Status.js
>> +++ b/www/manager6/ceph/Status.js
>> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>>   	let unhealthy = degraded + unfound + misplaced;
>>   	// update recovery
>>   	if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
>> -	    let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>> -	    if (toRecover === 0) {
>> +	    let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
> 
> why change the variable name, `toRecover` was still OK? Or at least I do not see
> any improvement in making it easier to understand with `totalRecovery` if byte vs.
> objects where a issue of confusion why not addressing that by using `toRecoverObjects`
> or the like
i read the code and thought 'toRecover' means objects that need 
recovery, but it is not. {misplaced,unfound,degraded}_total each contain
the total number of objects taking part in the recovery
(also the ones that are not unhealthy)

maybe 'totalRecoveryObjects' would make more sense ?

> 
> Also, why not adding those metrics up? If, misplaced and unfound do not have any
> overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
> about overlap with the other two from top of my head though.

they contain all the same number
src/mon/PGMap.cc:{467,482,498} pool_sum.stats.sum.num_object_copies

but are only given if the respective category has objects that need recovery

> 
>> +	    if (totalRecovery === 0) {
>>   		return; // FIXME: unexpected return and leaves things possible visible when it shouldn't?
>>   	    }
>> -	    let recovered = toRecover - unhealthy || 0;
>> +	    let recovered = totalRecovery - unhealthy || 0;
>>   	    let speed = pgmap.recovering_bytes_per_sec || 0;
>>   
>> -	    let recoveryRatio = recovered / total;
>> +	    let recoveryRatio = recovered / totalRecovery;
>>   	    let txt = `${(recoveryRatio * 100).toFixed(2)}%`;
>>   	    if (speed > 0) {
>>   		let obj_per_sec = speed / (4 * 1024 * 1024); // 4 MiB per Object
>>
> 

Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Thomas Lamprecht]

On 07.07.21 13:23, Dominik Csapak wrote:
> On 7/7/21 12:19 PM, Thomas Lamprecht wrote:
>> On 07.07.21 10:47, Dominik Csapak wrote:
>>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>>> index e92c698b..52563605 100644
>>> --- a/www/manager6/ceph/Status.js
>>> +++ b/www/manager6/ceph/Status.js
>>> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>>>       let unhealthy = degraded + unfound + misplaced;
>>>       // update recovery
>>>       if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
>>> -        let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>> -        if (toRecover === 0) {
>>> +        let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>
>> why change the variable name, `toRecover` was still OK? Or at least I do not see
>> any improvement in making it easier to understand with `totalRecovery` if byte vs.
>> objects where a issue of confusion why not addressing that by using `toRecoverObjects`
>> or the like
> i read the code and thought 'toRecover' means objects that need recovery, but it is not. {misplaced,unfound,degraded}_total each contain
> the total number of objects taking part in the recovery
> (also the ones that are not unhealthy)
> 
> maybe 'totalRecoveryObjects' would make more sense ?

totalRecoveryObjects and toRecoverObjects are so similar that they do not really
convey the difference to me for the confusion you had for any other reader, for that
I'd rather add a short comment, those tend to be a bit more explicit for subtle stuff.

> 
>>
>> Also, why not adding those metrics up? If, misplaced and unfound do not have any
>> overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
>> about overlap with the other two from top of my head though.
> 
> they contain all the same number
> src/mon/PGMap.cc:{467,482,498} pool_sum.stats.sum.num_object_copies

ah yeah true, I remember now again. Do you also know where this is actually
set (computed).

Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Dominik Csapak]

On 7/7/21 2:24 PM, Thomas Lamprecht wrote:
> On 07.07.21 13:23, Dominik Csapak wrote:
>> On 7/7/21 12:19 PM, Thomas Lamprecht wrote:
>>> On 07.07.21 10:47, Dominik Csapak wrote:
>>>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>>>> index e92c698b..52563605 100644
>>>> --- a/www/manager6/ceph/Status.js
>>>> +++ b/www/manager6/ceph/Status.js
>>>> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>>>>        let unhealthy = degraded + unfound + misplaced;
>>>>        // update recovery
>>>>        if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
>>>> -        let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>> -        if (toRecover === 0) {
>>>> +        let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>
>>> why change the variable name, `toRecover` was still OK? Or at least I do not see
>>> any improvement in making it easier to understand with `totalRecovery` if byte vs.
>>> objects where a issue of confusion why not addressing that by using `toRecoverObjects`
>>> or the like
>> i read the code and thought 'toRecover' means objects that need recovery, but it is not. {misplaced,unfound,degraded}_total each contain
>> the total number of objects taking part in the recovery
>> (also the ones that are not unhealthy)
>>
>> maybe 'totalRecoveryObjects' would make more sense ?
> 
> totalRecoveryObjects and toRecoverObjects are so similar that they do not really
> convey the difference to me for the confusion you had for any other reader, for that
> I'd rather add a short comment, those tend to be a bit more explicit for subtle stuff.

ok i'll leave it at 'toRecover' and add a comment what it is in my v2
> 
>>
>>>
>>> Also, why not adding those metrics up? If, misplaced and unfound do not have any
>>> overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
>>> about overlap with the other two from top of my head though.
>>
>> they contain all the same number
>> src/mon/PGMap.cc:{467,482,498} pool_sum.stats.sum.num_object_copies
> 
> ah yeah true, I remember now again. Do you also know where this is actually
> set (computed).
> 

no sadly, i tried to check, but i am not so deep into ceph code right now

Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Thomas Lamprecht]

On 07.07.21 14:30, Dominik Csapak wrote:
> On 7/7/21 2:24 PM, Thomas Lamprecht wrote:
>> On 07.07.21 13:23, Dominik Csapak wrote:
>>> On 7/7/21 12:19 PM, Thomas Lamprecht wrote:
>>>> On 07.07.21 10:47, Dominik Csapak wrote:
>>>>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>>>>> index e92c698b..52563605 100644
>>>>> --- a/www/manager6/ceph/Status.js
>>>>> +++ b/www/manager6/ceph/Status.js
>>>>> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>>>>>        let unhealthy = degraded + unfound + misplaced;
>>>>>        // update recovery
>>>>>        if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
>>>>> -        let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>>> -        if (toRecover === 0) {
>>>>> +        let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>>
>>>> why change the variable name, `toRecover` was still OK? Or at least I do not see
>>>> any improvement in making it easier to understand with `totalRecovery` if byte vs.
>>>> objects where a issue of confusion why not addressing that by using `toRecoverObjects`
>>>> or the like
>>> i read the code and thought 'toRecover' means objects that need recovery, but it is not. {misplaced,unfound,degraded}_total each contain
>>> the total number of objects taking part in the recovery
>>> (also the ones that are not unhealthy)
>>>
>>> maybe 'totalRecoveryObjects' would make more sense ?
>>
>> totalRecoveryObjects and toRecoverObjects are so similar that they do not really
>> convey the difference to me for the confusion you had for any other reader, for that
>> I'd rather add a short comment, those tend to be a bit more explicit for subtle stuff.
> 
> ok i'll leave it at 'toRecover' and add a comment what it is in my v2

Adding objects is fine to me though, the basic unit, i.e., size vs. bytes here,
is something that can be encoded in the variable name for dynamic typed languages
like JS - but no hard feelings.

>>
>>>
>>>>
>>>> Also, why not adding those metrics up? If, misplaced and unfound do not have any
>>>> overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
>>>> about overlap with the other two from top of my head though.
>>>
>>> they contain all the same number
>>> src/mon/PGMap.cc:{467,482,498} pool_sum.stats.sum.num_object_copies
>>
>> ah yeah true, I remember now again. Do you also know where this is actually
>> set (computed).
>>
> 
> no sadly, i tried to check, but i am not so deep into ceph code right now

ok, thanks nonetheless.