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
cephfs-top: addition of sort feature and limit option #48111
Conversation
c4d3176
to
231d61c
Compare
6bd51ce
to
eda5325
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.
Docs 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.
@neesingh-rh I'm playing with this from the last couple of days. Works really well 👍
Really nice work!
eda5325
to
8287369
Compare
@joscollin please review this change. |
@vshankar Looks good at the first look. Will review more tomorrow. |
Yeh, I know that - the sort limit field can be specified when registering the query with OSD. That's supported by the MDS too :) |
@joscollin Is this good to go? |
jenkins test make check |
Will review it again the next working day. |
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.
Some limit menu issues:
- Launch the limit menu, resize the window multiple times, the menu is gone.
- Launch the limit menu, input a number and then resize the window, the menu is gone.
- Launch the limit menu, press any random keys, the menu is gone.
- Why the inputed number is not displayed on the screen?
- Launch the limit menu, pressing 'q' doesn't immediately go back to the previous screen. It needs multiple q presses it seems.
Is there a need for 'Fields Management' menu screen, as it just a redirect to another selection screen? We could do the same from the home page?
Like:
Press 'm' to select a filesystem, 's' for sort, 'l' to limit the clients and and 'q' to quit
@vshankar @gregsfortytwo
Sort is fine, but do we really need a limit as it could be scrolled when #48090 is merged? Looks like once the limit is set, it cannot be reset. We need to exit cephfs-top and launch again.
I think it would be better have something like a 'Reset to default' key at the home screen itself. So that the user can reset to the default home screen view anytime, without going inside the Sort screen and select the 'default'. This new key would reset the current sort & limit setting. |
src/tools/cephfs/top/cephfs-top
Outdated
"rsp= READ_IO_SPEED", "wtio= WRITE_IO_SIZES", "waio= WRITE_AVG_IO_SIZES", | ||
"wsp= WRITE_IO_SPEED", "rlatavg= AVG_READ_LATENCY", "rlatsd= STDEV_READ_LATENCY", | ||
"wlatavg= AVG_WRITE_LATENCY", "wlatsd= STDEV_WRITE_LATENCY", "mlatavg= AVG_METADATA_LATENCY", | ||
"mlatsd= STDEV_METADATA_LATENCY", "Default"] |
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.
Instead of maintaining field_menu
, could it be created from MAIN_WINDOW_TOP_LINE_METRICS
, as we did in refresh_top_line_and_build_coord
? Then it would be easier to maintain the code.
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.
Firstly, I tried using that only for easy maintenance but it wasn't successful. Because currently we are using the field names stored in the field_menu
to match for the field chosen and the display structure of field menu is followed the way it is done in top(1). If I use the MAIN_WINDOW_TOP_LINE_METRICS
, firstly I will have to call its respective function for eg self.items()
and self.avg_items()
again and again.Secondly, MAIN_WINDOW_TOP_LINE_METRICS
have some fields which are not being displayed directly in cephfs-top display as fields, their avg, std is being used but for us, order is very important.That's why, it looked better and easy to do this way.
I still find the limit functionality useful - think of using it with sort and limiting the display to top two clients that are consuming bandwidth. As far as resetting the limit, I think that a bug and should be fixed. |
|
7188102
to
56a554b
Compare
Above issues are resolved, RESIZE was also accepted as input earlier.
Added this functionality.
Fixed, Earlier it was working by pressing Enter immediately after pressing 'q'.
I don't want to remove the Fields Management. It is keeping the things separated and easy to handle, even when some new functionalities being added later.And follows top(1) structure too.
It wasn't the bug earlier, we could set the limit value as many times we want, "d" key was there to reset. Now, I have added BACKSPACE too, to change the limit value during input time and currently I have set the length to digits allowed is FOUR. |
I think its better to have the default "d" key added to FIELDS MANAGEMENT Screen, which will makes the resetting the FIELD MANAGEMENT values only. Its good to have things modularized. I have added "d" key on the FIELDS MANAGEMNT Screen, it will reset the sort as well as limit value to the default. |
+1 , Scroll makes the user to have a view of all the clients easily but if user wants to analyze metrics for some top 50 or 60 clients among thousands of clients, he or she will have an option. |
Removed flake8 errors. Thanks |
56a554b
to
1e4893e
Compare
LGTM. @joscollin Need your approval for merge. |
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.
Looks really nice now. I have a few comments:
- Do we really need a
press "d" for default
option inside thel
menu? As default d is already there in the home screen, the user comes inside only for a custom value. - I think we certainly don't need filter to
0
clients. Please avoid that. - Doc screenshot update?
- In the
l
menu,q
quits only when the field is blank. I think we should enableq
always in-case the user changes his mind. - In the home screen, change to
Press 'd' for default setting
. - The
HELP
is getting bigger every-time. How about shortening it a little bit like:COMMANDS: m - select a filesystem | s - sort menu | l - limit number of clients | d - default setting | q - quit
?
Will check the code tomorrow.
1e4893e
to
80ab185
Compare
Let's keep it as the existing way only. If user wants default value only for limit not for the sort value, can explicitly do so.
Done.
Added
Enabled 'q' while giving input too.
updated
Done
|
Then mention that feature in the doc under |
f6b7c6a
to
dce6c07
Compare
dce6c07
to
b13a3c1
Compare
@neesingh-rh Fix the tox failures too. |
b13a3c1
to
276be14
Compare
This commit intends to add: - sort-by field value feature to cephfs-top. - feature to limit number of clients displayed Fixes: https://tracker.ceph.com/issues/55121 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Fixes: https://tracker.ceph.com/issues/55121 Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
276be14
to
bc360a4
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.
qa doesn't seems necessary (it's just a selftest). Let's go for merge.
vstart_runner test succeed:
|
This PR intends to add two new features to cephfs-top:
Fixes: https://tracker.ceph.com/issues/55121
Signed-off-by: Neeraj Pratap Singh neesingh@redhat.com
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