Handle discard all logfiles properly

[hanshasselberg]

Fixes #6892.

The docs are stating:

-log-rotate-max-files - to specify the maximum number of older log
file archives to keep. Defaults to 0 (no files are ever deleted). Set to
-1 to disable rotation and discard all log files.

But the -1 case was not implemented and led to a panic when being
used.

[crhino]

I'm wondering now whether we should just make this check if l.MaxFiles <= -1, so that we handle the edge cases of setting this to -2, -10000. etc. Not really expecting anyone to seriously do that, but better safe than sorry perhaps.

[hanshasselberg]

Agreed!

[freddygv]

I feel like the docs could be updated here.

This sentence feels misleading:

Set to -1 to disable rotation and discard all log files.

If we were discarding all log files then we wouldn't log to files at all. But with the current implementation we keep a file with the latest logs and what we discard are old log files.

So maybe it should read instead:

Set to -1 to disable rotation and discard old log files.

That would be more in line with the logrotate docs:
https://linux.die.net/man/8/logrotate

If count is 0, old versions are removed rather than rotated.

Let me know what you think @i0rek and @crhino.

[crhino]

I agree, it is ambiguous about what the current docs mean. In fact, I am now questioning what it means to "disable rotation" in this instance. This PR as it currently stands will prune all old log files, but will still "rotate" the singular log file being written to.

What is the intended use case for this feature? Perhaps an operator has a large cluster of agents and wants to set this configuration and roll it out in order to remove all old log files to free up space?

If we decide this is the correct action to take on a -1, perhaps some wording such as

Set to -1 to discard all log file archives.

[hanshasselberg]

Thank you for your reviews @crhino and @freddygv!

What is the intended use case for this feature?

If you don't setup log rotation, the active log file will keep growing.
If you setup rotation (aka log-rotate-bytes), it will create the archives and the active log file is capped. But you are still accumulating files.
If you setup rotation and discard archives, the active log file is capped and you don't risk running out of space.

And the fact that you can be confident that your server never runs out of space is the usecase in my mind.

This sentence feels misleading:

Set to -1 to disable rotation and discard all log files.
So maybe it should read instead:
Set to -1 to disable rotation and discard old log files.

I agree that the docs are unclear. I tried to be even more verbose:

Set to -1 to truncate the log file when reaching -log-rotate-bytes or -log-rotate-duration, without rotating the content into archives.

What do you think?

[codecov]

Codecov Report

Merging #6945 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6945      +/-   ##
==========================================
- Coverage   65.63%   65.57%   -0.07%     
==========================================
  Files         443      443              
  Lines       53292    53296       +4     
==========================================
- Hits        34978    34948      -30     
- Misses      14092    14115      +23     
- Partials     4222     4233      +11

Impacted Files	Coverage Δ
logger/logfile.go	74.07% <100%> (+2.07%)	⬆️
agent/consul/raft_rpc.go	78.72% <0%> (-4.26%)	⬇️
command/monitor/monitor.go	52% <0%> (-4%)	⬇️
agent/consul/session_ttl.go	85.48% <0%> (-3.23%)	⬇️
agent/consul/rpc.go	77.81% <0%> (-3.01%)	⬇️
agent/consul/replication.go	85.71% <0%> (-2.39%)	⬇️
api/watch/funcs.go	73.57% <0%> (-2.08%)	⬇️
agent/consul/config_replication.go	75.51% <0%> (-2.05%)	⬇️
agent/consul/intention_endpoint.go	67.68% <0%> (-1.22%)	⬇️
command/debug/debug.go	65.97% <0%> (-1.2%)	⬇️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9003f8b...90283dc. Read the comment docs.

[ghost]

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.