Enhance block pool mirroring status card to cover rados namespace #1666
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GowthamShanmugam The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
GowthamShanmugam
force-pushed
the
RHSTOR-6578
branch
from
de38eba
to
a1247df
Compare
|
/hold need UX approval |
GowthamShanmugam
force-pushed
the
RHSTOR-6578
branch
2 times, most recently
from
f0c0509
to
26371a1
Compare
|
|
LGTM. @SanjalKatiyar & @bipuladh for tagging. |
GowthamShanmugam
force-pushed
the
RHSTOR-6578
branch
from
26371a1
to
977b558
Compare
| className={classNames( | ||
| 'odf-block-pool__mirroring-card-item-dd', | ||
| valueClassName | ||
| !isHidden && ( |
There was a problem hiding this comment.
why not hide entire MirroringCardItem on the caller's side ?? instead of hiding returned JSX.
There was a problem hiding this comment.
My initial plan to hide the entire MirroringCardItem render but as per this comment, I moved the hiding part inside this component: #1666 (comment)
I am ok with both the approach
There was a problem hiding this comment.
what I understood from the comment is that for: !_.isEmpty(states) && isLoaded && !isLoadError, why to use "isLoaded" and "isLoadError" outside when we can pass it as a prop to MirroringCardItem, which internally handles it.
There was a problem hiding this comment.
but anyway, I am okay with current changes as well...
There was a problem hiding this comment.
oh ok! my bad
GowthamShanmugam
force-pushed
the
RHSTOR-6578
branch
2 times, most recently
from
a076d02
to
090c2a8
Compare
| const healthStateMap = healthStateMapping(t); | ||
| let mirroringHealth: HealthMappingValues = | ||
| healthStateMap[poolObj.status?.mirroringStatus?.summary?.health]; | ||
| let imageHealth: HealthMappingValues = |
There was a problem hiding this comment.
I saw a comment below @ToDo: Fetch aggregated image health from metrics: https://issues.redhat.com/browse/RHSTOR-6579, but here you are using CR. Which one we need to use CR or metrics ??
There was a problem hiding this comment.
For the last page, I want mirror image health for all block pools. If I read from CR, I will end up listening to all radio namespaces. To improve my performance I want to replace the CR read with metrics on list page.
Mirroring card under dashboard is for a specific pool, I am adding the watchers agianst the rados namespace using pool name label. Also, here I want the image state information, so I have to use CR.
| t: TFunction | ||
| ): HealthMappingValues[] => { | ||
| const healthStateMap = healthStateMapping(t); | ||
| let mirroringHealth: HealthMappingValues = |
There was a problem hiding this comment.
same here... we need to rely on CR or metrics ??
| const lastChecked: string = obj.status?.mirroringStatus?.lastChecked; | ||
| const formatedDateTime = lastChecked | ||
| const [mirroringHealth, imageHealth]: HealthMappingValues[] = | ||
| aggregateMirroringHealth(obj, radosNamespaces, t); |
There was a problem hiding this comment.
Function is called aggregateMirroringHealth, but u r aggregating image health too... either create 2 different functions or rename current one IMO.
There was a problem hiding this comment.
mirroring_image health is a subset of mirroring health
Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
GowthamShanmugam
force-pushed
the
RHSTOR-6578
branch
from
090c2a8
to
4429a4f
Compare
Issue: https://issues.redhat.com/browse/RHSTOR-6578
ToDo: The overall_image_health column has to be calculated using rados image health metrics. Fetching it from the CR has some performance impact.
Issue to track metrics change: https://issues.redhat.com/browse/RHSTOR-6579