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

GSoC'22: Log File Rotation #4145

Closed

Conversation

shikharvashistha
Copy link
Contributor

@shikharvashistha shikharvashistha commented Sep 20, 2022

Description

This pull request is intended to introduce a log file rotation mechanism for the syslog-ng application which will solve the current issue of log files becoming very large and thus affecting the performance of the application.

Overview of changes

The following changes have been made in order to implement the solution:

  • LogProtoFileWriter class have been modified to connect to the existing SignalSlotConnector class in order to receive notifications via log_proto_file_writer_new() function when a log file is opened and will carry filename and reopener pointer as user_data.

  • A new slot have been added to the SignalSlotConnector class which will receive notifications when a log file is opened and will call a new function log_proto_file_writer_reopen() in order to check if the opened file needs to be rotated and all the necessary logic is applied to rotate the log file for which the file-rotation functionality was built.

Configuration snippets

Whole configuration file

#############################################################################
# Default syslog-ng.conf file which collects all local logs into a
# single file called /var/log/messages.
#

@version: 3.37
@include "scl.conf"

destination d_file {
    file("/tmp/temp.log", file-rotation(size(0KB), interval("daily"), dateformat("-%Y-%m-%d"), rotate(2)));
};

source my_tcp {
    stdin();
};


log {
source(my_tcp);
destination(d_file);
};

Necessary changes

destination d_file {
    file("/tmp/temp.log", file-rotation(size(0KB), interval("daily"), dateformat("-%Y-%m-%d"), rotate(2)));
};

Alternatives/Problem faced

  1. lseek vs fstat call. Can refer the discussion here

  2. Attaching a reopner function to the LogProtoFileWriter class vs using a global flag to signal file-rotation plugin to rotate the file.

Discussions

Resolves #2964

MrAnno and others added 19 commits September 20, 2022 12:48
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
This grammar allows specifying the file-rotation() option inside
the file destination, and it accepts an option called size().

With this commit, syslog-ng can be started with the following config,
but it will stop without reporting any error as the plugin is still empty/
unimplemented.

log {
    source { stdin(); };

    destination {
        file("/tmp/test" file-rotation(size(100M)));
    };
};

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
This commit adds an empty implementation for the file rotation plugin
with a debug message, which can be seen if syslog-ng is started in debug
mode:

$ sbin/syslog-ng -Fedv
  ...
  File rotation plugin has been attached; to='...'

Signed-off-by: László Várady <laszlo.varady@protonmail.com>
This commit adds signal to the file destination involving passing AFFileDestDriver's instance signal slot connector to the LogProtoFileWriter

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit introduces new option to set interval for log rotation

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit introduces the controller function for updating/set log rotation interval & Adds support code for connecting AFFileDestDriver plugin's signal_slot_connector to our plugin

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
…in test for set_interval

This commit updates the location of plugin's request payload and the SIGNAL which is required to be emitted

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit assigns the AFFileDestDriver signal_slot_connector to the logproto's file writer struct

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
Signed-off-by: László Várady <laszlo.varady@protonmail.com>
This commit introduces functionality for setting up date format based of last_rotation_time

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit includes the file size checking and renaming if it exceeds the desired size

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit introduces few fixes to file size checking and rotation including reopening the log file based of the request

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit adds functional test for file-rotation plugin

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
Added file reopener struct to file-signal.h

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit adds reopening of a file via a flush signal

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit adds the functionality to reopen the destination file via reopener

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit introduces a new option to set number of file rotations possible and few validation checks

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
This commit introduces checking file everytime on disk

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

1 similar comment
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@github-actions
Copy link
Contributor

No news file has been detected. Please write one, if applicable.

@MrAnno
Copy link
Collaborator

MrAnno commented Sep 20, 2022

@kira-syslogng ok to test

@MrAnno
Copy link
Collaborator

MrAnno commented Sep 20, 2022

@kira-syslogng do stresstest

@shikharvashistha shikharvashistha changed the title File rotation GSoC'22: Log File Rotation Sep 20, 2022
@kira-syslogng
Copy link
Contributor

Kira-stress-test: Build SUCCESS

@MrAnno MrAnno requested review from bazsi and MrAnno September 29, 2022 15:43
@@ -218,7 +234,8 @@ log_proto_file_writer_prepare(LogProtoClient *s, gint *fd, GIOCondition *cond, g
}

LogProtoClient *
log_proto_file_writer_new(LogTransport *transport, const LogProtoClientOptions *options, gint flush_lines, gint fsync_)
log_proto_file_writer_new(LogTransport *transport, const LogProtoClientOptions *options, gint flush_lines, gint fsync_,
SignalSlotConnector *connector, const gchar *filename, FileReopener reopener)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the reopener should probably be a pointer here. Also I'd look into encapsulating the last three arguments into
a struct of some kind, which all relates to rotation support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sure

proto = file_opener_construct_dst_proto(self->owner->file_opener, transport,
&self->owner->writer_options.proto_options.super);
&self->owner->writer_options.proto_options.super, self->owner->super.super.super.signal_slot_connector, self->filename,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the slot is probably better placed in the FileOpener constructor so that if an implementation of construct_dst_proto needs it it can find it in self and we wouldn't need to change the prototype here.

I guess this only applies to regular files and not so much to /proc/kmsg or named pipes.

struct _FileFlushSignalData
{
const gchar *filename;
FileReopener *reopener;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could allow the file opener plugin to indicate that a reopen is needed via a return value, so we wouldn't have to push the FileReopener instance down to the guts of the rotation plugin.

This way the reopen would either be delegated to the LogProtoFileWriter or even better back to the driver via the use of the log_pipe_notify() mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that we can have a returning flag of some sort for the purpose of reopening the file ?

This commit introduces checking file everytime on disk

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
@bazsi
Copy link
Collaborator

bazsi commented Oct 19, 2023 via email

@tuxillo
Copy link

tuxillo commented Aug 22, 2024

What happened to this ?

@mehul-m-prajapati
Copy link
Contributor

@HofiOne @bazsi : I can work on the remaining part of this feature. Do you want to have this log rotation ?

@czanik
Copy link
Collaborator

czanik commented Sep 30, 2024

Thanks @mehul-m-prajapati for your offer. However, we discussed this question internally, and we do not want to continue this effort. If we ever get to this, it will be a different implementation.

@czanik czanik closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rotate log files based on size
8 participants