-
Notifications
You must be signed in to change notification settings - Fork 581
[cephadm] add plugin to collect cephadm config, logs and diagnostic o… #2682
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
base: main
Are you sure you want to change the base?
Conversation
…utput Signed-off-by: Harald Klein <[email protected]>
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
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.
There's a lot of overlap with the existing ceph
plugin - would it not make sense to add the cephadm
commands there, or should we instead be looking at potentially breaking up the ceph
plugin and moving some of the items there into this (and/or other plugins)?
profiles = ('storage') | ||
|
||
packages = ( | ||
'cephadm' | ||
) |
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.
These need to be tuples, so you'll need a trailing ,
even for 1-tuples. E.g.
profiles = ('storage',)
packages = ('cephadm',)
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.
cephadm
is only available in very recent Ceph versions. Hence I was considering a separate plugin. Thanks for the comment on the tuples.
if not all_logs: | ||
self.add_copy_spec([ | ||
"/var/log/ceph/cephadm.log" | ||
]) | ||
else: | ||
self.add_copy_spec([ | ||
"/var/log/ceph/cephadm*" | ||
]) |
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.
This is already captured by the ceph
plugin.
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.
This would only be captured via the ceph
plugin when the ceph-common
package is installed (to trigger the ceph plugin), which might not be the case in all situations (there is no package dependency between cephadm
and ceph-common
. ceph-common
would usually be installed via playbook from cephadm-ansible
).
self.add_copy_spec([ | ||
"/etc/ceph/" | ||
]) |
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.
Likewise, this is captured by the ceph
plugin today.
for s in cephadm_cmds: | ||
self.add_cmd_output(cmds="cephadm --timeout 5 %s" % s, suggest_filename="%s" % s) |
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_cmd_output()
supports a timeout
parameter, so you don't need to use the cephadm
option here or do any filename mangling. self.add_cmd_output(["cephadm %s" % cmd for cmd in cephadm_cmds], timeout=5)
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.
Isn't the "native" timeout of a command preferred? cephadm would give some hints on where the timeout did occur (e.g. if it couldn't acquire a lock).
@haklein were you planning to work on this. I think having |
…utput
Signed-off-by: Harald Klein [email protected]
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines