Skip to content
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

feature: add logrotate for librenms log files #339

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

charlyforot
Copy link

@charlyforot charlyforot commented Mar 3, 2023

LibreNMS log files can become very large: #310

The goal of this PR is to configure and activate logrotate with the environment variables provided in the README.

Working great from my side with LOGROTATE_ENABLED=true after have built the docker image.

The logrotate configuration file named librenms is correctly created.

librenms-dispatcher:/opt/librenms# cat /etc/logrotate.d/librenms 
/opt/librenms/logs/*.log {
	su librenms librenms
	create 664 librenms librenms
	weekly
	rotate 6
	compress
	delaycompress
	missingok
	notifempty
}

We can see that the crons runs.

librenms-dispatcher:/opt/librenms/logs# crontab -l
# do daily/weekly/monthly maintenance
# min	hour	day	month	weekday	command
*/15	*	*	*	*	run-parts /etc/periodic/15min
0	*	*	*	*	run-parts /etc/periodic/hourly
0	2	*	*	*	run-parts /etc/periodic/daily
0	3	*	*	6	run-parts /etc/periodic/weekly
0	5	1	*	*	run-parts /etc/periodic/monthly

logrotate cron will be called every day.

librenms-dispatcher:/opt/librenms/logs# find /etc/periodic/ -name logrotate
/etc/periodic/daily/logrotate

We can try to force the logrotate to test this feature :

librenms-dispatcher:/opt/librenms/logs# logrotate /etc/logrotate.conf --force
librenms-dispatcher:/opt/librenms/logs# ls -lah
total 12K
drwxrwxr-x  2 librenms librenms 4.0K Mar  3 15:40 .
drwxr-xr-x 10 librenms librenms 4.0K Mar  3 14:25 ..
-rw-rw-r--  1 librenms librenms    0 Mar  3 15:40 librenms.log
-rw-rw-r--  1 librenms librenms    5 Mar  3 15:16 librenms.log-20230303

All done.

@charlyforot charlyforot requested a review from crazy-max as a code owner March 3, 2023 14:54
@murrant
Copy link
Member

murrant commented Apr 7, 2023

FYI, I don't think this makes any sense to be user configurable. Just enable it with sensible defaults.

@charlyforot
Copy link
Author

charlyforot commented May 1, 2023

@murrant Following your advice, I have removed config variables. I just kept the env variable to enable/disable logrotate.

README.md Outdated Show resolved Hide resolved
rootfs/etc/cont-init.d/09-config-logrotate.sh Outdated Show resolved Hide resolved
# shellcheck shell=bash
set -e

if [ ${LOGROTATE_ENABLED:-false} = true ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only set logrotate for the main service and not sidecar containers:

Suggested change
if [ ${LOGROTATE_ENABLED:-false} = true ]
if [ "$SIDECAR_DISPATCHER" = "1" ] || [ "$SIDECAR_SYSLOGNG" = "1" ] || [ "$SIDECAR_SNMPTRAPD" = "1" ]; then
exit 0
fi
if [ ${LOGROTATE_ENABLED:-false} = true ]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure for this because if we consider a main service on a host using a sidecar dispatcher on a remote host, the dispatcher will generate a lot of logs contrary to the main service.

I have tested this case, I have installed only a sidecar dispatcher on a remote host and it generates a lot of logs locally.

Do you agree that we have to keep the logrotate configuration on the sidecar_dispatcher ?

@crazy-max crazy-max linked an issue Jul 22, 2023 that may be closed by this pull request
@JamesMenetrey
Copy link

Hello @charlyforot,

Thanks for this PR; it seems much needed. Do you still plan to fix the remaining elements, as highlighted by @crazy-max?

Cheers,
Jämes

@charlyforot
Copy link
Author

Hello @charlyforot,

Thanks for this PR; it seems much needed. Do you still plan to fix the remaining elements, as highlighted by @crazy-max?

Cheers, Jämes

Hello @JamesMenetrey

Sorry for the response time, I'm going to fix this ASAP

Cheers

Co-authored-by: CrazyMax <[email protected]>
@murrant
Copy link
Member

murrant commented Oct 30, 2023

This makes me wonder. Isn't it common to log to stdout for docker containers? Shouldn't we be logging there instead of to a file?

@charlyforot charlyforot requested a review from crazy-max October 31, 2023 09:22
@JamesMenetrey
Copy link

@murrant Admittedly, logging into stdout is more canonical for logging using Docker, but I have to say I would be happy to have a log rotate option rather than nothing :)

@charlyforot
Copy link
Author

This makes me wonder. Isn't it common to log to stdout for docker containers? Shouldn't we be logging there instead of to a file?

Yes, it's right !

I tried to redirect some logs from librenms.log to stdout, however it's not easy because there are differents ways to log into LibreNMS code :

  • In python files with logger
  • In PHP files with d-echo(), echo(), Log::channel()->alerts, logfile()

I can't redirect some PHP logs to stdout because of code design, if someone has an idea ?

Otherwise, we could disable some logs which are duplicated between librenms.log and stdout container output by adding these lines into rootfs/etc/cont-init.d/03-config.sh :

# Config : Disable logs to file already present in stdout
sed -i 's/logfile(/\/\/logfile(/g' ${LIBRENMS_PATH}/poll-billing.php
sed -i 's/logfile(/\/\/logfile(/g' ${LIBRENMS_PATH}/discovery.php
sed -i "s/Log::channel('single')/Log::channel('stdout')/g" ${LIBRENMS_PATH}/LibreNMS/Poller.php

This would remove the large part of the annoying logs from librenms.log.

Other logs in librenms.log come from poll-billing.php and discovery.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable LOGROTATE
4 participants