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: fix 500 error on block device iSCSI status page #20928
Conversation
@dillaman thanks for fixing this! it's very hard for us to test this backend controller (and frontend). Could you also fix the unit test |
@rjfd I have no idea how to run the unit tests for the dashboard since they don't appear to be connected to |
e99b9d7
to
36e3dad
Compare
if data and len(data) > 1: | ||
return (data[-1][1] - data[-2][1]) / float(data[-1][0] - data[-2][0]) | ||
else: | ||
return 0 |
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.
_get_rate
looks very similar to some existing functions:
mgr.dashboard.controllers.cephfs.CephFS#get_rate
mgr.dashboard.controllers.osd.Osd#get_rate
mgr.dashboard.controllers.perf_counters.PerfCounter#_get_rate
Maybe, it would make sense to move get_rate
to mgr.dashboard.services.ceph_service.CephService
refactor the other occurrences?
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.
Please feel free to tackle that -- they used to all share the same function and during the refactor they all moved to duplicate implementations. I don't really have the bandwidth.
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.
@dillaman sure, that refactor should be done in a different PR by us. thanks
@dillaman our unit tests are executed by ctest. |
@sebastian-philipp What's the name of the test? I don't see a |
... never mind, I see your link now. it's just outside the test directory. |
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
36e3dad
to
3cfd3a2
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, thanks for this fix
@rjfd Thanks |
Signed-off-by: Jason Dillaman dillaman@redhat.com