-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Logrotate] Add log rotate configuration #15116
base: master
Are you sure you want to change the base?
Conversation
@@ -8,7 +49,6 @@ | |||
/var/log/kern.log | |||
/var/log/user.log | |||
/var/log/lpr.log | |||
/var/log/debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of differentiating debug message alone through separate configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the debug file separately, so we need to have a separation. By default that file has the same configuration as it had before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we need separate file only if debugs are much more and require special attention. I don't think that's the case in sonic as we have all debug logs as part of syslog itself and I am not sure if debug file is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to rotate it, you just don't configure it. For the rest of people who want to use debug file and rotate they can configure it in the DB and rotate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update configuration guide Configuration.md
files/build_templates/defaults.j2
Outdated
@@ -0,0 +1,13 @@ | |||
{# Don't use that file to generate something, it is just for holding defaults #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a different file for setting defaults. Can't we incorporate this in init_cfg.j2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is shared between two configuration files. I don't want to explicitly incorporate debug/syslog files in the configuration. The default values here are used to fill data if the configuration in init_cfg.j2
doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default only for syslog related configuration. I would prefer renaming the file to avoid configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to remove that file. I remember we discussed that approach with Ying Xie and decided to remove defaults from that file and use defaults in-place
586be93
to
2c99088
Compare
e8258c1
to
2d17ed8
Compare
We reviewed the code change needed, and will address it in the upcoming 2 weeks. |
11e2e6f
to
e3a3c6b
Compare
e3a3c6b
to
1e8368c
Compare
1e8368c
to
f7fad71
Compare
* Separate debug log rotation config * Separate syslog log rotation config * Add CONFIG_DB LOGGING table to hold logging configuration Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
f7fad71
to
7f89174
Compare
@dgsudharsan any further comments? |
files/build_templates/defaults.j2
Outdated
@@ -0,0 +1,13 @@ | |||
{# Don't use that file to generate something, it is just for holding defaults #} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default only for syslog related configuration. I would prefer renaming the file to avoid configuration
@@ -8,7 +49,6 @@ | |||
/var/log/kern.log | |||
/var/log/user.log | |||
/var/log/lpr.log | |||
/var/log/debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we need separate file only if debugs are much more and require special attention. I don't think that's the case in sonic as we have all debug logs as part of syslog itself and I am not sure if debug file is used.
sonic-cfggen -d -t /usr/share/sonic/templates/logrotate-debug.j2 \ | ||
-a "$APPEND_DATA" > /etc/logrotate.d/debug | ||
|
||
sonic-cfggen -d -t /usr/share/sonic/templates/logrotate-syslog.j2 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was not required previously. Why is this required now. Isn't rsyslog enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rsyslog was split into common and per-logfile config.
And the reason why it was done like that, because once config file will be generated, you can use to rotate corresponding log file manually, and not all files together.
{% set size = (size_mb * 1024 * 1024) | int -%} | ||
/var/log/syslog | ||
{ | ||
{{ frequency }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what does this frequency control? The frequency of logrotate itself is being controlled by timer. Why is frequency needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liat-grozovik Left few comments. |
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
/azpw run Azure.sonic-buidimage |
/AzurePipelines run Azure.sonic-buidimage |
No pipelines are associated with this pull request. |
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Tests will fail until sonic-net/sonic-host-services#61 is fixed |
/azpw run Azure.sonic-buildimage |
{% set size_mb = (debug.size | d(0.001)) | float -%} | ||
{%- endif %} | ||
{% set size = (size_mb * 1024 * 1024) | int %} | ||
/var/log/debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this log file is used at all on SONiC?
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
depends-on #61
Why I did it
No be able to do the next things:
syslog
anddebug
filesWork item tracking
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
syslog
anddebug
filesLink to config_db schema for YANG module changes
Logrotation config
A picture of a cute animal (it is my cat Finn)