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

Migrate path of logrotate.d configration #505

Merged
merged 4 commits into from
Jul 5, 2023

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Jun 26, 2023

Before: /etc/logrotate.d/td-agent
  If td-agent user/group exists, renaming td-agent
  to fluent user/group then rewrite logrotate.d configuration.
  But, it may fail if user modify that configuration unexpected
  way.

After: /etc/logrotate.d/fluentd

  If td-agent user/group exists (e.g upgrading from v4), create
  _fluent user/group as same uid/gid. If not, just create new _fluent
  user/group (no td-agent user/group).

  This approach make easy to migrate /etc/logrotate.d/td-agent
  configuration file without modification.

Additionally, rewrite /etc/logrotate.d/td-agent path of pid
because the pid file will be dynamically created/destroyed.

@kenhys kenhys marked this pull request as ready for review June 26, 2023 06:11
@daipom daipom self-requested a review June 28, 2023 04:08
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

Thanks for this fix!
Indeed, we have to update the file /etc/logrotate.d for the new package.
Do we also need to update the USER and GROUP name to _fluentd for the deb package?

For migration, I have the following concerns.

  • Shouldn't the old rotate config /etc/logrotate.d/td-agent work as is after the update?
    • Because we have the alias to access the old path: /var/log/td-agent/td-agent.log
  • So, I think we should not make the new rotate config, and continue to use the old one as is.
    • Possibly, we need to migrate USER and GROUP settings, but the other migration process seems to be not needed.

I confirmed the alias /var/log/td-agent work as expected for rotation:

/etc/logrotate.d/td-agent

/var/log/td-agent/td-agent.log {
  #daily
  size 1
  rotate 30
  compress
  delaycompress
  notifempty
  create 640 _fluentd _fluentd
  sharedscripts
  postrotate
    pid=/var/run/fluent/fluentd.pid
    if [ -s "$pid" ]
    then
      kill -USR1 "$(cat $pid)"
    fi
  endscript
}

log files:

/var/log/fluent/td-agent.log

Run rotate: $ sudo logrotate -s state /etc/logrotate.d/td-agent

rotated log files:

/var/log/fluent/td-agent.log
/var/log/fluent/td-agent.log.1

@kenhys
Copy link
Contributor Author

kenhys commented Jun 28, 2023

It may be a simple solution to do:

1st approach PR:

  • Ship /etc/logrotate.d/fluentd
  • Rewrite to old path in /etc/logroate.d/fluentd
  • Remove /etc/logrotate.d/td-agent not to conflict with

2nd approach PR:

  • Ship /etc/logrotate.d/flentd (it use new path)
    • thus, both of /etc/logrotate.d/td-agent and /etc/logrotate.d/fluentd is effective.
  • Replace user/group in /etc/logrotate.d/td-agent

3rd approach:

  • Instead of renaming td-agent user/group, add _fluentd/_fluentd user/group which uid is same as td-agent.

@kenhys
Copy link
Contributor Author

kenhys commented Jun 28, 2023

Try 3rd approach whether it will work as expected.

@kenhys kenhys force-pushed the migrate-logrotate branch 3 times, most recently from fc60961 to 450f959 Compare June 29, 2023 09:43
@kenhys
Copy link
Contributor Author

kenhys commented Jun 30, 2023

3rd approach is not bad, but tend to be broken on some deb platform. 🤔
we need to tackle on another PR.

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

I confirmed this basically works as expected on Ubuntu Focal.

There was one problem about SIGUSR1 signal on rotation.
I commented on it below.

kenhys added 3 commits July 4, 2023 16:48
Before: /etc/logrotate.d/td-agent
  If td-agent user/group exists, renaming td-agent
  to fluent user/group then rewrite logrotate.d configuration.
  But, it may fail if user modify that configuration unexpected
  way.

After: /etc/logrotate.d/fluentd

  If td-agent user/group exists (e.g upgrading from v4), create
  _fluent user/group as same uid/gid. If not, just create new _fluent
  user/group (no td-agent user/group).

  This approach make easy to migrate /etc/logrotate.d/td-agent
  configuration file without modification.

Additionally, rewrite /etc/logrotate.d/td-agent path of pid
because the pid file will be dynamically created/destroyed.

Signed-off-by: Kentaro Hayashi <[email protected]>
Signed-off-by: Kentaro Hayashi <[email protected]>
Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys force-pushed the migrate-logrotate branch from 450f959 to cc380ae Compare July 4, 2023 08:15
@kenhys
Copy link
Contributor Author

kenhys commented Jul 4, 2023

Added fix for logrotate.d/td-agent

@daipom
Copy link
Contributor

daipom commented Jul 4, 2023

I will confirm it!

@daipom
Copy link
Contributor

daipom commented Jul 4, 2023

Is it intentional that 43e9343 was added into this PR?

@kenhys
Copy link
Contributor Author

kenhys commented Jul 4, 2023

Is it intentional that 43e9343 was added into this PR?

It should be seperate PR.

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys force-pushed the migrate-logrotate branch from cc380ae to fcfb402 Compare July 4, 2023 09:21
Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

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

I confirmed that the SIGUSR1 worked correctly on rotation!
LGTM.

@kenhys
Copy link
Contributor Author

kenhys commented Jul 5, 2023

Thanks!

@kenhys kenhys merged commit 130f5c8 into fluent:master Jul 5, 2023
@kenhys kenhys deleted the migrate-logrotate branch July 5, 2023 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants