Support hourly logrotate directory on EL9 again.

[traylenator]

Pull Request (PR) description

When EL9 switched to using systemd (quite correctly ) in #216 support for a special directory for hourly logrotations was removed - that change was backwards incompatible.

Add that functionality back with a new unit and timer

• logrotate-hourly.service
• logrotate-hourly.timer

While this could be achieved by reducing the period of existing timer to hourly I presume the motivation for this was to avoid processing all logrotate directives every hour which does make some sense.

Following this patch:

# systemctl list-timers | grep logro
Mon 2024-02-19 14:00:00 CET 46min left  -                           -            logrotate-hourly.timer       logrotate-hourly.service
Tue 2024-02-20 00:00:00 CET 10h left    Mon 2024-02-19 09:53:53 CET 3h 19min ago logrotate.timer              logrotate.service

and

# systemctl show logrotate-hourly.service -p ExecStart
ExecStart={ path=/usr/sbin/logrotate ; argv[]=/usr/sbin/logrotate /etc/logrotate.d/hourly ; ignore_errors=no ; start_time=[n/a] ; stop_time=[n/a] ; pid=0 ; code=(null) ; status=0/0 }

[traylenator]

CentOS 8 tests are a timeout.

[treydock]

Would it be worth doing to make this timer also trigger logrotate.service by setting I believe Unit=logrotate.service? This way the hourly and regular OS services can't run at same time. If both timers use the same service, that ensures there would be no overlap of log rotation commands.

[traylenator]

Hi,

I don't think hourly and daily can call the same service. They have different exec lines as they operate on different config files.

As for the hourly and daily overlapping I can't immediately see why it would matter but if we could then yes makes sense to avoid it.

Quick search around systemd does not have anything obvious to mutex a couple of units.

Only suggestions I can see are an external flock around the logrotate command or use of some ExecCondition.

Should check if logrotate maintains it's own lock....

[traylenator]

Logrotate has a --wait-for-state-lock which sounds like the ticket.

• Need to check there's a timeout configured.
• Such a change is probably better as it's own MR since this applies to the cron variant the same.

[treydock]

If that wait lock doesn't work it could be done with like ExecStartPre=/bin/touch /var/lock/logrotate and ExecStartPost=/bin/rm -f /var/lock/logrotate then have the units conditional on that path not existing. That wait for state lock sounds better though. A new PR for those changes sounds fine.

[traylenator]

flock is the way to do that - better to do that than maintain your own lock always with external commands. i.e
logrotate becomes `

/usr/bin/flock --wait 21600  /run/lock/logrotate.service  /usr/bin/logrotate

this also solves the timeout problem since the waiting unit will enter a failed state after 6 hours which seems perfect.

Unfortuantly the --wait-for-state-lock in logrotate itself only arrived with 3.21.0 which is newer than EL9 has :-(

[traylenator]

/usr/bin/flock now added to mutex the two instances.

[treydock]

Why not allow this to be removed when hourly is ensure=absent?

[traylenator]

When which parameter is set not /set ?

• $logrotate::manage_cron_hourly controls if the crons are managed.
• $logrotate::manage_systemd_timer controls if the units should be managed

Just noticed this confusion in the class:

In logrotate::rule it includes logrotate::daily if and only if there is a rule which is hourly.

However there is also a contain in logrotate::init - that contain seems wrong to me. Should we be creating these units, crons and directories if there are no hourly rules.

In reference to the "${logrotate::rules_configdir}/hourly": I don't see how you can know up front that the directory need to be created or not - certainly not that it can be deleted.

I would remove that contain from manifests::init myself so for the default case the extra cron, timer and directory never get created.

[traylenator]

Looking at this ancient MR https://github.com/voxpupuli/puppet-logrotate/pull/174/files it looks to be quite deliberate
that the ${logrotate::rules_configdir}/hourly" is always created in all circumstances.

Either that should go or the one here in logrotate::rule

case $rotate_every {
 'hour', 'hourly': {
   include logrotate::hourly
   $rule_path = "${logrotate::rules_configdir}/hourly/${rulename}"

one of these is definitely redundant.

[traylenator]

Could split hourly in two files.
Move the directory management into its own file to create and purge always.
Existing file without that could be included iff there an hourly logrotate rule.