* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.