-
Notifications
You must be signed in to change notification settings - Fork 321
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
add additional stats from mdstat #380
Conversation
We need to make sure this works for all kernels we support. If that's the case, LGTM! |
This should gracefully handle different kernels...and I believe the mdstat file format hasn't changed in quite some time. |
Counters added: * excessive_buffer_overrun_errors * local_link_integrity_errors Signed-off-by: Trey Dockendorf <[email protected]> Signed-off-by: John Seekins <[email protected]>
Signed-off-by: John Seekins <[email protected]>
Signed-off-by: John Seekins <[email protected]>
Signed-off-by: John Seekins <[email protected]>
@discordianfish Is there a standard way to prove out the "kernels supported"? I know this works on 4.19, for example. |
Typically it involves a lot of browsing the kernel source tree. :-( To note, we want to keep kernel support all the way back to 2.6.23. |
Unfortunately I don't think we have a way. We should probably collect fixtures for multiple kernel versions. It's silly but I'd probably spin up a VM with e.g 2.6.23 and run tests there.. |
If that's the case...how did y'all validate this collector in the first place? While I appreciate that we should try to validate this as much as possible, it does fail gracefully on missing stats. |
Lots of trial and error. The current mdstat fixture is a collection of various examples from bugs reported by users. |
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.
Fair enough, I think we should extend the fixtures to cover more kernel versions but until then, I think it's fair to support the procfs files as shown in the fixtures. So LGTM.
@SuperQ wdyt?
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.
Yup, adding new features while we maintain backwards compatibility is what I desire.
LGTM
* Add several Infiniband counters Counters added: * excessive_buffer_overrun_errors * local_link_integrity_errors Signed-off-by: Trey Dockendorf <[email protected]> Signed-off-by: John Seekins <[email protected]> * add additional stats from mdstat Signed-off-by: John Seekins <[email protected]> * return successful values every time Signed-off-by: John Seekins <[email protected]> * add count of 'downed' disks Signed-off-by: John Seekins <[email protected]> Co-authored-by: Trey Dockendorf <[email protected]>
* Add several Infiniband counters Counters added: * excessive_buffer_overrun_errors * local_link_integrity_errors Signed-off-by: Trey Dockendorf <[email protected]> Signed-off-by: John Seekins <[email protected]> * add additional stats from mdstat Signed-off-by: John Seekins <[email protected]> * return successful values every time Signed-off-by: John Seekins <[email protected]> * add count of 'downed' disks Signed-off-by: John Seekins <[email protected]> Co-authored-by: Trey Dockendorf <[email protected]>
In the recovery line for mdstat, we can also track percentage complete, estimated time to completion, and current recovery write speed. This MR adds those additional stats.