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

server: add support for log rotation #12774

Merged
merged 2 commits into from
May 7, 2021

Conversation

hexfusion
Copy link
Contributor

@hexfusion hexfusion commented Mar 14, 2021

This PR adds the option to have lumberjack[1] manage log rotation for file paths defined in log-output. The flag --log-rotation-config-json accepts a JSON config directly reducing the footprint for enabling the feature. Currently, the implementation supports log rotation of a single file target as outlined in the example below. A similar approach is taken by kube-apiserver for audit logs.[2]

new flags

  --enable-log-rotation
  --log-rotation-config-json

example usage

etcd \ 
  --logger zap \
  --log-outputs stderr,/var/log/etcd/etcd.log \
  --enable-log-rotation \
  --log-rotation-config-json '{"maxsize": 1, "maxbackups": 5, "compress": true}'
-rw-r--r-- 1 root root  21K May  6 20:52 /var/log/etcd/etcd-2021-05-07T00-52-07.344.log.gz
-rw-r--r-- 1 root root  11K May  6 20:52 /var/log/etcd/etcd-2021-05-07T00-52-14.004.log.gz
-rw-r--r-- 1 root root 9.8K May  6 20:52 /var/log/etcd/etcd-2021-05-07T00-52-20.523.log.gz
-rw-r--r-- 1 root root  10K May  6 20:52 /var/log/etcd/etcd-2021-05-07T00-52-26.902.log.gz
-rw-r--r-- 1 root root  11K May  6 20:52 /var/log/etcd/etcd-2021-05-07T00-52-34.252.log.gz
-rw-r--r-- 1 root root 323K May  6 20:52 /var/log/etcd/etcd.log

[1] http://github.com/natefinch/lumberjack/
[2] https://github.com/kubernetes/apiserver/blob/fa8a40ce7f57e4ffeb1b811dca2dbed9209d7795/pkg/server/options/audit.go#L27

@codecov-io
Copy link

codecov-io commented Mar 14, 2021

Codecov Report

Merging #12774 (84d8c03) into master (0c1e6d0) will decrease coverage by 1.21%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12774      +/-   ##
==========================================
- Coverage   64.23%   63.02%   -1.22%     
==========================================
  Files         404      415      +11     
  Lines       32504    32321     -183     
==========================================
- Hits        20880    20371     -509     
- Misses       9490     9910     +420     
+ Partials     2134     2040      -94     
Flag Coverage Δ
all 63.02% <28.57%> (-1.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/embed/config.go 49.83% <ø> (ø)
server/embed/config_logging.go 50.67% <21.05%> (-5.05%) ⬇️
server/etcdmain/config.go 79.74% <100.00%> (+0.17%) ⬆️
pkg/tlsutil/cipher_suites.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/transport/sockopt_unix.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/transport/listener_opts.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/transport/timeout_listener.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/report/report.go 0.00% <0.00%> (-95.58%) ⬇️
pkg/srv/srv.go 0.00% <0.00%> (-87.94%) ⬇️
pkg/report/timeseries.go 0.00% <0.00%> (-86.77%) ⬇️
... and 124 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c1e6d0...84d8c03. Read the comment docs.

@hexfusion hexfusion force-pushed the add-log-rotate branch 3 times, most recently from 7dfe87c to 84d8c03 Compare March 14, 2021 03:03
Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

As discussed in the other comment, marking as 'Request changes' due to
"--log_rotation_lumberjack_json={...}" idea.

@hexfusion
Copy link
Contributor Author

hexfusion commented Apr 12, 2021

cc @lilic

hexfusion added a commit to hexfusion/etcd that referenced this pull request May 5, 2021
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion hexfusion requested a review from ptabor May 5, 2021 18:13
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2021

Codecov Report

Merging #12774 (06c9578) into master (eb128d2) will increase coverage by 0.89%.
The diff coverage is 31.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12774      +/-   ##
==========================================
+ Coverage   62.06%   62.95%   +0.89%     
==========================================
  Files         399      408       +9     
  Lines       32753    34243    +1490     
==========================================
+ Hits        20327    21557    +1230     
- Misses      10359    10505     +146     
- Partials     2067     2181     +114     
Flag Coverage Δ
all 62.95% <31.11%> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/embed/config_logging.go 52.70% <11.42%> (-13.39%) ⬇️
server/embed/config.go 52.27% <100.00%> (+2.44%) ⬆️
server/etcdmain/config.go 79.49% <100.00%> (-0.08%) ⬇️
client/v2/members.go 43.39% <0.00%> (-41.51%) ⬇️
client/v2/client.go 43.22% <0.00%> (-39.57%) ⬇️
etcdctl/ctlv3/command/watch_command.go 5.29% <0.00%> (-37.65%) ⬇️
client/pkg/v3/types/slice.go 66.66% <0.00%> (-33.34%) ⬇️
client/v2/keys.go 65.34% <0.00%> (-26.71%) ⬇️
server/etcdserver/api/v3rpc/member.go 92.45% <0.00%> (-3.78%) ⬇️
server/etcdserver/api/v3rpc/lease.go 71.83% <0.00%> (-2.82%) ⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb128d2...06c9578. Read the comment docs.

server/embed/config_logging.go Outdated Show resolved Hide resolved
server/embed/config_logging.go Outdated Show resolved Hide resolved
server/embed/config_logging.go Outdated Show resolved Hide resolved
server/embed/config_test.go Show resolved Hide resolved
@xiang90
Copy link
Contributor

xiang90 commented May 6, 2021

@hexfusion @ptabor

How stable is https://github.com/natefinch/lumberjack? Do we want a tight-coupled config with that implementation? Or shall we provide something simpler first (or at least not exposed the rotation implementation?)?

@xiang90
Copy link
Contributor

xiang90 commented May 6, 2021

@ptabor Does k8s support any built-in log rotation?

@hexfusion
Copy link
Contributor Author

@xiang90 lumberjack is used by kube-apiserver for audit logs[1]. That was my reason for choosing it and the direct reference to its use case in zap FAQ[2].

[1] https://github.com/kubernetes/apiserver/blob/fa8a40ce7f57e4ffeb1b811dca2dbed9209d7795/pkg/server/options/audit.go#L27
[2] https://github.com/uber-go/zap/blob/master/FAQ.md#does-zap-support-log-rotation

@hexfusion
Copy link
Contributor Author

hexfusion commented May 6, 2021

It was implemented in KAS[1] almost 4 years ago so I feel like this should be pretty stable. But I am open to alternatives of course.

[1] kubernetes/apiserver@d3c1c03#diff-a12a91ffb3cb7f3f6b159e4f146b64f72e59667fca475ec3f74e2b6039201d34R21

server/etcdmain/config.go Outdated Show resolved Hide resolved
@ptabor
Copy link
Contributor

ptabor commented May 6, 2021

@serathius has a lot of experience with observability aspects of k8s. PTAL

hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion
Copy link
Contributor Author

Updated to reflect previous comments will rebase when review is complete

server/embed/config.go Outdated Show resolved Hide resolved
server/etcdmain/config.go Outdated Show resolved Hide resolved
@xiang90
Copy link
Contributor

xiang90 commented May 7, 2021

LGTM. defer to @ptabor.

hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@ptabor
Copy link
Contributor

ptabor commented May 7, 2021

Looks good to me. Thank you.

@hexfusion hexfusion merged commit 6decbe1 into etcd-io:master May 7, 2021
@hexfusion hexfusion deleted the add-log-rotate branch May 7, 2021 16:18
hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request May 7, 2021
openshift-merge-robot added a commit to openshift/etcd that referenced this pull request May 10, 2021
Bug 1958405: UPSTREAM: <carry>: server: add support for log rotation (etcd-io#12774)
tangcong pushed a commit to tangcong/etcd that referenced this pull request May 15, 2021
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 16, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jun 17, 2021
hexfusion added a commit to hexfusion/etcd that referenced this pull request Jul 28, 2021
Elbehery pushed a commit to Elbehery/etcd that referenced this pull request May 12, 2022
Elbehery pushed a commit to Elbehery/etcd that referenced this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants