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

[Logrotate] Add log rotate configuration #15116

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

fastiuk
Copy link
Contributor

@fastiuk fastiuk commented May 17, 2023

depends-on #61

Why I did it

No be able to do the next things:

  • Perform log rotation separately for syslog and debug files
  • Configure log rotation parameters like rotation frequency, file size, file rotation count
Work item tracking
  • Microsoft ADO (number only):

How I did it

  • Add YANG model for a new configuration
  • Extend configuration templates to support new knobs

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Tested branch (Please provide the tested image version)

Description for the changelog

  • Add the ability Perform log rotation separately for syslog and debug files
  • Configure log rotation parameters like rotation frequency, file size, file rotation count

Link to config_db schema for YANG module changes

Logrotation config

A picture of a cute animal (it is my cat Finn)

PXL_20230413_140941610 PORTRAIT

@dgsudharsan dgsudharsan added the YANG YANG model related changes label May 18, 2023
files/build_templates/init_cfg.json.j2 Outdated Show resolved Hide resolved
@@ -8,7 +49,6 @@
/var/log/kern.log
/var/log/user.log
/var/log/lpr.log
/var/log/debug
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

files/image_config/logrotate/logrotate-config.sh Outdated Show resolved Hide resolved
files/image_config/logrotate/logrotate-config.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@dgsudharsan dgsudharsan left a 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

@@ -0,0 +1,13 @@
{# Don't use that file to generate something, it is just for holding defaults #}
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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

@fastiuk fastiuk force-pushed the dev-logrotate-config branch from 586be93 to 2c99088 Compare July 4, 2023 22:09
@fastiuk fastiuk self-assigned this Jul 11, 2023
@fastiuk fastiuk force-pushed the dev-logrotate-config branch from e8258c1 to 2d17ed8 Compare July 22, 2023 11:12
@Meir-renford
Copy link

We reviewed the code change needed, and will address it in the upcoming 2 weeks.

fastiuk added 2 commits April 28, 2024 02:06
* Separate debug log rotation config
* Separate syslog log rotation config
* Add CONFIG_DB LOGGING table to hold logging
configuration

Signed-off-by: Yevhen Fastiuk <[email protected]>
@fastiuk fastiuk force-pushed the dev-logrotate-config branch from f7fad71 to 7f89174 Compare April 27, 2024 23:07
@liat-grozovik
Copy link
Collaborator

@dgsudharsan any further comments?

files/build_templates/init_cfg.json.j2 Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
{# Don't use that file to generate something, it is just for holding defaults #}
Copy link
Collaborator

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
Copy link
Collaborator

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.

files/image_config/logrotate/logrotate-config.sh Outdated Show resolved Hide resolved
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 \
Copy link
Collaborator

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?

Copy link
Contributor Author

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 }}
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgsudharsan
Copy link
Collaborator

@dgsudharsan any further comments?

@liat-grozovik Left few comments.

@fastiuk
Copy link
Contributor Author

fastiuk commented Aug 14, 2024

/azpw run Azure.sonic-buidimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buidimage

Copy link

No pipelines are associated with this pull request.

@fastiuk fastiuk closed this Aug 14, 2024
@fastiuk fastiuk reopened this Aug 14, 2024
@fastiuk fastiuk closed this Aug 19, 2024
@fastiuk fastiuk reopened this Aug 19, 2024
@fastiuk fastiuk closed this Aug 22, 2024
@fastiuk fastiuk reopened this Aug 22, 2024
@fastiuk
Copy link
Contributor Author

fastiuk commented Aug 28, 2024

Tests will fail until sonic-net/sonic-host-services#61 is fixed

@liat-grozovik
Copy link
Collaborator

/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
Copy link
Contributor

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?

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

Successfully merging this pull request may close these issues.

8 participants