-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
mgr/dashboard: add inventory card and single stat cards to rgw overview dashboard #52317
Conversation
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.
Thanks @aaSharma14, I haven't tested it locally. Will do it and give more reviews here
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
...rd/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.spec.ts
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.html
Outdated
Show resolved
Hide resolved
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
b5a7436
to
427d800
Compare
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.
few nit picks
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/core/navigation/navigation/navigation.component.html
Outdated
Show resolved
Hide resolved
total_objects = total_objects + pool['stats']['objects'] | ||
total_pool_bytes_used = total_pool_bytes_used + pool['stats']['bytes_used'] |
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.
we should review if this is correct with rgw folks.
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.
Yeah I will confirm this with them
427d800
to
d93e4b2
Compare
unit-test failure
|
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
Check if Avg object size is available from backend. If not we can remove the till or comment it for now. |
8937b65
to
f2ad547
Compare
1884beb
to
3248576
Compare
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.
LGTM!
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.
Given the new landing page (v3) no longer uses cards in this way, why not adopting that approach for depicting this info too:
- Inventory card: could show the count and status of all the RGW items:
- Daemons
- Realms
- Zonegroups
- Zones
- Users
- Buckets
- Objects
- Daemons
- Capacity: a pie-chart like the one used for the main landing page Capacity.
The only piece of info missing from this proposal would be the Avg. Object Size, but my question here is: how's this info actionable? Are there any recommendations about an ideal avg. object size? If there's some threshold/s under/beyond what the avg. object size should be kept, then we could use alerts/health check warnings for that purpose?
private rgwBucketService: RgwBucketService, | ||
private rgwUserService: RgwUserService | ||
) { | ||
this.permissions = this.authStorageService.getPermissions(); |
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.
If I recall it correctly, @nSedrickm created long time ago an Angular directive to avoid having to deal directly with the Permissions (it'd be desirable to have a similar thing for feature toggles, or reuse the cdScope
, given both authorization/RBAC and feature-toggles rely on mapping HTML/DOM to specific features/scope), which allowed to simply tag HTML elements with specific permissions/feature toggles.
<div cdScope='"rgw"'>
...
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.
@aaSharma14 can you add *cdScope
to the first div
. Then we can remove the permissions thing.
Thanks @epuertat , We initially went with a design similar to the dashboard v3 landing page , a sample design for that approach was something like this - The issues that we identified with this approach were -
We also have this other PR - #52405, which adds some graphs to the overview page, the overall view looks like this for now - The space left below is to show the multi-site sync status, for which we are planning to use a design similar to this - |
@epuertat I agree we thought of going with the inventory approach and discussed it with the UX team as well. Things were not fitting well other details we wanted for us with inventory card. Also we had timing constraints we have to choose for the approach and start the implementation. As lot of the data from the cluster dashboard is not present in RGW dashboard. We went with this for now and maybe based on feedback we can adapt. WDYT? |
That's great! I didn't know about this design.
First of all, there's no need to replicate exactly the Landing Page Dashboard. We can simply pick the widgets that are required. That said, even if there's no "native" concept of overall status in RGW, we can infer that. What does it mean that everything is OK? E.g.: All services up & running. The idea of having a single indicator of a service status is to provide a single place at which operators should pay attention.
Same: we can remove if if not required.
You can still display:
It doesn't matter whether they provide "status", what we want to depict is whether everything is ok or not: are all daemons up? are all realms/zonegroups/zones up? ...
I already reviewed this one too, and my suggestion is the same: to deliver the existing chart widget. |
As I replied to @aaSharma14 there would be no need to replicate the same exact dashboard, but if the information can fit in an existing widget (like the inventory), I think it's good to leverage that. In any case, this can be implemented incrementally. |
jenkins test make check |
Signed-off-by: Aashish Sharma <aasharma@redhat.com>
3248576
to
e3ed882
Compare
jenkins test make check |
e3ed882
to
224746c
Compare
jenkins test dashboard cephadm |
224746c
to
803602d
Compare
Thanks @cloudbehl , changed Daemons -> Gateways. We are not hiding the object count if its 0..we are hiding it if it is null |
If its null we can assume its 0. As there are not objects that's why it returns null. We should not hide it |
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.
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.html
Outdated
Show resolved
Hide resolved
...board/frontend/src/app/ceph/rgw/rgw-overview-dashboard/rgw-overview-dashboard.component.scss
Outdated
Show resolved
Hide resolved
private rgwBucketService: RgwBucketService, | ||
private rgwUserService: RgwUserService | ||
) { | ||
this.permissions = this.authStorageService.getPermissions(); |
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.
@aaSharma14 can you add *cdScope
to the first div
. Then we can remove the permissions thing.
9f7112f
to
0b84f31
Compare
overview dashboard Signed-off-by: Aashish Sharma <aasharma@redhat.com>
0b84f31
to
6a21d60
Compare
jenkins test dashboard |
jenkins test windows |
jenkins test dashboard |
now its using row-col. 1 row 3 col |
.pb-5 { | ||
padding-bottom: 3.5rem !important; | ||
} |
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.
is this necessary? this could break the layout in some views. You can test it out by resizing your window.
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, this is necessary..tested with resizing the window..works fine
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.
mostly looks good! just minor comment on layout!
Add Daemons, Zoning, Buckets, Users, Used capacity(capacity used by all the pools in the cluster), Avg object Size(yet to be implemented) cards to the rgw overview dashboard.
Fixes: https://tracker.ceph.com/issues/62258
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows