Skip to content

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.

# 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 <github@crazymax.dev>
@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

@garryshtern
Copy link

Can someone comment on the timeline for this merge? The way I see it, there is no harm in having logrotate installed in the container for either dispatcher or UI. Additionally, I am not really sure why we are disabling cron for non-ui deployments. What’s the harm with it being enabled?

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

5 participants