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

Support fine-grained logrotateAdditionalFiles intervals, with a separate logrotate conf file for each configured interval #2180

Merged
merged 11 commits into from
Mar 19, 2021

Conversation

baconmania
Copy link
Contributor

@baconmania baconmania commented Feb 16, 2021

In Singularity, we don't want to rely on logrotate(8) on Mesos agent hosts having support for hourly or finer-grained logrotate intervals. We therefore "fake" it by having SingularityExecutor write cron entries with schedules corresponding to those finer-grained intervals. These cron entries invoke logrotate -f to force rotation on the cron schedule.

Up until now, the only one of these "finer" intervals we supported in Singularity was hourly. We'd now like to support even finer intervals than that. However, Singularity up until now has written all these "cron-faked" hourly logrotate entries for a given Singularity Task to a single logrotate conf file. If we now add support for "cron-faked" rotation every five minutes, for example, we wouldn't want to write them into the same logrotate file as an hourly config, because then when cron calls logrotate -f on the file, it'll run both the hourly and the five-minutely configs.

This PR addresses this problem by separating out the cron-faked logrotate conf files by interval. So now, if we have three logrotateAdditionalFiles in SingularityExecutor's configuration...

  logrotateAdditionalFiles:
  - filename: logs/a.log
    logrotateFrequencyOverride: HOURLY
  - filename: logs/b.log
    logrotateFrequencyOverride: EVERY_MINUTE
  - filename: logs/c.log
    logrotateFrequencyOverride: EVERY_MINUTE
  - filename: logs/d.log
    logrotateFrequencyOverride: EVERY_FIVE_MINUTES

...then we write three separate logrotate conf files:

/etc/logrotate.d/hourly/SomeSingularityTaskId.HOURLY:
    <logrotate config entry for a.log>

/etc/logrotate.d/hourly/SomeSingularityTaskId.EVERY_MINUTE:
    <logrotate config entry for b.log>
    <logrotate config entry for c.log>

/etc/logrotate.d/hourly/SomeSingularityTaskId.EVERY_FIVE_MINUTES
    <logrotate config entry for d.log>

...and the cron file now looks like:

/etc/cron.d/SomeSingularityTaskId.logrotate:
0 * * * * root logrotate -f -s /statefile /etc/logrotate.d/hourly/SomeSingularityTaskId.HOURLY > /dev/null 2>&1

* * * * * root logrotate -f -s /statefile /etc/logrotate.d/hourly/SomeSingularityTaskId.EVERY_MINUTE > /dev/null 2>&1

*/5 * * * * root logrotate -f -s /statefile /etc/logrotate.d/hourly/SomeSingularityTaskId.EVERY_FIVE_MINUTES > /dev/null 2>&1

@baconmania
Copy link
Contributor Author

NOTE: This PR changes the filename format for the conf files written to /etc/logrotate.d/hourly/ (from ${singularityTaskId} to ${singularityTaskId}.${logrotateInterval}, but this should be safe, because all changes are limited to the SingularityExecutor module. So existing tasks will undergo SingularityExecutor cleanups based on the old name format, and new tasks launched after this rolls out will use the new name format on both the write and cleanup paths.

{{#if logrotateSizeBasedConfig}}
{{{ cronSchedule }}} root {{{ logrotateCommand }}} -s {{{ logrotateStateFile }}} {{{ logrotateSizeBasedConfig }}} {{{ outputRedirect }}}
*/5 * * * * root {{{ logrotateCommand }}} -s {{{ logrotateStateFile }}} {{{ logrotateSizeBasedConfig }}} {{{ outputRedirect }}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't strictly related to this PR, but I noticed that we were previously only evaluating size-based logrotateAdditionalFiles configs every hour. This hardcodes it to every five minutes, which makes it less likely that we let an extremely high-volume logfile fill up a Mesos host. Happy to lengthen this interval (or back out this change entirely) if we think this is too frequent, though.

@ssalinas
Copy link
Member

@baconmania is this one still needed?

@baxterdemers
Copy link

Yeah - we're still working on testing it

@ajammala ajammala merged commit 4c458f9 into master Mar 19, 2021
@ssalinas ssalinas added this to the 1.5.0 milestone May 4, 2022
@ssalinas ssalinas deleted the granular-forced-logrotate-crons branch May 4, 2022 16:40
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.

None yet

4 participants