-
Notifications
You must be signed in to change notification settings - Fork 602
feat: add Charmed Kafka plugins #4214
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?
feat: add Charmed Kafka plugins #4214
Conversation
2d8043b to
275b5d8
Compare
|
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. |
59ad158 to
bf38433
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.
other than the review comments, a few other things
- There is no trigger for these plugins, which means these plugins will always run on any Ubuntu platform. Please add packages, services, files tuples depending on the plugin, so that the plugins are only enabled as required. This probably the reason why the CI has failed on only on the Ubuntu runs.
- The 2 options imho don't add value here. Typically sos is collected for logs and data from a date to the current date. Also collecting logs by default from 1970-01-01 doesn't make sense. This would mean you're collecting all the data by default. It would be preferred, that this was behind the all_logs option. You can look at the maas plugin which parses the since option, which can then allow you to utilise this. So I would work this in this way.
| self.add_cmd_output( | ||
| "snap logs charmed-kafka.cruise-control -n 100000", | ||
| suggest_filename="snap-logs", | ||
| ) |
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.
would it be worth using the all_logs option to get all logs, and then have a default of 10000 lines?
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.
again same comment as below
| self.add_cmd_output( | ||
| "snap logs charmed-kafka.daemon -n 100000", | ||
| suggest_filename="snap-logs", | ||
| ) |
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.
same comment here around all_logs, and maybe have a conditional here for that. There are examples of this in other plugins if you wanted to look
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.
same comment I made below
| self.add_cmd_output( | ||
| "snap logs charmed-zookeeper.daemon -n 100000", | ||
| suggest_filename="snap-logs", | ||
| ) |
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.
same as above on all_logs option
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.
snap logs by default only captures 10 lines, is it intended that you only want 10 lines
snap logs --help
Usage:
snap logs [logs-OPTIONS] <service>...
The logs command fetches logs of the given services and displays them in
chronological order.
[logs command options]
--abs-time Display absolute times (in RFC 3339 format). Otherwise, display relative times up to 60 days, then YYYY-MM-DD.
-n= Show only the given number of lines, or 'all'. (default: 10)
-f Wait for new lines and print them as they come in.
[logs command arguments]
<service>: A service specification, which can be just a snap name (for all services in the snap), or <snap>.<app> for a single service.
What I meant with all_logs option was that we could do something similar to
if self.getOption('all_logs'):
self.add_cmd_output(
"snap logs -n 50000 charmed-zookeeper.daemon",
suggest_filename="snap_logs_charmed-zookeeper_daemon",
)
else:
self.add_cmd_output(
"snap logs -n 500 charmed-zookeeper.daemon",
suggest_filename="snap_logs_charmed-zookeeper_daemon",
)bf38433 to
5a6b097
Compare
|
@arif-ali - Thanks for the speedy review! I think I've addressed all points, let me know! |
TurboTurtle
left a comment
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 for the PR!
Some notes on these plugins below. Overall the plugins look beneficial and useful, but need some alignment with sos norms.
| # --- CRUISE CONTROL STATE --- | ||
|
|
||
| self.add_cmd_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.
These can all be cleaned up, and made to fit line lengths:
endpoints = {
'cruise-control-state': 'state?super_verbose=true',
'cluster-state': 'kafka_cluster_state?verbose=true',
'partition_load': 'partition_load',
...etc...
}
url = 'localhost:9090/kafkacruisecontrol'
for fname, api in endpoints.items():
self.add_cmd_output(
f"curl {self.credentials_args} {url}/{api}",
suggest_filename=fname
)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 don't see issue with the verbosity here, as having each step explicit helps with potential future post-processing for individual steps if needed.
02d862b to
15f49e5
Compare
|
@TurboTurtle - Have addressed the majority of your points, let me know if all is OK! |
15f49e5 to
c36056f
Compare
|
Hey, gentle bump on this. I'm not sure why the CI is failing, any ideas? |
|
Here are the backtraces that caused the Plus also: that I commented directly in the code. |
c36056f to
79d2c98
Compare
|
@pmoravec - Thank you! CI fixed I think, we good to go? |
261352d to
839e50d
Compare
Signed-off-by: Marc Oppenheimer <marcaoppenheimer@gmail.com>
|
@marcoppenheimer added some comments on log collection, otherwise looks good to me |
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines