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

app-admin/logrotate: cronhourly to install cron file into cron.hourly #37201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jkroonza
Copy link
Contributor

@jkroonza jkroonza commented Jun 18, 2024

rather than cron.daily.

The alternative is to copy the existing cron.daily/logrotate file into cron.hourly, then remove the cron.daily/logrotate file and then rely on the fact that this file will never into the future get updated (Install logrotate with USE=-cron).

Additionally, using USE=-cron for logrotate will drop the virtual/cron dependency.

For these reasons I think it's safer to rather add an extra IUSE to install for hourly rather than daily use.


Please check all the boxes that apply:

  • I can submit this contribution in agreement with the Copyright Policy.
  • I have certified the above via adding a Signed-off-by line to every commit in the pull request.
  • This contribution has not been created with the assistance of Natural Language Processing artificial intelligence tools, in accordance with the AI policy.
  • I have run pkgcheck scan --commits --net to check for issues with my commits.

Please note that all boxes must be checked for the pull request to be merged.

By default with USE=cron logrotate installs a cron.daily file, we have a
small number of systems where this needs to be hourly.

One strategy is to just move the /etc/cron.daily/logrotate into
/etc/cron.hourly, then set USE=-cron for logrotate,  and then rely on
the fact that this file will never into the future get updated.

Setting USE=-cron for logrotate will drop the virtual/cron dependency,
so I'd refer to keep merging with USE=cron.  Which risks having the file
both for .hourly and .daily.

For these reasons I think it's safer to rather add an extra IUSE to
install for hourly rather than daily use.

Signed-off-by: Jaco Kroon <[email protected]>
@gentoo-bot
Copy link

Pull Request assignment

Submitter: @jkroonza
Areas affected: ebuilds
Packages affected: app-admin/logrotate

app-admin/logrotate: @gentoo/base-system

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jun 18, 2024
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-06-18 11:26 UTC
Newest commit scanned: 51bc869
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/abd4b51dee/output.html

@juippis
Copy link
Member

juippis commented Jun 18, 2024

I honestly believe this should be left for sysadmins to manage. Now that I look at it, I don't personally even like the idea of putting it automatically under any cron*. I'd just install the file somewhere and print out a message to move it where an administrator finds it most suitable.
You're introducing new annoying logic to the ebuild REQUIRED_USE which makes it harder to test, and REQUIRED_USE is often worsening the user experience.

Now I'm not part of the base-system project so I have no say here - but my opinion was asked.

@floppym
Copy link
Contributor

floppym commented Jun 18, 2024

Would there be any harm in installing the job in cron.hourly instead of cron.daily unconditionally?

The logrotate man page says it wont touch log files more than once per day, so there's already some logic baked in there.

@floppym
Copy link
Contributor

floppym commented Jun 18, 2024

The man page also says it will rotate sized-based logs more than once per day. So I suppose if you have a runaway process creating huge logs, running logrotate more frequently would result in added log file rotations.

@jkroonza
Copy link
Contributor Author

I honestly believe this should be left for sysadmins to manage. Now that I look at it, I don't personally even like the idea of putting it automatically under any cron*. I'd just install the file somewhere and print out a message to move it where an administrator finds it most suitable. You're introducing new annoying logic to the ebuild REQUIRED_USE which makes it harder to test, and REQUIRED_USE is often worsening the user experience.

Now I'm not part of the base-system project so I have no say here - but my opinion was asked.

Thank you for your input, I appreciate. I do think that installing into cron.daily (or cron.hourly) by default is the right thing to do though. People expect a working logrotate directly after installing, so I think requiring additional action by default here is worse than current, but I definitely take your input on the REQUIRED_USE. This feels almost like it should be a USE_EXPAND where you pick zero or no implementation with daily being the default, but I don't like that idea either.

@jkroonza
Copy link
Contributor Author

Would there be any harm in installing the job in cron.hourly instead of cron.daily unconditionally?

Default rotate time for daily would (barring effects of minsize/maxsize) shift from 3am or 4am to 0am (cron.hourly seems to run one minute after every hour), and cron.daily seems to depend on whether your using cron or anacron. With minsize/maxsize it would be dependent on log file size and rotation time would vary, possibly to move into busier system periods, potentially invoking tools like gzip at unexpected times.

In our case we have "runaway logs" (in some cases up to 100GB/day/log, where others for systems that happen to be quiet is maybe 10MB/day) and that's required logging so we're OK to sacrifice some CPU to get that compressed more often. So plan is something like:

