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

allow several matches for one log file, most specific wins #38

Open
CoolCold opened this issue Jun 4, 2016 · 37 comments
Open

allow several matches for one log file, most specific wins #38

CoolCold opened this issue Jun 4, 2016 · 37 comments
Labels

Comments

@CoolCold
Copy link

CoolCold commented Jun 4, 2016

I'd be nice to allow (and not produce error message) in case of several matches for log. For example, i have /var/log/nginx/ dir, where every nginx log is going. Some specific logs, which are used for statistics collection (machine parsable format, for example), can be rotated everyday, and kept for 1 day only. But if i produce two entires, like:

"/var/log/nginx/server.com.access.reqsize.log" {
      daily
..
}

and

/var/log/nginx/*.log {
        weekly
...}

i got the error on stderr (but exit code is 0) about duplication.
Most specific file match (pattern length which matched?) should win and do not produce errors.

@johnlinp
Copy link

Hi, I would like to take on this feature and implement it.

I will define the following functions:

  1. compareGlobPattern(pattern1, pattern2): it compares 2 glob patterns, and returns 1 if pattern1 is more specific than pattern2, returns -1 if pattern2 is more specific than pattern1, returns 0 if the comparison is not possible.
  2. compareGlobPatternGroup(patternGroup1, patternGroup2): it compares 2 glob pattern groups, and returns 1 if one of the pattern in patternGroup1 is more specific than all of the patterns in patternGroup2, returns -1 vice versa, returns 0 if no pattern is more specific than all the patterns in the other pattern group.

For example, the following 2 configs:

/var/log/a.* /var/log/a.b.c.* {
   daily
...
}

/var/log/a.b.* /var/log/a.b.c.d.* {
   weekly
...
}

They makes 2 pattern groups:

patternGroup1: /var/log/a.*, /var/log/a.b.c.*
patternGroup2: /var/log/a.b.* /var/log/a.b.c.d.*

patternGroup2 will win because /var/log/a.b.c.d.* is more specific than all the patterns in patternGroup1. Therefore, the log file /var/log/a.b.c.d.e will be rotated weekly.

Any comments would be appreciated.

@kdudka
Copy link
Member

kdudka commented Dec 19, 2017

I do not think we can (or want to) define strict order on these patterns. How would you compare /var/log/a.* with /var/log/*.b?

@johnlinp
Copy link

No, I didn't have the intention to do so. In your example, I will treat it as duplicate configs, just like before.

@johnlinp
Copy link

More specifically, I will define compareGlobPattern(pattern1, pattern2) as the following logic:

  1. If pattern1 and pattern2 are the same, return 0.
  2. Use pattern1 as a pattern and use pattern2 as a normal string to do a glob matching.
  3. Use pattern2 as a pattern and use pattern1 as a normal string to do a glob matching.
  4. If the above 2 glob matches:
    a. both fails: return 0 since they are not comparable. e.g. /var/log/a.* and /var/log/*.b.
    b. both are success: impossible.
    c. only the first one is success: return -1
    d. only the first one is success: return 1

@wyohm
Copy link

wyohm commented Jan 23, 2018

It might be simpler to evaluate the patterns in order, and ignore files which have already been rotated. Thus the original example:

"/var/log/nginx/server.com.access.reqsize.log" {daily}
/var/log/nginx/*.log {weekly}

So the above would rotate ...reqsize.log daily and everything else weekly. This puts the creator of the config file in control of the order with little guesswork, and fits well with the logrotate style of declaring directives (e.g. compress) generally, then undoing them in specific cases (nocompress).
It could even allow for directives like:

/var/log/* {daily}

to be put in /etc/logrotate.conf as a catch-all after the includes. Especially if used in combination with Recursion support.

@johnlinp
Copy link

@wyohm Your idea is better. Thanks for suggesting that.

@kdudka What do you think?

@rahulshw
Copy link

I like @wyohm's idea.

@kdudka
Copy link
Member

kdudka commented Feb 6, 2018

Yes, it sounds reasonable. Sorry for the delay!

@johnlinp
Copy link

johnlinp commented Feb 7, 2018

@kdudka I will start working on it. Do you think I should add an option (e.g. --ignore-duplicates) for this behavior, or just make it as the default behavior?

johnlinp pushed a commit to johnlinp/logrotate that referenced this issue Feb 7, 2018
johnlinp added a commit to johnlinp/logrotate that referenced this issue Feb 7, 2018
@johnlinp
Copy link

johnlinp commented Feb 7, 2018

I submitted my draft pull request. Please tell me if the patch needs modified (coding style, unit tests, ...), thanks.

@wyohm
Copy link

wyohm commented Feb 7, 2018

Could this be a directive so that it only applies to the current config file?

@kdudka
Copy link
Member

kdudka commented Feb 8, 2018

@johnlinp Thank you for working on it! I agree with @wyohm that a configuration directive would be better as it would be consistent with how tabooext and taboopat directives work. It would also ease the review if you showed us some examples that you have been using for testing it. Adding them directly as regression tests would be even better!

@johnlinp
Copy link

johnlinp commented Feb 9, 2018

@wyohm @kdudka OK, I will update my pull request for that.

Specifically, a config file like this:

/var/log/aaa.log {
   weekly
}

/var/log/*.log {
    daily
    ignore-duplicates
}

will rotate aaa.log weekly and everything else daily.

@wyohm
Copy link

wyohm commented Feb 9, 2018

Great, thanks. However, you may want to rethink the name of the directive. I believe ignore-duplicates would be the only directive with a dash in it. I haven't looked, but that may make parsing more difficult. Also, no-ignore-duplicates to turn off the behavior for an included config sounds complicated.

May I suggest wildcardsweep and nowildcardsweep?
Other possibilities: allowoverlap, duplicatecontinue, allowduplicate.

@rahulshw
Copy link

rahulshw commented Feb 9, 2018

allowduplicate and noallowduplicate looks better to me.

@johnlinp
Copy link

johnlinp commented Feb 9, 2018

I am not an English native speaker, so I am not sure if this is correct. I think allowduplicate is not accurate because the when it encounter a duplicate log file, the log file is ignored and skipped instead of allowed or rotated.

So I propose skipduplicates and noskipduplicates.

@wyohm
Copy link

wyohm commented Feb 9, 2018

It depends on what you're considering duplicated: the log file, or the rule. The error message:
error: /etc/logrotate.conf:45 duplicate log entry for /var/log/messages
seems to indicate more that it's talking about the rule, but it's quite open to interpretation.

@johnlinp
Copy link

johnlinp commented Feb 9, 2018

@kdudka What do you say?

@kdudka
Copy link
Member

kdudka commented Feb 9, 2018

allowduplicates sounds too generic to me. It could be easily confused with duplicated log files, config files, directives, etc.

What about allowgloboverride?

@johnlinp
Copy link

johnlinp commented Feb 9, 2018

allowgloboverride sounds better to me.

@wyohm
Copy link

wyohm commented Feb 9, 2018

Or simply globoverride.

@kdudka
Copy link
Member

kdudka commented Feb 9, 2018

Yes, globoverride works for me, too.

@johnlinp
Copy link

OK, I'll use globoverride.

johnlinp added a commit to johnlinp/logrotate that referenced this issue Feb 13, 2018
@johnlinp
Copy link

I had my second version pushed. Please give me some comments. An example with globoverride is in the regression test.

@kdudka
Copy link
Member

kdudka commented Oct 25, 2018

What should we do with sharedscripts?

As it works now, each configuration section is associated with a set of globs and the actual set of files produced by expansion of the globs. If a prerotate/postrotate script runs with sharedscripts, it gets a line-separated list of globs as its first argument. However, if we implement the globoverride feature, there will not always be 1:1 mapping between the sets of globs and sets of files.

If we only update the set of files and keep the original set of globs while overriding a previous configuration, some files might be processed by multiple prerotate/postrotate scripts when sharedscripts is enabled. That is certainly not what the user expects. They should be able to override previously specified prerotate/postrotate script instead. The problem is that the current script interface is simply not ready for that.

Would it make sense to pass line-separated list of files (instead of globs) to prerotate/postrotate scripts when both sharedscripts and globoverride are in effect?

@voodoodemon
Copy link

This issue seems to have gotten left by the wayside, are there still plans in the works to implement this?

@kdudka
Copy link
Member

kdudka commented Aug 20, 2019

While analyzing all the consequences of the proposed extension, it turned out that it is more difficult to implement it properly than it had originally seemed to. Obviously, logrotate has been evolving for a long time without keeping such extensions in mind. Some decisions incompatible with this proposal have been made in the past (the sharedscripts directive is a good example). So it is difficult now to extend logrotate in a backward-compatible way. I am not proactively working on this proposal myself any more.

@iliv
Copy link

iliv commented Nov 22, 2019

That's too bad, @kdudka. This would be such an improvement.

Has anyone considered addressing this problem from a different perspective? E.g. by switching to /bin/bash (or adding optional support for it) with extglob option on, it would be possible to exclude certain patterns by just writing a glob.

E.g. /var/log/central/*/*/*/!(cookie).log would exclude all /var/log/central/*/*/*/cookie*.log files.

I've scoured the Internet in an attempt to find an elegant way to exclude files like suggested by this extglob example but eventually ended up here only to learn that the effort to address this issue was abandoned.

Meanwhile, Stack Overflow and other places suggest ugly hacks that some people do use.

For example:

prerotate
      bash -c "[[ ! $1 =~ cookie ]]"
endscript

Which may work in some scenarios but not with patterns as in the example of the topic starter.

@otheus
Copy link

otheus commented Nov 17, 2022

This is really crazy. After 6 years, there's got to be a straight-forward solution here. Can we have a discussion forum in which the high level functioning is completely described, and in another thread or page, summarize the desired behavior? It seems to me that @kdudka has mentioned the specific behavior, but it's not easy to find in these issue threads. There have been several good points made here by many. A recent pull request ("strong") is a very good idea except for the name of the option. It also certainly is a much needed band-aid, but might its implementation come into conflict with the "right" way to do things and to other needs?

My question is this: why is it even a fatal error when a log entry is matched multiple times?? Was this always the case for 20 years? What would happen if it simply stops being an error, but a warning instead?

If it stops being an error, how would the current code handle it? Would it override it with the last one to match? If so, simply document that behavior and be done with it.

Someone mentioned that such behavior could be a problem for environments that tend to use things like ".local" file-naming conventions for configuration. I don't see how ignoring the duplicity could be a problem here.

If that change I suggest later proves to be a problem, then you add a global switch which has several flavors:

  1. fatal error
  2. pick the first that matches and ignore the rest
  3. pick the last match, applying only the final-matching block's overrides

Do NOT let this progress be obstructed by difficult questions such as shared-script handling-with-inheritance / glob-override. Flip a coin and decide how it will go. In the future, scripts should be named with their own blocks indepdenent of fileglobs, and then fileglob-blocks should refer to the scripts by name. But really, we're getting ahead of ourselves.

@kdudka
Copy link
Member

kdudka commented Nov 25, 2022

The previous comment refers to pull request #473, which might be the ultimate solution for use cases like this. While it is not what this issue originally asked for, the proposed solution can be implemented in a reasonable time frame and without breaking backward compatibility.

@gkapad
Copy link

gkapad commented Mar 23, 2023

i think that since the 1st post of @CoolCold , the thread could have gone way more simple.
i totaly agree with @otheus.

  • 1st since exit code == 0, this should be a warning not an error just to point out duplication.
  • Even the error, things run as should be, if ...
  • ... you have define the specific log fisrt.
  • let it like this, its a logic, also simple as should be.

what confused me, and make me realised the above the hard way, was that part of man page:

logrotate reads everything about the log files it should be handling from the series of configuration files specified on the command line. Each configuration file can set global options (local definitions override global ones, and later definitions override earlier ones) and specify logfiles to rotate. Global options do not affect preceding include directives. A simple configuration file looks like this: ....

while that is true about options is exactly the oposite for current logic handling the log target definition.
So this should strictly mention in man page, or change the logic to fit the above, that in my opinion it's too late and would cause problems.

I use: Debian 11 / logrotate 3.18.0-2+deb11u1

@kdudka
Copy link
Member

kdudka commented Mar 23, 2023

@gkapad Thanks for feedback! Unfortunately, I do not understand what exactly is confusing in the man page. Could you please open a merge request with the proposed changes of the documentation?

@kdudka
Copy link
Member

kdudka commented Mar 23, 2023

Note that the ignoreduplicates directive was introduced in logrotate-3.21.0.

@gkapad
Copy link

gkapad commented Mar 23, 2023

@gkapad Thanks for feedback! Unfortunately, I do not understand what exactly is confusing in the man page. Could you please open a merge request with the proposed changes of the documentation?

Cant check 3.21, but see ignoreduplicates and how logrotate works before that commit, seems to me that ignoreduplicates just stop emmiting an error in stdout. If i'm wrong please say.

for example, in my debian's version ( 3.18.0-2),

  • /var/log/this.log { .... }
  • /var/log/*.log {}
    ^ while debugging an error print-out, but besides this, this.log was the winner and logrotate continue its job. So no matter the setting ignoreduplicates (that i dont have anyway), we could say later definitions DONT override earlier ones.

Describing the configuration file, still here: https://github.com/logrotate/logrotate/blob/master/logrotate.8.in#L130 , says

Each configuration file can set global options (local definitions override global ones, and later definitions override earlier ones) and specify logfiles to rotate.

All i want to say is:
While definding logs and defining options is 2 different things, there is a reverse logic between these 2, that is good to point it out and be told somewhere early, cause in my case because of man page, i thought later definitions override earlier ones was also for log definitions.

@kdudka
Copy link
Member

kdudka commented Mar 24, 2023

If I remember correctly, the problem was not with a warning/error being printed. The main problem was that, without ignoreduplicates, the non-conflicting logs from the /var/log/*.log glob would not be rotated at all.

@kdudka
Copy link
Member

kdudka commented Mar 24, 2023

i thought later definitions override earlier ones was also for log definitions.

I think the above statement actually means this:

/var/log/xxx.log {
    compress
    # [...]
    nocompress    # overrides compress
}

@shantanugadgil
Copy link

I have stumbled upon this GitHub issue as I was recently hit by this same logrotate issue.

I have a custom logrotate file, which is a collection of files specific to my system.
This collection of files clashes (overlaps) with many OS specific config files.

So, instead of fixing the duplicates, what I do is to setup a cron job using my "custom" file as the parameter.

This seems to work for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants