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 logrotate feature HLD #1683

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

Conversation

fastiuk
Copy link
Contributor

@fastiuk fastiuk commented Apr 27, 2024

This document provides general information about log rotate implementation in SONiC

The scope of this document is to cover definition, design and implementation of SONiC log rotate feature and related CLI.

PR title state context
[Logrotate] Add log rotate configuration GitHub issue/pull request detail GitHub pull request check contexts
[Logrotate] Update log rotate configuration via ConfigDB GitHub issue/pull request detail GitHub pull request check contexts
[Logrotate] Add log rotate configuration tables GitHub issue/pull request detail GitHub pull request check contexts
[Logrotate] Add config and show commands GitHub issue/pull request detail GitHub pull request check contexts

Copy link

linux-foundation-easycla bot commented Apr 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
@fastiuk fastiuk force-pushed the dev-logrotate-config branch from f6d8a33 to af5b905 Compare April 27, 2024 22:11
@fastiuk fastiuk changed the title Added logrotate feature HLD Add logrotate feature HLD Apr 27, 2024
@fastiuk fastiuk changed the title Add logrotate feature HLD [Logrotate] Add logrotate feature HLD Apr 27, 2024
@zhangyanzhao
Copy link
Collaborator

HLD is reviewed in community on 5/14/2024

@zhangyanzhao
Copy link
Collaborator

call for reviewers, please leave your comments if you want to be a reviewer of this HLD. Thanks.

doc/logrotate/logrotate_hld.md Outdated Show resolved Hide resolved
doc/logrotate/logrotate_hld.md Outdated Show resolved Hide resolved
doc/logrotate/logrotate_hld.md Outdated Show resolved Hide resolved
It reads log rotate configuration from database and apply it to log rotate config files.


**The log rotate config files will be used:**
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the default log rotation configuration?

**The log rotate config files will be used:**
1. /etc/logrotate.d/rsyslog - generic log files
2. /etc/logrotate.d/debug - debug log files
3. /etc/logrotate.d/syslog - syslog files
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mentioning the contents of these files would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, while syslog is obvious, what files fall under debug log files? What about generic log files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump on this

Copy link
Contributor

Choose a reason for hiding this comment

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

@fastiuk could you respond on this?


**User interface**:
```
config
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 should be able to revert back the default value



**The log rotate config files will be used:**
1. /etc/logrotate.d/rsyslog - generic log files
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarify the content and maybe change the name for an old file

@zhangyanzhao
Copy link
Collaborator

fastiuk added 3 commits May 21, 2024 00:17
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
@fastiuk fastiuk self-assigned this May 21, 2024
@prgeor prgeor requested a review from saiarcot895 June 11, 2024 06:44
@prgeor
Copy link
Contributor

prgeor commented Jun 11, 2024

@saiarcot895 could you review?

@liat-grozovik
Copy link
Collaborator

we need to expedite this review. the PRs are ready for more than 3 months
@zhangyanzhao if not additional feedback, the PR should be merged by end of this week.

@fastiuk
Copy link
Contributor Author

fastiuk commented Aug 6, 2024

@liat-grozovik can we merge this? No further comments were provided.

Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>
@fastiuk
Copy link
Contributor Author

fastiuk commented Aug 28, 2024

@saiarcot895 all comments were addressed, can I get an approval?

3 similar comments
@fastiuk
Copy link
Contributor Author

fastiuk commented Oct 21, 2024

@saiarcot895 all comments were addressed, can I get an approval?

@fastiuk
Copy link
Contributor Author

fastiuk commented Nov 4, 2024

@saiarcot895 all comments were addressed, can I get an approval?

@fastiuk
Copy link
Contributor Author

fastiuk commented Dec 2, 2024

@saiarcot895 all comments were addressed, can I get an approval?

@saiarcot895
Copy link
Contributor

@fastiuk There's an open discussion item above.

@zhangyanzhao
Copy link
Collaborator

PRs are not merged, move to backlog

@liat-grozovik
Copy link
Collaborator

@zhangyanzhao i disagree
The document was shared along time ago and qa reviewed
2 out of the PRs were already merged
You cannot just put things in the backlog
The maintainers need to be responsive and allow changes to get in

I will work with the author and the reviewers the complete and I do expect that to be in 202411

@fastiuk please check what is still not approved form PRs pov and make sure the checkers are passing for the missing PRs

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

Successfully merging this pull request may close these issues.

7 participants