Skip to content
This repository has been archived by the owner on Dec 20, 2024. It is now read-only.

Configure dfclient/dfdaemon/dfget log file path via 'logConfig.path' property #1145

Merged
merged 5 commits into from
Jan 16, 2020

Conversation

YanzheL
Copy link
Contributor

@YanzheL YanzheL commented Dec 27, 2019

Signed-off-by: YanzheL [email protected]

Ⅰ. Describe what this PR did

Currently, there is no option for user to customize the log file path, which causes inconvenience while using dfclient in container.

For a typical use case, it is expected that the dfclient container logs to stdout so we can inspect the logs via docker logs -f <dfclient> instead of checking the log file in default location ~/.small-dragonfly/logs/dfdaemon.log

This PR enables user to configure the log file path via configuration property logConfig.path.

For example

logConfig:
  path: /dev/stdout

Ⅱ. Does this pull request fix one issue?

NONE

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

Already tested in container.

Ⅳ. Describe how to verify it

  1. Build the dockerfile

  2. Add this property in dfclient configuration file

    logConfig:
      path: /dev/stdout
  3. You will see the log in stdout instead of the default location ~/.small-dragonfly/logs/dfdaemon.log

Ⅴ. Special notes for reviews

@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Dragonfly, @YanzheL
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/dragonflyoss/Dragonfly/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@YanzheL YanzheL force-pushed the configurable-logfile-path branch 2 times, most recently from b214098 to bc6f296 Compare December 27, 2019 11:07
@pouchrobot pouchrobot added size/M and removed size/S labels Dec 27, 2019
@YanzheL YanzheL force-pushed the configurable-logfile-path branch from bc6f296 to 618ba1d Compare December 27, 2019 11:11
@pouchrobot pouchrobot added size/S and removed size/M labels Dec 27, 2019
@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #1145 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
- Coverage   47.65%   47.63%   -0.02%     
==========================================
  Files         115      115              
  Lines        7225     7228       +3     
==========================================
  Hits         3443     3443              
- Misses       3508     3511       +3     
  Partials      274      274
Impacted Files Coverage Δ
pkg/dflog/log.go 52.42% <ø> (ø) ⬆️
cmd/dfdaemon/app/init.go 0% <0%> (ø) ⬆️
cmd/dfget/app/server.go 48.83% <0%> (-1.17%) ⬇️
cmd/dfget/app/root.go 71.34% <0%> (-0.44%) ⬇️

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 def2b21...c02b0b0. Read the comment docs.

pkg/dflog/log.go Outdated
@@ -72,7 +77,7 @@ func getLumberjack(l *logrus.Logger) *lumberjack.Logger {
func WithLogFile(f string, maxSize, maxBackups int) Option {
return func(l *logrus.Logger) error {
if f == "" {
return nil
f = filepath.Join("logs", "dfdaemon.log")
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, not only dfdaemon will use this function, so do with dfget and supenrode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I rollbacked the change of this line, and set the default log path for dfget/dfdaemon in caller (See my new commits).

As for supernode, I didn't touch its code. Because it writes to two different log files, a single logConfig.path property is not enough to configure the location of these files.

For now, this PR configures the log path of dfget/dfdaemon, not for supernode.

@lowzj
Copy link
Member

lowzj commented Jan 13, 2020

@YanzheL thanks for your contributing very much! Could you please rebase the code and fix the conflict and reviewed comment, then I will merge this pr.

@YanzheL YanzheL changed the title Configure dfclient log file path via 'logConfig.path' property Configure dfclient/dfdaemon/dfget log file path via 'logConfig.path' property Jan 14, 2020
@YanzheL
Copy link
Contributor Author

YanzheL commented Jan 14, 2020

@YanzheL thanks for your contributing very much! Could you please rebase the code and fix the conflict and reviewed comment, then I will merge this pr.

I updated and rebased the PR, thanks.

Copy link
Member

@lowzj lowzj left a comment

Choose a reason for hiding this comment

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

LGTM

@lowzj lowzj merged commit cbb4d50 into dragonflyoss:master Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants