Skip to content
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: crushmap tree doesn't display crush type other than root #41758

Merged
merged 1 commit into from Jun 18, 2021

Conversation

avanthakkar
Copy link
Contributor

@avanthakkar avanthakkar commented Jun 8, 2021

Fixes: https://tracker.ceph.com/issues/50971
Signed-off-by: Avan Thakkar athakkar@redhat.com

Dashboard resembles o/p of ceph osd tree:

Screenshot from 2021-06-08 19-22-11

# ceph osd tree

ID  CLASS  WEIGHT   TYPE NAME         STATUS  REWEIGHT  PRI-AFF
-8         0.09859  row a                                      
-7         0.09859      rack 2                                 
 2    hdd  0.09859          osd.2         up   1.00000  1.00000
-5         0.09859  datacenter site1                           
 1    hdd  0.09859      osd.1             up   1.00000  1.00000
-1         0.09859  root default                               
-3         0.09859      host ceph                              
 0    hdd  0.09859          osd.0         up   1.00000  1.00000

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@avanthakkar avanthakkar requested review from a team, pereman2 and Waadkh7 and removed request for a team June 9, 2021 13:04
@avanthakkar avanthakkar added this to In progress in Dashboard via automation Jun 9, 2021
@avanthakkar avanthakkar marked this pull request as ready for review June 9, 2021 13:04
@avanthakkar avanthakkar requested a review from a team June 9, 2021 13:04
@avanthakkar avanthakkar requested a review from a team as a code owner June 9, 2021 13:04
@avanthakkar avanthakkar requested review from callithea and removed request for a team June 9, 2021 13:04
Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think we should do 2 things:

  • Rename the front-end component (or at least the visible title) to Crush Tree/Structure or Location (the CRUSH map itself contains other data)
  • Provide a back-end endpoint that generates the information ready to be used directly by the UI, and not processed in any way in the client-side.

Comment on lines 60 to 64
return {
'names': [r['rule_name'] for r in mgr.get('osd_map_crush')['rules']],
'nodes': mgr.get('osd_map_tree')['nodes']
'nodes': mgr.get('osd_map_tree')['nodes'],
'roots': crush.find_roots()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to keep this crush_rule endpoint as a legacy one ok, but I'd suggest creating a new endpoint called crush_tree and expose the information as it's going to be consumed by the front-end: that is, in tree-like form (what we display in the Dashboard is not the CRUSH map or CRUSH rules, but the CRUSH tree).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd have more sense to create a separate endpoint if it can somehow returh the tree-like form as u mentioned, but that can't be the case as I pointed out here #41758 (comment) . The info provided by backend still remains the same i.e. nodes and roots of different trees. And on the other hand if we create a endpoint crush_tree with this same info we would need to introduce the same in frontend (creating crushTree service kinda duplicate of crushRule, etc..). Also on the other hand it's just we aren't exposing more info about crush map in UI, but that can be done as future improvement (tracker already exist https://tracker.ceph.com/issues/47483) and we can for now just keep the name of component and the endpoint as it is?

@avanthakkar
Copy link
Contributor Author

Looks good, but I think we should do 2 things:

* Rename the front-end component (or at least the visible title) to [Crush Tree/Structure or Location](https://docs.ceph.com/en/latest/rados/operations/crush-map/#crush-structure) (the CRUSH map itself contains other data)

Yes, we may expose a separate endpoint crush_tree and also change the frontend component name, but we don't just show the tree view right we can also see some details about a selected node like crush weight, etc..

* Provide a back-end endpoint that generates the information ready to be used directly by the UI, and not processed in any way in the client-side.

Well the thing is anyway we need to construct tree in the frontend from the info provided by the backend : root nodes, children,etc.. And most of the data processing is for the same, about constructing the tree from top to bottom. So whatever changes we make for the endpoint I don't think we can improve much the data processing in frontend. But will look more if possible.

@avanthakkar avanthakkar requested review from aaSharma14 and removed request for callithea June 17, 2021 07:06
Dashboard automation moved this from In progress to Reviewer approved Jun 17, 2021
Copy link
Contributor

@aaSharma14 aaSharma14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @avanthakkar

Copy link
Contributor

@pereman2 pereman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working fine on my machine 👍

@epuertat epuertat moved this from Reviewer approved to Ready-to-merge in Dashboard Jun 18, 2021
Dashboard automation moved this from Ready-to-merge to Reviewer approved Jun 18, 2021
@epuertat epuertat merged commit 6676352 into ceph:master Jun 18, 2021
Dashboard automation moved this from Reviewer approved to Done Jun 18, 2021
@epuertat epuertat deleted the support-multiple-crush-trees branch June 18, 2021 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Dashboard
  
Done
5 participants