* [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
@ 2021-07-07 8:47 Dominik Csapak
2021-07-07 10:19 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2021-07-07 8:47 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
2021-07-07 8:47 [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display Dominik Csapak
@ 2021-07-07 10:19 ` Thomas Lamprecht
2021-07-07 11:23 ` Dominik Csapak
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2021-07-07 10:19 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
2021-07-07 10:19 ` Thomas Lamprecht
@ 2021-07-07 11:23 ` Dominik Csapak
2021-07-07 12:24 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2021-07-07 11:23 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
2021-07-07 11:23 ` Dominik Csapak
@ 2021-07-07 12:24 ` Thomas Lamprecht
2021-07-07 12:30 ` Dominik Csapak
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Lamprecht @ 2021-07-07 12:24 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
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).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
2021-07-07 12:24 ` Thomas Lamprecht
@ 2021-07-07 12:30 ` Dominik Csapak
2021-07-07 12:36 ` Thomas Lamprecht
0 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2021-07-07 12:30 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display
2021-07-07 12:30 ` Dominik Csapak
@ 2021-07-07 12:36 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2021-07-07 12:36 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-07 12:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07 8:47 [pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display Dominik Csapak
2021-07-07 10:19 ` Thomas Lamprecht
2021-07-07 11:23 ` Dominik Csapak
2021-07-07 12:24 ` Thomas Lamprecht
2021-07-07 12:30 ` Dominik Csapak
2021-07-07 12:36 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox