-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat(ui): Requests info for Cpu and Mem added to Pod details #20637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ui): Requests info for Cpu and Mem added to Pod details #20637
Conversation
Signed-off-by: Surajyadav <[email protected]>
❗ Preview Environment undeploy from Bunnyshell failedSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: Surajyadav <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20637 +/- ##
=======================================
Coverage 60.20% 60.21%
=======================================
Files 347 347
Lines 59274 59282 +8
=======================================
+ Hits 35688 35694 +6
- Misses 20722 20726 +4
+ Partials 2864 2862 -2 ☔ View full report in Codecov by Sentry. |
andrii-korotkov-verkada
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT, have some minor comments
ui/src/app/applications/components/application-pod-view/pod-tooltip.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-pod-view/pod-view.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-pod-view/pod-view.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-pod-view/pod-view.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
ui/src/app/applications/components/application-pod-view/pod-view.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Suraj yadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
bfac740 to
23198a4
Compare
agaudreault
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@surajyadav1108 I do like the implementation of 1), but the "Request?" box seems very weird and out of place. I dont really see the necessity to have this information if we are not in the node view. cpu/mem seems more like advanced configuration that you will go look at the resource details/manifest for.
|
@agaudreault ok we can have just 1st implementation of rows in tooltip then. |
Signed-off-by: Suraj yadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
|
Removed Requests box... you can merge this now @agaudreault improved-pod-resource-view.mp4 |
| <div style={{display: 'flex', alignItems: 'center'}}> | ||
| <div className='pod-view__node__container--header'> | ||
| <div | ||
| style={{display: 'flex', alignItems: 'center', ...(group.kind !== 'node' && {cursor: 'pointer'})}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As seen on the recording, the tooltip style seems broken since the list items are truncated on the pod that is "Progressing". It may be because the size of the tooltip is set based on the pod name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the tooltip is set based on the pod name or on content if its too short. the intention was just to move the onClick to the header title instead of the entire pod header container. I’ve now reverted it back to how it was.
no truncation anymore since I reduced the label length for CPU and memory requests.
ui/src/app/applications/components/application-pod-view/pod-tooltip.tsx
Outdated
Show resolved
Hide resolved
controller/cache/info.go
Outdated
|
|
||
| // requests will be released for terminated pods either with success or failed state termination. | ||
|
|
||
| if reason != "Completed" && reason != "Error" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the code above, it is very likely that the reason string could be set to a more "detailed" value, such as ExitCode:%s. Instead of basing the condition on the string, use the bool variable available above such as initializing and hasRunning, or add a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, used !isPodPhaseTerminal no need to use these initializing hasRunning as these are requests. It's also helpful for the user to see when pods aren't scheduled.
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
Signed-off-by: Surajyadav <[email protected]>
|
@agaudreault this is how it looks now pod-cpu-mem-refactor.mp4 |
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]> Signed-off-by: enneitex <[email protected]>
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]>
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]> Signed-off-by: Mangaal <[email protected]>
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]>
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]>
…j#20637) Signed-off-by: Surajyadav <[email protected]> Signed-off-by: Suraj yadav <[email protected]>
Resolves issue: #19457
1. Added the pod requests info to the Pod tooltip.
This will only show if the pod currently has some reserved requests (running pods with allocated requests).
When the pod completes successfully or fails with an error, the requests will be shown as 0 since they are no longer reserved.
(also if the resources aren't declared )
2. For the original request of adding the top/parent resource groups,
Added tests in info.go f and utils.test for scenerios of completed and failed state with allocated resources.
whole-pod-view-test.mp4
Checklist: