-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix: modernize Ceph input plugin metrics #10797
Conversation
Updated the ceph outputs based on the current (v16) json data structures Made the tests validate against that data.
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.
Great you are willing to modernise this input, but I have some doubts about backwards compatibility.
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
Versions of Ceph <15 used to have their OSD maps nested, but newer ones don't. This change aims to capture both situations properly.
Removed uneeded comments Remove fields that will never exist update tests
Hi Greg, could you rebase this on current master? CI isn't running on this branch. Rebasing will pull in the new circle ci config and should trigger CI to run again. |
Thanks for merging master to get CI working. I did a quick review of the PR and it's great that you've made it compatible with new versions of ceph. I'm concerned that it may break compatibility with older versions. Do you know if the PR will work with older versions? It looks like the changes to the unit tests affect both input json and expected metrics. Could we leave the old test cases as they were to show the plugin still works when run with older ceph versions, and add new test cases to cover the new json format? |
Hi @G-regL, were you able to look into compatibility with older versions of Ceph? |
I hadn't looked at it since your previous comment, but in looking at it now, there's two unit tests, one which covers the old data for Cluster Status; Was there something more specific you were looking for cover? |
Looking at Should this change have been a new test case? Thanks - trying to ensure we aren't suddenly breaking existing users as Dave pointed out. |
Given version 16 of Ceph is currently the oldest version of ceph now supported, I'm going to retract my questions about tests. @G-regL - if you could update the PR with some of our recent changes to the sample config and build tags we can look to getting this added. |
I can certainly update the sample config, but I don't quite know what you mean with regards to the |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
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
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 good to me. Thanks for all your effort @G-regL!
(cherry picked from commit 21a7d6f)
Required for all PRs:
resolves #10788
The Ceph input plugin was likely written against version 11 or 12 of Ceph, but since then there's been some changes to the data structures that the respective
ceph
commands use for cluster-level statistics.This PR aims to bring those values back inline with at least version 15 of Ceph.