Skip to content

Ignore duplicate file matches#473

Merged
kdudka merged 2 commits intologrotate:masterfrom
falk-werner:feat/ignore-duplicates
Nov 30, 2022
Merged

Ignore duplicate file matches#473
kdudka merged 2 commits intologrotate:masterfrom
falk-werner:feat/ignore-duplicates

Conversation

@falk-werner
Copy link
Contributor

@falk-werner falk-werner commented Nov 17, 2022 •
edited
Loading

Edit: changed option name from to "ignoreduplicates".

This pull request introduced the new option "ignoreduplicates", which allows to ignore duplicate file matches.

Example:

#logrotate.conf

# a specific log can be marked
/var/log/specific.log {
  size 10k
  rotate 3
  ignoreduplicates
}

# matches any other log
/var/log/*.log {
  size 100k
  rotate 3
}

Note that order is significant: the first occurrence of a unique file must be marked to achieve the desired behavior.

This feature is in response to following issues:

It is not an ideal solution for these issues, but I believe it solves the basic use cases:

  • an admin can include a common directory and provide some generic matchers afterwards (as shown above)
  • multiple matches can be specified for one log file (although not the most specific wins, but the first one)

When run in verbose mode, every duplicate issues a debug message. If ignoreduplicates is not used or it is not specified for the first occurrence, an error is issued for each duplicate (same behavior as before).

@otheus
Copy link

otheus commented Nov 17, 2022

I love the commit. But why not have an option that is indicative of what is actually going on?
--strong means pretty much anything and nothing here. A better option name would be --first-filematch-wins or (a bit ambiguous) --ignore-duplicate-file-matches

@falk-werner
Copy link
Contributor Author

falk-werner commented Nov 17, 2022

Indeed, finding names is the tricky part here and you are totally right, strong can mean anything. One early impression was ingore-duplicates. Would that be sufficiant? (ignore-duplicate-file-matches describes perfectly what is done, but it's a bit long compared to the existing options.)

@otheus
Copy link

otheus commented Nov 17, 2022

You realize this is a 6+ year old problem, you're solving! :) I want to award you with a trophy or something. Anyway, In issue #38 and related, they discuss an option, such as "ignore-duplicates". I used to always hate long options. It irked me in the early 90s when GNU utils started with their --ridiculously-long-options-that-were-short-stories. But as I've aged (or maybe as I've become stoopider), I've seen the wisdom in this. Which "duplicates" does that option refer to? Maybe it refers to the target of the file rotation? So if you have logfile.log and logfile-2022-11-17.log, and dateext is enabled, do you overwrite? Or ignore that duplicate? Maybe someday in the future, some guy will come along and add an option for that case and be confused. I dunno.

Also, take a look at my comment at the end of issue #38 . Consider if this change might break the implementations they want there. Yeah, I know it's a lot of work, but dammit. This utility is used by everyone in the RPM world.

@falk-werner falk-werner changed the title Ignore duplate file matches Ignore duplicate file matches Nov 18, 2022
@otheus
Copy link

otheus commented Nov 18, 2022

big thumbs up (from me --let's hope the guy with the keys agrees)

@kdudka
Copy link
Member

kdudka commented Nov 25, 2022

Thank you for working on this! I think this is an easy-to-implement alternative to pull request #173, which tried to solve a bigger problem but in the end failed because of some corner-cases that were nearly impossible to address in a backward-compatible way.

ignoreduplicatefilematches feels a bit long. What about ignoreduplicates instead?

I would also like to cover this by a test-case.

@kdudka kdudka mentioned this pull request Nov 25, 2022
Open
@falk-werner
Copy link
Contributor Author

falk-werner commented Nov 25, 2022

ignoreduplicatefilematches feels a bit long. What about ignoreduplicates instead?

I like ignoreduplicates very much.

I would also like to cover this by a test-case.

I added basic test just yet. The test ensures that no error occurs when the option is specified - otherwise an error occurs. I'm not sure about adding a negative test - should I?

kdudka
kdudka requested changes Nov 28, 2022
View reviewed changes
@falk-werner falk-werner requested a review from kdudka November 28, 2022 16:34
@kdudka kdudka requested a review from cgzones November 29, 2022 12:20
@kdudka
Copy link
Member

kdudka commented Nov 29, 2022

@cgzones What is your take on this?

@kdudka kdudka self-assigned this Nov 29, 2022
cgzones
cgzones approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@cgzones cgzones left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

@kdudka
Copy link
Member

kdudka commented Nov 30, 2022

@falk-werner I have squashed the fixup commits and slightly tweaked the commit messages. Could you please confirm that we can merge the current version as it is?

@falk-werner
Copy link
Contributor Author

falk-werner commented Nov 30, 2022

@kdudka I confirm, please merge it.

@kdudka
Copy link
Member

kdudka commented Nov 30, 2022

Merging, thanks!

@kdudka kdudka merged commit bce6eec into logrotate:master Nov 30, 2022
@falk-werner falk-werner deleted the feat/ignore-duplicates branch November 30, 2022 18:00
@kdudka kdudka mentioned this pull request Dec 2, 2022
Closed
@bazaah bazaah mentioned this pull request Dec 13, 2022
Closed
archlinux-github pushed a commit to archlinux/aur that referenced this pull request Dec 23, 2022
@bazaah
pkgbuild: add logrotate-ignore-dups.patch
fe1e3d3 
This fixes an unfortunate issue in the upstream's provided logrotate,
and the one which `cephadm` creates during runs.

Unfortunately, logrotate throws an error if more than one rule matches a
given file, which is the case between our '.../ceph/*.log' and the
cephadm generated one: '.../ceph/cephadm.log'.

Because the file is generated we can't really complain to the maintainer
for cephadm, and we also can't change our rule because ceph may
generate any pattern matching '<cluster>-<svc>-<id>', all of which are
user / cluster specific.

Fortunately, the most recent (as of this commit) logrotate as introduced
a workaround: 'ignoreduplicates' which does what you expect. We patch
the upstream logrotate.conf to include this keyword, fixing this
issue.

See the issue for more.

References: logrotate/logrotate#473
References: https://github.com/ceph/ceph/blob/v17.2.5/src/cephadm/cephadm#L9408
Closes: #8
Issue: bazaah/aur-ceph#8
Reported-by: snack@aur.archlinux.org
@behrmann behrmann mentioned this pull request Aug 29, 2025
Open
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kdudka kdudka kdudka approved these changes

@cgzones cgzones cgzones approved these changes

Assignees

@kdudka kdudka

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@falk-werner @otheus @kdudka @cgzones