-
Notifications
You must be signed in to change notification settings - Fork 541
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
[ceph] Ceph commands output enhancement #1726
Conversation
I have added two The reason behind this change is that automation tools could parse |
Why the first commit referencing to #1 ? :) For the commands you take twice (once in human-readable format, once in json): isnt json format sufficient for humans as well? Or how much additional time these extra commands execution could take? (I won't like much duplicated info collected that takes too long time). |
Seems it took the merge commit of my fork.
I am getting ceph cluster and will give you the rough estimates. One min |
|
To me seems the JSON are faster then regular ones:
|
I will update the MR and replace the old ones with JSON, its faster, better usage with automation tools as well it's still readable. |
We are adding 4 new commands to ceph plugin as well output will be in JSON format that allows everyone for easier integration with automation tool. The execution time raise roughly by 1s
CHANGE FROM
TO
|
ACK to the PR. |
hey @zerodayz, this PR would be helpful from Insights perspective. However, from a support engineer point of view having these command outputs in 'json-pretty' format will make troubleshooting very difficult. Please close this PR or keep the original commands and append the new commands with 'json-pretty' format. |
How difficult is to read the json format for humans? (I guess it is very subjective) Am I right that keeping also the non-json commands will increase plugin execution time by approx. 6 seconds? (I am trying to assess pros and cons - how much it will hurt customers and support if the plugin will always take extra 6s and how much will we gain by the extra-human-convenient format collected) |
General ack from me on the PR, but it needs to drop the leading merge commit. We don't allow merge commits in PRs to keep the tree clean. In regards to @ashishkumsingh's request, I tend to agree about json-formatting being slightly more difficult for human eyes to parse, and I don't think an extras 6 seconds of execution time is killer for us. @zerodayz I'd recommend keeping the current collection and then making a secondary pass through all those commands with logic that appends the self.add_cmd_output([
"%s --format json-pretty" % s for s in ceph_cmds
], subdir="json_output") |
BEFORE CHANGE:
AFTER CHANGE:
|
03b9048
to
7f700ea
Compare
@ashishkumsingh Do you agree with the last PR? It keeps the old commands in place and adds the new ones in json_output directory. |
@ashishkumsingh What about now? |
@zerodayz LGTM |
@TurboTurtle Seems fine now, can we merge the PR please? |
It looks fine to me, however merges are done by @bmr-cymru who is on leave at the moment. |
@TurboTurtle is @bmr-cymru still on leave? Thanks! |
@zerodayz can you rebase this PR ? |
f800826
to
9962ee3
Compare
Prior to this patch, ovn_host was disabled on containerized setups due to the fact that ovn-controller package is not installed in the host. This patch fixes it by checking if the ovn-controller process is running. Resolves: sosreport#1767 Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
This pull request introduces 1 alert when merging 9962ee3 into a895bf4 - view on LGTM.com new alerts:
|
9962ee3
to
eb52c26
Compare
This pull request introduces 1 alert when merging eb52c26 into a895bf4 - view on LGTM.com new alerts:
|
"ceph osd crush show-tunables", | ||
"ceph-disk list", | ||
"ceph versions", | ||
"ceph osd crush dump" | ||
"ceph -v" |
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.
Add a coma to the end of previous line (that's the "implicit concatenation" failure of LGTM - currently a command "ceph osd crush dumpceph -v"
would be executed).
"fs ls", | ||
"fs dump", | ||
"pg dump", | ||
] |
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.
Why some ceph *
commands are taken from this list while others are executed "directly" (cf. L58-62 and L82)? Just because the later ones are taken with json format as well? (maybe renaming the ceph_cmds
to something more obvious will help code clarity)
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.
Because those commands share both regular and json output
ceph_cmds = [
"status",
"health detail",
"osd tree",
"osd stat",
"osd df tree",
"osd dump",
"osd df",
"mon dump",
"df",
"df detail",
"fs ls",
"fs dump",
"pg dump",
]
Regular output
self.add_cmd_output([
"ceph %s" % s for s in ceph_cmds
])
As well the same command can be re-used and collect json-pretty
self.add_cmd_output([
"ceph %s --format json-pretty" % s for s in ceph_cmds
], subdir="json_output")
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.
That output is using same commands from ceph_cmds, they are shared and just subdirectory is different, this is to avoid duplicated code.
Generally ACK up to the missing coma. |
@pmoravec I am rebasing on master and always get this change in way: sos/plugins/ovn_host.py Not sure what I can do to drop sos/plugins/ovn_host.py off the change. |
eb52c26
to
31958c2
Compare
Add json-pretty output for most of the useful commands for easier consumption by Automation tools Signed-off-by: Robin Cernin rcerninr@redhat.com
31958c2
to
6877fe1
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.
I guess you forgot to update your master branch of your fork (that you forked before the a895bf4 commit with ovn_host
)?
Anyway we can merge just your commit.
Add json-pretty output for most of the useful commands
for easier consumption by Automation tools
Signed-off-by: Robin Cernin rcerninr@redhat.com
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines