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.

Sign up for GitHub

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

Support hourly logrotate directory on EL9 again. #221

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Support hourly logrotate directory on EL9 again. #221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06b1bed
Support hourly logrotate directory on EL9 again.
traylenator Feb 19, 2024
71423d1
Add flock to mutex the two logrotates
traylenator Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .fixtures.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ fixtures:
repositories:
stdlib: "https://github.com/puppetlabs/puppetlabs-stdlib.git"
cron: "https://github.com/puppetlabs/puppetlabs-cron_core.git"
systemd: "https://github.com/voxpupuli/puppet-systemd.git"
69 changes: 63 additions & 6 deletions manifests/hourly.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,8 @@
) {
assert_private()

$dir_ensure = $logrotate::ensure_cron_hourly ? {
'absent' => $logrotate::ensure_cron_hourly,
'present' => 'directory'
}

file { "${logrotate::rules_configdir}/hourly":
ensure => $dir_ensure,
ensure => 'directory',
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

owner => $logrotate::root_user,
group => $logrotate::root_group,
mode => $logrotate::rules_configdir_mode,
Expand All @@ -38,4 +33,66 @@
require => File["${logrotate::rules_configdir}/hourly"],
}
}

# Make copies of the rpm provided unit and timers
if $logrotate::manage_systemd_timer {
$_lockfile = '/run/lock/logrotate.service'
$_timeout = 21600
systemd::manage_dropin { 'hourly-service.conf':
ensure => $logrotate::ensure_systemd_timer,
unit => 'logrotate-hourly.service',
unit_entry => {
'Description' => [
'',
'Extra service to run hourly logrotates only',
],
},
service_entry => {
'ExecStart' => ['', "/usr/bin/flock --wait ${_timeout} ${_lockfile} /usr/sbin/logrotate ${logrotate::rules_configdir}/hourly"],
},
before => Systemd::Unit_file['logrotate-hourly.service'],
}

# Once logrotate >= 3.21.1 replace flock with the `--wait-for-state-lock` option.
systemd::manage_dropin { 'logrotate-flock.conf':
ensure => $logrotate::ensure_systemd_timer,
unit => 'logrotate.service',
service_entry => {
'ExecStart' => ['', "/usr/bin/flock --wait ${_timeout} ${_lockfile} /usr/sbin/logrotate /etc/logrotate.conf"],
},
}

systemd::unit_file { 'logrotate-hourly.service':
traylenator marked this conversation as resolved.
Show resolved Hide resolved
ensure => $logrotate::ensure_systemd_timer,
source => 'file:///lib/systemd/system/logrotate.service',
before => Systemd::Unit_file['logrotate-hourly.timer'],
}

systemd::manage_dropin { 'hourly-timer.conf':
ensure => $logrotate::ensure_systemd_timer,
unit => 'logrotate-hourly.timer',
unit_entry => {
'Description' => [
'',
'Extra timer to run hourly logrotates only',
],
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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....

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 :-(

Copy link
Author

Choose a reason for hiding this comment

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

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

},
timer_entry => {
'OnCalendar' => ['', 'hourly'],
},
before => Systemd::Unit_file['logrotate-hourly.timer'],
}

$_timer = $logrotate::ensure_systemd_timer ? {
'present' => true,
default => false,
}

systemd::unit_file { 'logrotate-hourly.timer':
ensure => $logrotate::ensure_systemd_timer,
source => 'file:///lib/systemd/system/logrotate.timer',
active => $_timer,
enable => $_timer,
}
}
}
4 changes: 4 additions & 0 deletions metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
{
"name": "puppetlabs/stdlib",
"version_requirement": ">= 4.22.0 < 10.0.0"
},
{
"name": "puppet/systemd",
"version_requirement": ">= 6.3.0 < 7.0.0"
}
],
"requirements": [
Expand Down
104 changes: 66 additions & 38 deletions spec/classes/init_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,6 @@
let(:facts) do
facts
end
let(:cron_ensure) do
if facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'].to_i >= 9
'absent'
else
'present'
end
end
let(:hourly_dir_ensure) do
if cron_ensure == 'present'
'directory'
else
'absent'
end
end

context 'logrotate class without any parameters' do
it { is_expected.to compile.with_all_deps }
Expand Down Expand Up @@ -54,23 +40,6 @@
is_expected.to contain_class('logrotate::defaults')
end
else
it do
is_expected.to contain_file('/etc/logrotate.d/hourly').with(
'ensure' => hourly_dir_ensure,
'owner' => 'root',
'group' => 'root',
'mode' => '0755'
)
end

it do
is_expected.to contain_file('/etc/cron.hourly/logrotate').with(
'ensure' => cron_ensure,
'owner' => 'root',
'group' => 'root',
'mode' => '0700'
)
end

it do
is_expected.to contain_package('logrotate').with_ensure('present')
Expand All @@ -80,11 +49,6 @@
'group' => 'root',
'mode' => '0755')

is_expected.to contain_file('/etc/cron.daily/logrotate').with('ensure' => cron_ensure,
'owner' => 'root',
'group' => 'root',
'mode' => '0700')

is_expected.to contain_class('logrotate::defaults')
end

Expand All @@ -94,10 +58,74 @@
'ensure' => 'running',
'enable' => true
)

is_expected.to contain_systemd__unit_file('logrotate-hourly.timer').with(
'ensure' => 'present',
'enable' => true,
'active' => true
)

is_expected.to contain_systemd__manage_dropin('hourly-timer.conf').with(
'ensure' => 'present',
'unit' => 'logrotate-hourly.timer'
)

is_expected.to contain_systemd__unit_file('logrotate-hourly.service').with_ensure('present').without_enable.without_active

is_expected.to contain_systemd__manage_dropin('hourly-service.conf').with(
'ensure' => 'present',
'unit' => 'logrotate-hourly.service',
'service_entry' => {
'ExecStart' => [
'',
'/usr/bin/flock --wait 21600 /run/lock/logrotate.service /usr/sbin/logrotate /etc/logrotate.d/hourly'
]
}
)

is_expected.to contain_systemd__manage_dropin('logrotate-flock.conf').with(
'ensure' => 'present',
'unit' => 'logrotate.service',
'service_entry' => {
'ExecStart' => [
'',
'/usr/bin/flock --wait 21600 /run/lock/logrotate.service /usr/sbin/logrotate /etc/logrotate.conf'
]
}
)
end
else

it do
is_expected.to contain_file('/etc/cron.hourly/logrotate').with(
'ensure' => 'present',
'owner' => 'root',
'group' => 'root',
'mode' => '0700'
)

is_expected.to contain_file('/etc/cron.daily/logrotate').with('ensure' => 'present',
'owner' => 'root',
'group' => 'root',
'mode' => '0700')
end

it do
is_expected.to contain_file('/etc/logrotate.d/hourly').with(
'ensure' => 'directory',
'owner' => 'root',
'group' => 'root',
'mode' => '0755'
)
end

it do
is_expected.not_to contain_service('logrotate.timer')
is_expected.not_to contain_systemd__unit_file('logrotate-hourly.timer')
is_expected.not_to contain_systemd__unit_file('logrotate-hourly.service')
is_expected.not_to contain_systemd__unit_file('logrotate-hourly.timer')
is_expected.not_to contain_systemd__unit_file('logrotate-hourly.service')
is_expected.not_to contain_systemd__manage_dropin('logrotate-flock.conf')
end
end
end
Expand Down Expand Up @@ -169,9 +197,9 @@

case facts[:operatingsystem]
when 'FreeBSD'
it { is_expected.to contain_file('/usr/local/etc/logrotate.d/hourly').with_ensure('absent') }
it { is_expected.to contain_file('/usr/local/etc/logrotate.d/hourly').with_ensure('directory') }
else
it { is_expected.to contain_file('/etc/logrotate.d/hourly').with_ensure('absent') }
it { is_expected.to contain_file('/etc/logrotate.d/hourly').with_ensure('directory') }
it { is_expected.to contain_file('/etc/cron.hourly/logrotate').with_ensure('absent') }
end
end
Expand Down