Add logrotate config file/task + CI updates #16
Conversation
There was a problem hiding this comment.
Hi Frank. Thanks for the PR, much appreciated as always.
I’m currently on vacation for a few days so may be a bit delayed in my responses. I noticed that the Molecule test in the build process failed. Seems like maybe a role name issue and unrelated to your changes. Is possibly due to these latest Ansible Galaxy changes and the name of my repo, see https://galaxy.ansible.com/docs/contributing/creating_role.html#role-names. Would you be able to add that role_name: kafka Entry in the meta/main.yaml file like they mention in your next push and see if that resolves it please.
Cheers.
templates/logrotate.j2
Outdated
| @@ -0,0 +1,11 @@ | |||
| {{ kafka_root_dir }}/logs/*.log { | |||
There was a problem hiding this comment.
This looks like it will resolve to /opt/logs/*.log. Should be {{ kafka_log_dir }}/*.log instead.
templates/logrotate.j2
Outdated
| delaycompress | ||
| missingok | ||
| copytruncate | ||
| create 644 {{ kafka_user }} {{ kafka_group }} |
There was a problem hiding this comment.
I had a look at the docs and the copytruncate option says that the create option will be ignored if copytruncate is used.
There was a problem hiding this comment.
I've made all the requested changes with added CI refactoring. Waiting for your feedbacks :)
There was a problem hiding this comment.
Thanks. I'm happy with all the CI changes but not 100% sure about the logrotate stuff. Kafka already has log4j config that will roll the logs on a daily basis, which logrotate is also doing here. It's unclear to me if you would have "conflicts" and have logs being rolled over twice. You'd get some rolled over logs with a numeric suffix and some with a date suffix. An alternative approach could be to instead template in the default Kafka log4j config, but with an updated copy that uses the filesize and max number of files instead, the same config as you have used for logrotate. This would need to use the log4j RollingFileAppender instead of the DailyFileAppender, see here for an example https://stackoverflow.com/questions/51970191/log-rotation-based-on-monthly-and-log-retention-for-30-days-in-rollingfileappend. The downside to this however is that this will only then roll by size, and not by date...but this may not be an issue. Thoughts?
There was a problem hiding this comment.
Bringing up logrotate was meant to be an opportunistic and easy change to make, which does not requires much Kafka/log4j knowledge. Therefore I understand and agree with your concerns.
If we want this role to be as much enterprise-ready as possible, offering users to configure log4j instead is the way to go.
I suggest I revert the first logrotate commit then add a jinja template and default variables to support log4j configuration 👍🏻
The role users will then be free to chose whichever configuration they want to help prevent disk saturation the "right" way.
Thoughts ?
There was a problem hiding this comment.
Yup, I agree, sounds good. Thanks for that.
fidanf
changed the title
Greetings,
Here is a simple PR for a simple need. Thanks for the review