MDEV-22659: Create one single unified and optimal logrotate config (pending on RPM packaging changes)

[ottok]

Replace mysql-log-rotate.sh and debian/...mysql-server.logrotate with one
new unified and well documented version.

Name is mariadb.logrotate.in as in 10.5 branch we use now the 'mariadb'
name, and use 'logrotate' to match the actual name of the utlity, and
use '.in' instead of '.sh' as this is not a shell script but a template
file.

[ottok]

To test this, install MariaDB in an environment that does not have journald but instead sysv init and logrotate. Then configure log-error to go to a file, restart MariaDB and forcefully run logrotate.

root@2fa7f88e3698:/build# echo "[mysqld_safe]
log-error=/var/log/mysql/error.log
" > /etc/mysql/conf.d/mysqld_safe_syslog.cnf

root@2d75264bf7d2:/var/log/mysql# my_print_defaults mysqld_safe
--socket=/var/run/mysqld/mysqld.sock
--nice=0
--log-error=/var/log/mysql/error.log

root@2fa7f88e3698:/build# /etc/init.d/mariadb restart
Stopping MariaDB database server: mysqld.
Starting MariaDB database server: mysqld.

root@2d75264bf7d2:/var/log/mysql# logrotate --force --verbose /etc/logrotate.d/mariadb 
reading config file /etc/logrotate.d/mariadb
Reading state from file: /var/lib/logrotate/status
Allocating hash table for state file, size 64 entries
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state
Creating new state

Handling 1 logs

rotating pattern: /var/log/mysql/mysqld.log /var/log/mysql/mysql.log /var/log/mysql/mysql-slow.log /var/log/mysql/mariadb-slow.log /var/log/mysql/error.log  forced from command line (7 rotations)
empty log files are not rotated, old logs are removed
considering log /var/log/mysql/mysqld.log
  log /var/log/mysql/mysqld.log does not exist -- skipping
considering log /var/log/mysql/mysql.log
  log /var/log/mysql/mysql.log does not exist -- skipping
considering log /var/log/mysql/mysql-slow.log
  log /var/log/mysql/mysql-slow.log does not exist -- skipping
considering log /var/log/mysql/mariadb-slow.log
  log /var/log/mysql/mariadb-slow.log does not exist -- skipping
considering log /var/log/mysql/error.log
  Now: 2020-05-21 19:24
  Last rotated at 2020-05-21 19:20
  log needs rotating
rotating log /var/log/mysql/error.log, log->rotateCount is 7
dateext suffix '-20200521'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
compressing log with: /bin/gzip
renaming /var/log/mysql/error.log.7.gz to /var/log/mysql/error.log.8.gz (rotatecount 7, logstart 1, i 7), 
old log /var/log/mysql/error.log.7.gz does not exist
renaming /var/log/mysql/error.log.6.gz to /var/log/mysql/error.log.7.gz (rotatecount 7, logstart 1, i 6), 
old log /var/log/mysql/error.log.6.gz does not exist
renaming /var/log/mysql/error.log.5.gz to /var/log/mysql/error.log.6.gz (rotatecount 7, logstart 1, i 5), 
old log /var/log/mysql/error.log.5.gz does not exist
renaming /var/log/mysql/error.log.4.gz to /var/log/mysql/error.log.5.gz (rotatecount 7, logstart 1, i 4), 
old log /var/log/mysql/error.log.4.gz does not exist
renaming /var/log/mysql/error.log.3.gz to /var/log/mysql/error.log.4.gz (rotatecount 7, logstart 1, i 3), 
old log /var/log/mysql/error.log.3.gz does not exist
renaming /var/log/mysql/error.log.2.gz to /var/log/mysql/error.log.3.gz (rotatecount 7, logstart 1, i 2), 
renaming /var/log/mysql/error.log.1.gz to /var/log/mysql/error.log.2.gz (rotatecount 7, logstart 1, i 1), 
renaming /var/log/mysql/error.log.0.gz to /var/log/mysql/error.log.1.gz (rotatecount 7, logstart 1, i 0), 
old log /var/log/mysql/error.log.0.gz does not exist
log /var/log/mysql/error.log.8.gz doesn't exist -- won't try to dispose of it
renaming /var/log/mysql/error.log to /var/log/mysql/error.log.1
running postrotate script
mysqld is alive

root@2d75264bf7d2:/var/log/mysql# find /var/log/mysql/ -ls
    41551      0 drwxr-s---   1 mysql    adm           166 May 21 19:24 /var/log/mysql/
    42770      4 -rw-rw----   1 mysql    adm            34 May 21 19:18 /var/log/mysql/mariadb-bin.index
    42786      4 -rw-rw----   1 mysql    adm           330 May 21 19:18 /var/log/mysql/mariadb-bin.000001
    42848      4 -rw-rw----   1 mysql    adm          1242 May 21 19:18 /var/log/mysql/error.log.3.gz
    42854      4 -rw-rw----   1 mysql    adm          1242 May 21 19:18 /var/log/mysql/error.log.2.gz
    42849      4 -rw-rw----   1 mysql    adm          3182 May 21 19:18 /var/log/mysql/error.log.1
    42855      0 -rw-rw----   1 mysql    adm             0 May 21 19:24 /var/log/mysql/error.log

[ottok]

My branch at http://buildbot.askmonty.org/buildbot/grid?branch=ok-10.5-logrotate&category=main&category=experimental did not run rpm tests. This needs to be verified that it works correctly for rpm packages as well.

Due to MDEV-19933 the PR #1504 should be merged before this, so that reading myslds_safe config would work correctly in the first place.

[ottok]

CI: The thread_pool_info test failure on buildbot/aarch64-centos-7 in unrelated to changes in this PR. All CI systems pass OK for this commit, but on the other hand, there is no CI that specifically tests logrotate.

[ottok]

Rebased on latest 10.5.

[grooverdan]

Good selection of log-rotate directives and certainly a great improvement covering aspects of other submitted works. Slightly over-commented in the file, however the most critical elements are well covered.

The debian named configuration file in a global file looks a little out of place. Could replace it with a few large cmake text substitutions.

This is adding @logdir@ as install path that is probably one of the only ones not in cmake/install_layout.cmake

[ottok]

Rebased on latest 10.5.

[ottok]

Rebased on latest master again and added maxsize since two people indicated in https://jira.mariadb.org/browse/MDEV-22659 that it would be a very useful addition, even though the main point of this PR is to refactor and fix bugs without "new features".

[ottok]

Please indicate if this bugfix change in acceptable for 10.5 (and thus close many long standing bugs there)? If not, I can rebase this on 10.6 and merge there.

[ottok]

Rebased on latest 10.5 as it looks this is motivated to merge on 10.5 to fix a bug in mariadbd process detection (and not postpone this and merge on 10.6).

Was there some tweaks you wanted for the config here @grooverdan ?

[grooverdan]

thanks @ottok

[grooverdan]

I suspect rpm based uses the mysql user - support-files/rpm/server-postin.sh + scripts/mysql_install_db.sh which means they'll need a 'user mysql' in this file.

[ottok]

Why don't we have Unix socket authentication everywhere? If user 'mysql' is needed in rpm world, the postrorate -x could be extended with another test, maybe check for user mysql in /etc/passwd and then prepend the mariadb-admin call with su mysql or something?

[grooverdan]

It is unix socket auth, its just using the 'mysql' user. It just needs to be checked that this implementation falls under the permitted selinux rules.

[ottok]

User 'mysql' is not here now has this been tested for SELinux. Neither was in the legacy logrotate file this replaces, so it should not be a step backward.

I suggest you merge this and do additional changes in a follow-up commit.

[ottok]

Rebased on latest master 10.5 and updated everything as suggested by @grooverdan, apart from mysql user usage for rpm distros.

[ottok]

I rebased this on 10.7. Original PR was made in early 2020 before the 10.5 release went GA. This change should not go into a stable release, hence changing base on 10.7.

[FaramosCZ]

I did some research and this proposal looks good 👍

I love to get rid of the authentication issue to mysqladmin, solved with unix sockets and everpresent 'root' and 'mysql' users.

I agree that using mysqladmin is far more better approach than flushing using a SIGHUP.
Even if brings another dependency to the server. (Atleast in Fedora, the mysqladmin binary is packed in the client package).
A standalone package ("mariadb-mysqladmin" ?) might suit it more, so client-only users won't need to install the whole server and the server size-concerned setups (e.g. containers) won't pull in the whole client.
At this moment Fedora package 'mariadb-server' pulls in the client, so nothing needs to be changed, however I've already got requests to truly divide the server and the client for container use cases.

We are taking a look at the @localstatedir@/*.log -like expression, just to verify it will work for us where we need.

You also changed the default values / configuration of the logrotate itself, for which I can't see any justification. (Are those values better? Why?) (I also would split it to a standalone commit)
The values looks fine, I just miss the justification for _ the change _ to them.

---

There is a one comment we have in our patch:

The 'mysqladmin flush logs' doesn´t guarantee, no entries are lost during flushing, the operation is not atomic.

I couldn't trace the origin of this comment.
However I'd like to hear your opinions, on:
1/ whether you see that statement to be true
2/ whether it is really concerning, or no one gives a damn

[ottok]

However I'd like to hear your opinions, on:
1/ whether you see that statement to be true
2/ whether it is really concerning, or no one gives a damn

I would go for option 2 here. Considering that mariadb.logrotate only rotates event/debug logs, it does not matter if they are truly atomic or not. I can understand that for e.g. binlogs where every transaction matters, but for these logrotate logs it should not make any material difference.

i am happy to update this PR if you have feedback and think any of the changes should be done differently.

If you have RPM related additions that extend this PR, then I suggest we merge this first and then merge a second PR by you on top of it.

[illuusio]

If you have RPM related additions that extend this PR, then I suggest we merge this first and then merge a second PR by you on top of it.

I would do it like this that first get this in and then figure out RPM stuff.

[grooverdan]

@FaramosCZ answering some:

I did some research and this proposal looks good +1

Thanks.

I love to get rid of the authentication issue to mysqladmin, solved with unix sockets and everpresent 'root' and 'mysql' users.

At the moment without a su mysql in the logrotate script this still runs as root. If we need to converge to having mysql database user in upstream to match Fedora (an others) some more work will be needed.

I agree that using mysqladmin is far more better approach than flushing using a SIGHUP.
Even if brings another dependency to the server. (Atleast in Fedora, the mysqladmin binary is packed in the client package).
A standalone package ("mariadb-mysqladmin" ?) might suit it more, so client-only users won't need to install the whole server

Yes.

and the server size-concerned setups (e.g. containers) won't pull in the whole client.

They tend to anyway to get a decent entry point to initialize a datadir to arbitary content.

At this moment Fedora package 'mariadb-server' pulls in the client, so nothing needs to be changed, however I've already got requests to truly divide the server and the client for container use cases.

Containers use straight stderr/stdout for the main log, having slow query log via a separate file makes a partial case.

We are taking a look at the @localstatedir@/*.log -like expression, just to verify it will work for us where we need.

I'm about to add a commit to effect this. @localstatedir@ is actually the default datadir so I've changed this to @logdir@ and added an RPM condition.

You also changed the default values / configuration of the logrotate itself, for which I can't see any justification. (Are those values better? Why?) (I also would split it to a standalone commit)
The values looks fine, I just miss the justification for _ the change _ to them.

Please @ottok summarize change reasons above.

There is a one comment we have in our patch:
The 'mysqladmin flush logs' doesn´t guarantee, no entries are lost during flushing, the operation is not atomic.
I couldn't trace the origin of this comment.
However I'd like to hear your opinions, on:
1/ whether you see that statement to be true

I suspect the delaycompress means there may be no loss. I'll still need to strace logrotate to confirm. There is also no loss if the compression happens after the postrotate.

2/ whether it is really concerning, or no one gives a damn

The errorlog - unlikely to be growing, of if so, its likely to be fairly repetitive messages so missing one isn't critical.

slow query log - also usually used in aggregation only. If its too noisy its not going to miss much.

general log - usually only debugging for short periods (if sane). So not a concern.

I'd like to see some better justification in a commit message of the logrotate files based on various distro defaults.

[CLAassistant]

All committers have signed the CLA.

[ottok]

Force pushed refreshed version that is rebased on the 10.8 branch, 658a1e1, 1b0fb2f and includes fix from https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/a67e39ccc6209c719138738fa5370c3eb63640d7.

Next I will update this with suggestions from ottok#6.

[ottok]

Rebased on 10.9. Branch name has still 10.5 in it :)

Please re-review @grooverdan

Latest version addresses all review feedback

[ottok]

Rebased on latest 10.9. Note that we have been using this successfully in Debian for a couple of years already with zero bug reports.

Merging this would fix several long standing MDEVs:

• https://jira.mariadb.org/browse/MDEV-11884 (Status 'Closed', but wasn't actually fixed by Ondrej in upstream) - deb version, upstreaming of this in PR#934
• https://jira.mariadb.org/browse/MDEV-13050 - deb version, also fixed in PR#934)
• https://jira.mariadb.org/browse/MDEV-16621 - delaycompress and sighup suggestions
• https://jira.mariadb.org/browse/MDEV-16772 - deb version, duplicate pid result fixed in PR#934
• https://jira.mariadb.org/browse/MDEV-19436 - deb version, duplicate of duplicate pid issue
• https://jira.mariadb.org/browse/MDEV-20863 - deb version, duplicate of duplicate pid issue

(list copied from https://jira.mariadb.org/browse/MDEV-22659)

[FaramosCZ]

Rebased on latest 10.9. Note that we have been using this successfully in Debian for a couple of years already with zero bug reports.

I gave it a thought again.

So far I've been pretty conservative.
However now, I finished a great load of a downstream work, putting into our latest products the oldest MariaDB release that will ever be supported there - 10.5.

If any time is good for the introduction of bigger changes, it is now.
On Zulip it was discussed which release should be marked LTS. The last time I read the thread, I saw an agreement that MariaDB 10.10 or some near major version would fit the various operating systems maintainers.

Right now, there is a time window on our side in which we wait for LTS to be announced and then all our efforts will be put into packing it and releasing it in our products.
We don't mind introducing even bigger changes if we have them well documented.

So if there is any good time to go with this change, it is after 10.5 and until the LTS (inclusively), which I expect to be some major versions around 10.10.

[grooverdan]

Thanks @FaramosCZ , been meaning to get a LTS email out. Next week I hope. Please get a wish list ready.

[grooverdan]

Previous review comments weren't addressed. It couldn't be a unified configuration without this.

As many distros are waiting for such changes, I've done these changes.

I couldn't push to this branch to update this PR, so a new PR was created #2312.

@ottok thanks for the work to get most of the logrotate configuration to a suitable state.

Thanks for the review @FaramosCZ