Skip to content

Add --log-file flag for windows service.#3831

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:add-windows-log-file
Nov 18, 2019
Merged

Add --log-file flag for windows service.#3831
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:add-windows-log-file

Conversation

@Random-Liu
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu commented Nov 15, 2019

Event and tracing are good, but the traditional log file is also very useful, e.g. I can't find an ETW plugin for fluentd yet.

--log-file is what we are doing for kubelet and kube-proxy on windows, too. It is optional, old behavior is kept if the flag is not specified.

@jterry75

Signed-off-by: Lantao Liu lantaol@google.com

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 15, 2019

Build succeeded.

@Random-Liu
Copy link
Copy Markdown
Member Author

The test failure is another occurrence of #3787.

Not sure why I can't trigger travis rerun now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think GlobalString will return "" if the param is not set, so you can probably just write logFileFlag = context.GlobalString("log-file").

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. Also enhanced the service name flag handling a little bit.

@kevpar
Copy link
Copy Markdown
Member

kevpar commented Nov 15, 2019

Is there any desire to support a log-file param on non-Windows platforms? Or is supporting only Windows sufficient?

@Random-Liu
Copy link
Copy Markdown
Member Author

@kevpar I think only windows is sufficient.

On linux people usually use journald or IO redirection directly. :)

Signed-off-by: Lantao Liu <lantaol@google.com>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Nov 16, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3831 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3831      +/-   ##
==========================================
+ Coverage   41.98%   41.99%   +0.01%     
==========================================
  Files         130      130              
  Lines       14577    14577              
==========================================
+ Hits         6120     6122       +2     
+ Misses       7543     7542       -1     
+ Partials      914      913       -1
Flag Coverage Δ
#linux 45.44% <ø> (+0.01%) ⬆️
#windows 36.94% <ø> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/btrfs/btrfs.go 58.29% <0%> (+0.89%) ⬆️

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 ec661e8...0bb48ae. Read the comment docs.

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

LGTM

@crosbymichael crosbymichael merged commit 31ea7b4 into containerd:master Nov 18, 2019
@Random-Liu Random-Liu deleted the add-windows-log-file branch November 21, 2019 08:22
@ameyag
Copy link
Copy Markdown
Contributor

ameyag commented Nov 22, 2019

Requesting 1.3.X backport with #3840

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.

6 participants