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

Fix logrotate config on GCI #29139

Merged
merged 1 commit into from
Jul 19, 2016
Merged

Conversation

adityakali
Copy link
Contributor

@adityakali adityakali commented Jul 18, 2016

we need to add the dateformat option so that the logrotate
can create unique logfiles for each rotation. Without this,
logrotation is skipped with message like (generated in
verbose mode of logrotate):

rotating log /var/log/rotate-test.log, log->rotateCount is 5
dateext suffix '-20160718'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
destination /var/log/rotate-test2.log-20160718.gz already exists, skipping rotation

Tested as follows:

config in '/etc/logrotate.d/rotate-test':

/var/log/rotate-test.log {
rotate 5
copytruncate
missingok
notifempty
compress
maxsize 100M
daily
dateext
dateformat -%Y%m%d-%s
create 0644 root root
}

create 150Mb of /var/log/rotate-test.log

$ dd if=/dev/zero of=/var/log/rotate-test.log bs=1048576 count=150 conv=notrunc oflag=append

run logrotate

$ /usr/sbin/logrotate -v /etc/logrotate.conf
...
rotating pattern: /var/log/rotate-test.log after 1 days (5 rotations)
empty log files are not rotated, log files >= 104857600 are rotated earlier, old logs are removed
considering log /var/log/rotate-test.log
log needs rotating
rotating log /var/log/rotate-test.log, log->rotateCount is 5
Converted ' -%Y%m%d-%s' -> '-%Y%m%d-%s'
dateext suffix '-20160718-1468875268'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
copying /var/log/rotate-test.log to /var/log/rotate-test.log-20160718-1468875268
truncating /var/log/rotate-test.log
compressing log with: /bin/gzip

Repeating 'dd' and 'logrotate' commands now generate logfiles correctly.
#27754

@bprashanth can you please review?

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jul 18, 2016
@bprashanth bprashanth assigned bprashanth and unassigned mikedanese Jul 18, 2016
daily
dateext
dateformat -%Y%m%d-%s
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment about the significance (i.e collisions, and that exceeding maxsize within a second will not rotate). A meta comment about how we expect logorate to happen will also help, something like: "logrotate daily or if the size exceeds maxsize, into a gzipped timestamped backup. Backups are discarded after ."

Copy link
Member

Choose a reason for hiding this comment

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

Why are we going backwards on maxsize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. From my testing, this is the only combination (maxsize with dateformat) that gives us expected behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. So this will stamp out >100M files every 1h on busy clusters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I tried explain a bit in the comment. PTAL.

@bprashanth
Copy link
Contributor

LGTM just comment nit

@bprashanth bprashanth added release-note-none Denotes a PR that doesn't merit a release note. cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-label-needed cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Jul 18, 2016
@bprashanth bprashanth added this to the v1.3 milestone Jul 18, 2016
@bprashanth
Copy link
Contributor

@zmerlynn fyi, Aditya ran some tests and found the old config lacking. I tested this version, looks ok.

@zmerlynn zmerlynn added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 18, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2016
@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2016
@bprashanth
Copy link
Contributor

reapplying lgtm

we need to add the dateformat option so that the logrotate
can create unique logfiles for each rotation. Without this,
we logrotation is skipped with message like (generated in
verbose mode of logrotate):

rotating log /var/log/rotate-test.log, log->rotateCount is 5
dateext suffix '-20160718'
glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
destination /var/log/rotate-test2.log-20160718.gz already exists, skipping rotation

Tested as follows:

  # config in '/etc/logrotate.d/rotate-test':
  /var/log/rotate-test.log {
    rotate 5
    copytruncate
    missingok
    notifempty
    compress
    maxsize 100M
    daily
    dateext
    dateformat -%Y%m%d-%s
    create 0644 root root
  }

  # create 150Mb of /var/log/rotate-test.log
  $ dd if=/dev/zero of=/var/log/rotate-test.log bs=1048576 count=150 conv=notrunc oflag=append

  # run logrotate
  $ /usr/sbin/logrotate -v /etc/logrotate.conf
  ...
  rotating pattern: /var/log/rotate-test.log  after 1 days (5 rotations)
  empty log files are not rotated, log files >= 104857600 are rotated earlier, old logs are removed
  considering log /var/log/rotate-test.log
    log needs rotating
  rotating log /var/log/rotate-test.log, log->rotateCount is 5
  Converted ' -%Y%m%d-%s' -> '-%Y%m%d-%s'
  dateext suffix '-20160718-1468875268'
  glob pattern '-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]'
  copying /var/log/rotate-test.log to /var/log/rotate-test.log-20160718-1468875268
  truncating /var/log/rotate-test.log
  compressing log with: /bin/gzip

  Repeating 'dd' and 'logrotate' commands now generate logfiles correctly.
@@ -77,12 +77,21 @@ function setup-logrotate() {
compress
maxsize 10M
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 would like to bring this to your attention @zmerlynn @bprashanth . The 10M size for docker-container log seems arbitrary too. But I guess we don't encourage users to log to stdout/stderr from their containers. So this may not be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I thought stdout/stderr was the default way fluentd picked up the container logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind. I recalled some discussion on google-containers group recommending to redirect logs to files.
So, is 10Mb here fine?

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2016
@zmerlynn zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2016
@zmerlynn
Copy link
Member

Yes, since fluentd should be slurping the logs up, in theory.

@bprashanth
Copy link
Contributor

@kubernetes/goog-testing I've see this error quite a bit off late:

Checking out Revision ddd9a8b6f884a020b38dfcfbfa86878955fc95a4 (origin/pr/28803/merge)
 > /usr/bin/git config core.sparsecheckout # timeout=10
 > /usr/bin/git checkout -f ddd9a8b6f884a020b38dfcfbfa86878955fc95a4 # timeout=20
FATAL: Could not checkout ddd9a8b6f884a020b38dfcfbfa86878955fc95a4
hudson.plugins.git.GitException: Could not checkout ddd9a8b6f884a020b38dfcfbfa86878955fc95a4
    at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$9.execute(CliGitAPIImpl.java:1992)

Does that mean rebase (github usually detcts rebase though right), or flake?

@spxtr
Copy link
Contributor

spxtr commented Jul 18, 2016

That's #27462, it's a flake in the Jenkins plugin.

@bprashanth
Copy link
Contributor

@k8s-bot test this github issue: #27462

@k8s-bot
Copy link

k8s-bot commented Jul 19, 2016

GCE e2e build/test passed for commit 09b2c27.

@bprashanth bprashanth added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 19, 2016
@bprashanth
Copy link
Contributor

prio 0 because 1.3

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 19, 2016

GCE e2e build/test passed for commit 09b2c27.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8b16c75 into kubernetes:master Jul 19, 2016
k8s-github-robot pushed a commit that referenced this pull request Jul 19, 2016
…9139-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #29139

Cherry pick of #29139 on release-1.3.
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@roberthbailey roberthbailey changed the title fix logrotate config (again) Fix logrotate config on GCI Jul 21, 2016
@roberthbailey roberthbailey added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 21, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…pick-of-#29139-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29139

Cherry pick of kubernetes#29139 on release-1.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants