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

feature: add support for maximum log size and max number of log files #2262

Closed
wants to merge 0 commits into from

Conversation

zjumoon01
Copy link
Contributor

@zjumoon01 zjumoon01 commented Sep 20, 2018

Signed-off-by: Wangrui [email protected]

Ⅰ. Describe what this PR did

Add two flags max-size and max-file followed by --log-opt to support container log rotate.
Only support jsonfile driver.

Add max-size and max-file flags followed by --log-opt in pouchd.
Example:

pouchd --log-opt max-size=100m --log-opt max-file=5

This will rotate container log when its size exceeds 100Mb, and it is saved in 5 log files at most.
Note that max-size and max-file flags are only supported in jsonfile driver.

Ⅱ. Does this pull request fix one issue?

fixes #2177

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Now some test cases are done manually.
Test code is absent. It's welcome and a "TODO" work.

Ⅳ. Describe how to verify it

step1. launch pouchd with max-size and max-file flags

\# pouchd --log-opt max-size=10k --log-opt max-file=2

step2. run a container which could write stdout to its log

\# pouch run docker.io/library/busybox:latest top

step3. check container log is rotated

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #2262 into master will decrease coverage by 0.18%.
The diff coverage is 38.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
- Coverage    66.8%   66.61%   -0.19%     
==========================================
  Files         208      208              
  Lines       16934    16986      +52     
==========================================
+ Hits        11312    11316       +4     
- Misses       4260     4301      +41     
- Partials     1362     1369       +7
Flag Coverage Δ
#criv1alpha1test 32.67% <37.28%> (+0.15%) ⬆️
#criv1alpha2test 35.95% <37.28%> (-0.02%) ⬇️
#integrationtest 39.49% <38.98%> (-0.04%) ⬇️
#nodee2etest 33.28% <37.28%> (-0.15%) ⬇️
#unittest 23.73% <20.33%> (-0.03%) ⬇️
Impacted Files Coverage Δ
daemon/containerio/jsonfile.go 73.23% <100%> (ø) ⬆️
daemon/mgr/container_logs.go 84.21% <100%> (ø) ⬆️
daemon/logger/jsonfile/jsonfile.go 46.15% <36.84%> (-23.08%) ⬇️
cri/ocicni/cni_manager.go 65% <0%> (-15%) ⬇️
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
daemon/containerio/cri_log_file.go 84.31% <0%> (-3.93%) ⬇️
ctrd/image.go 76.75% <0%> (-2.2%) ⬇️
pkg/meta/store.go 62.5% <0%> (-1.57%) ⬇️
daemon/mgr/container_utils.go 83.13% <0%> (-1.21%) ⬇️
cri/v1alpha2/cri.go 66.46% <0%> (-0.6%) ⬇️
... and 7 more

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2018

CLA assistant check
All committers have signed the CLA.

return &JSONLogFile{
f: f,
perms: perms,
closed: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add more filed here as https://docs.docker.com/config/containers/logging/json-file/#options, and add a validate func to check in case other filed can't work but appeared in pouch inspect?

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. Now pouch's log options is not implemented completely. More filed should be supported in the future, but in other independent patches, I think.
validate check function is necessary, too. Should it be also treated as another issue to fix ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok LGTM

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

please remove the submodule

@fuweid
Copy link
Contributor

fuweid commented Sep 21, 2018

basically, it look good to me, but if we merge this, the pouch logs doesn't work well.

@zjumoon01 would you add one more issue to track this? thanks

@pouchrobot
Copy link
Collaborator

ping @zjumoon01
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

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.

[feature request] Pouchd supports --log-opt flag
6 participants