minsize 100M
maxsize 1G
hourly
delaycompress

The logrotate man page says it wont touch log files more than once per day, so there's already some logic baked in there.

Well, that statement is slightly misleading (not you, the man page). There are a few options that interact with each other, including dateext and which of the period options you pick. The way I understand this after multiple reads of the man page this morning, I think the simplest way I can explain this:

  1. if not using dateext then logrotate won't rotate more frequently than the period selected for the log file, unless maxsize is exceeded, but may skip rotation if minsize hasn't been reached. This means that if you run logrotate every 15 minute and maxsize is exceeded every time you can end up with 15 minute interval rotations.

  2. If using dateext, the same rules will apply, except if ${logfile}${dateformat} or ${logfile}${dateformat}.${compressext} already exist, in which case rotation will be skipped too (ie, rotation will be skipped if dateformat doesn't allow for a unique filename that hasn't already been used).

The man page also says it will rotate sized-based logs more than once per day. So I suppose if you have a runaway process creating huge logs, running logrotate more frequently would result in added log file rotations.

Refer to above, if you're using dateext, and dateformat doesn't allow for this, then more than daily rotations isn't possible.

Personally, I'd be OK if the choice was made to just install the logrotate cron file to cron.hourly rather than cron.daily. The possible downsides (in my opinion are):

  1. If you have many, many log files, the time taken, and io pressure, to check if rotations should happen or not could possibly be significant (I think our worst system has log counts approaching 10k, primarily apache access and error log files).
  2. using excessive CPU for compressing during "peak times" (whatever your definition for that would be).
  3. copytruncate in particular in my experience is bad news, but that applies whether you're doing cron.hourly or cron.daily.

The PR tried to go middle-ground by giving a choice to switch to hourly rather than the default daily. As per @juippis this may not be the most user-friendly experience during installation, and I'm happy to concede that, but my reasoning is that if you set cronhourly and not cron then a user might expect that the cron is installed into cron.hourly where it would not install at all, so forcing cron for cronhourly results in the simplest change here. Happy to complicate the PR if the user-experience can be simplified. Eg, having two IUSE variables:

crondaily
cronhourly

Each with (hopefully) obvious meaning, and internally depending on virtual/cron if either is set, and installing into appropriate locations, preferring cronhourly if set, and ignoring crondaily in that case. I think this is also a somewhat sensible meaning, but some users may see the conflict here and query that. So whatever the concensus is, I'm happy with, even a "go away" (which will just force the -cron situation for those few servers where we need cron.hourly).

@jkroonza
Copy link
Contributor Author

For anybody else reading this looking for a solution now:

in make.conf:

INSTALL_MASK="${INSTALL_MASK} /etc/cron.daily/logrotate"

Then:

mv /etc/cron.daily/logrotate /etc/cron.hourly/

Downside: if there are ever changes to the cron*/logrotate file, you won't get them. Looking at the file this seems fairly unlikely at least.

@floppym
Copy link
Contributor

floppym commented Jun 19, 2024

The INSTALL_MASK entry should be unnecessary. CONFIG_PROTECT will prevent the cron.daily file from being reinstalled until the user runs dispatch-conf.

@jkroonza
Copy link
Contributor Author

The INSTALL_MASK entry should be unnecessary. CONFIG_PROTECT will prevent the cron.daily file from being reinstalled until the user runs dispatch-conf.

OK, so doing what I did was slightly overkill, either way, I still won't get the benefit of updates to the cron*/logrotate file. If the conclusion is that moving the file and then relying on etc-update to not clobber a non-existent file, unless the file has changed, and then hoping the user remembers that after dispatch-conf/etc-update the file will need to be moved again, so be it.

I'm actually thinking that a USE_EXPAND like CRON_FREQUENCY with options for daily,hourly,weekly that's more generally applicable to things that need to install into /etc/cron.*/ (not cron.d) may not be a bad idea, so please advise how I should proceed.

@floppym
Copy link
Contributor

floppym commented Jun 27, 2024

I don't think it makes sense to add USE flags for every possible frequency of cron execution.

I think my personal preference would be to drop the "cron" USE flag entirely, and instead install logrotate.cron in a location that is not executed by default. We would then leave it to the system administrator to enable it by copying or symlinking the script, or configuring it to run via crontab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits.
Projects
None yet
5 participants