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

Do not use copytruncate for xcat log rotation #7425

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Conversation

Obihoernchen
Copy link
Member

@Obihoernchen Obihoernchen commented Feb 23, 2024

PR #6510 tried to fix missing logs for goconserver, but also added copytruncate to all xcat logs in /var/log/xcat*.log. This is not needed because these logs are written by rsyslog and xcat itself, not goconserver.

The main rsyslog developer does not recommend to use copytruncate for rsyslog: https://serverfault.com/a/901366

For HA setups with logs on NFS etc. copytruncate can be very slow. A simple move/rename is way faster and cleaner.

I'm keeping delaycompress because I think it's nice to have some more days/weeks of logs uncompressed. Furthermore, some users might be used to it already.

PR #6510 tried to fix missing logs for goconserver, but also added copytruncate to all xcat logs in /var/log/xcat*.log. This is not needed because these logs are written by rsyslog and xcat itself, not goconserver.

The main rsyslog developer does not recommend to use copytruncate for rsyslog: https://serverfault.com/a/901366

For HA setups with logs on NFS etc. copytruncate can be very slow.
@Obihoernchen Obihoernchen added this to the 2.17 milestone Feb 23, 2024
@Obihoernchen Obihoernchen self-assigned this Feb 23, 2024
Copy link

@ocfmatt ocfmatt left a comment

Choose a reason for hiding this comment

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

When logrotate runs are log files correctly written to post rotate?

I have seen issues in other software where logrorate runs and the process is still writing to the old inode. To resolve these issues, I implemented copytruncate.

@Obihoernchen
Copy link
Member Author

Yes but this shouldn't be the case here because we do:

 test -f /var/run/rsyslogd.pid && kill -HUP `cat /var/run/rsyslogd.pid 2> /dev/null` 2> /dev/null || true

The official rsyslog-logrotate package is doing the same:

[root@service01 ~]# cat /etc/logrotate.d/rsyslog
/var/log/cron
/var/log/maillog
/var/log/messages
/var/log/secure
/var/log/spooler
{
    missingok
    sharedscripts
    postrotate
        /usr/bin/systemctl -s HUP kill rsyslog.service >/dev/null 2>&1 || true
    endscript
}

I tested the change and for me it's working fine. And before #6510 there was no issue with xCAT logs either.

But please test on your system, too.

@samveen
Copy link
Member

samveen commented Feb 23, 2024

Given that rsyslog honors the HUP signal to close and reopen logs, copytruncate isn't required for it. The postrotate script just needs to ensure that the rsyslogd is correctly signaled about log rotation completion.

Ubuntu 20.04 uses the following:

samveen@prod-1:~$ grep '^VERSION=' /etc/os-release 
VERSION="20.04.6 LTS (Focal Fossa)"
samveen@prod-1:~$ grep postrotate -A2 /etc/logrotate.d/rsyslog 
        postrotate
                /usr/lib/rsyslog/rsyslog-rotate
        endscript
--
        postrotate
                /usr/lib/rsyslog/rsyslog-rotate
        endscript

Ubuntu 22.04 uses the following:

samveen@Ubuntu-2204-jammy-amd64-base ~ # grep '^VERSION=' /etc/os-release; grep -A2 postrotate /etc/logrotate.d/rsyslog 
VERSION="22.04.3 LTS (Jammy Jellyfish)"
        postrotate
                /usr/lib/rsyslog/rsyslog-rotate
        endscript

Debian 12 Bookworm uses the following:

samveen@zero2w:~/workspace/xcat-core $ grep '^VERSION=' /etc/os-release 
VERSION="12 (bookworm)"
samveen@zero2w:~/workspace/xcat-core $ grep -A2 postrotate /etc/logrotate.d/rsyslog 
        postrotate
                /usr/lib/rsyslog/rsyslog-rotate
        endscript

I'd suggest adding the debian/ubuntu check+execute into the postrotate section of xCAT/etc/logrotate.d/xcat , or I can create a PR for this change this is merged.

@Obihoernchen
Copy link
Member Author

There is no /usr/lib/rsyslog/rsyslog-rotate on EL, therefore I would just stick to the current implementation of sending the HUP signal to /var/run/rsyslogd.pid. This should work on most OSs.

@samveen
Copy link
Member

samveen commented Feb 24, 2024

@Obihoernchen Yes that script doesn't exist for the RH based distributions. However:

samveen@zero2w:~/workspace/xcat-core $ awk '/postrotate/{P=1} /endscript/{print;P=0} {if (P==1)print}'<xCAT/etc/logrotate.d/xcat
    postrotate
        test -f /var/run/rsyslogd.pid && kill -HUP `cat /var/run/rsyslogd.pid 2> /dev/null` 2> /dev/null || true
        test -f /var/run/syslogd.pid && kill -HUP `cat /var/run/syslogd.pid 2> /dev/null` 2> /dev/null || true
        test -f /var/run/xcat/cmdlogservice.pid && kill -HUP `cat /var/run/xcat/cmdlogservice.pid 2> /dev/null` 2> /dev/null || true
    endscript

What I am proposing is to add the following to the list in the postrotate above:

        test -x  /usr/lib/rsyslog/rsyslog-rotate && /usr/lib/rsyslog/rsyslog-rotate || true

This fixes a hole in Ubuntu support, as well as increases Debian compatibility. It's also possible that copytruncate was specifically added just because such signalling is missing on Ubuntu, thus leading to the error #6510 was trying to fix.

@Obihoernchen
Copy link
Member Author

@samveen Very good point thank you! Added.

@samveen
Copy link
Member

samveen commented Feb 26, 2024

@Obihoernchen Trying to get closet to the tip than the base in Graham's Hierarchy of Disagreement 🤣

Copy link

@ocfmatt ocfmatt left a comment

Choose a reason for hiding this comment

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

Thanks for the additional comments above. Changes approved.

@Obihoernchen Obihoernchen merged commit cbee70b into master Feb 28, 2024
1 check passed
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.

3 participants