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

add a flag to force output to stdout #5449

Closed
alban opened this issue May 25, 2016 · 27 comments
Closed

add a flag to force output to stdout #5449

alban opened this issue May 25, 2016 · 27 comments

Comments

@alban
Copy link
Contributor

alban commented May 25, 2016

In older versions (e.g. 2.0.10), etcd writes the output both on stdout and the journal. In newer version, it only writes to the journal:

sudo systemd-run -t $HOME/go/bin/etcd-v2.0.10

--> output on stdout and journald (visible with journalctl)

sudo systemd-run -t $HOME/go/bin/etcd-master

--> output on journald only

Could we have a flag to force output to stdout as well?

Initially reported in #4993
See also rkt/rkt#2417 (comment)

/cc @jonboulle @iaguis

@jonboulle
Copy link
Contributor

Introduced by coreos/pkg#28

also see coreos/pkg#38 (comment) :-(

@xiang90 xiang90 added this to the unplanned milestone Jun 7, 2016
@gyuho
Copy link
Contributor

gyuho commented Jun 13, 2016

I think etcd just needs to make capnslog formatter configurable as capnslog.SetFormatter(capnslog.NewPrettyFormatter(os.Stdout, false)).

Maybe add --log-output=[stdout,stderr,journald] flag to etcd?

@jlamillan
Copy link
Contributor

Maybe add --log-output=[stdout,stderr,journald] flag to etcd?

Is the intent here that a user choose one? i.e.
--log-output=[stdout|stderr|journald]
etcd ... --log-output=journald

Or is it that the user could choose many? i.e.
--log-output=[stdout,stderr,journald]
etcd ... --log-output=journald,stdout

Also, seeing as how there is conditional logic to behave differently if etcd is running under init / systemd, should there be a documented default?

@gyuho
Copy link
Contributor

gyuho commented Jun 15, 2016

@jlamillan We can overwrite that logic by calling capnslog.SetFormatter. And I think we should make user choose one, since rkt team wants to

have a flag to force output to stdout as well?

gyuho added a commit to gyuho/etcd that referenced this issue Jun 15, 2016
So we can choose where to write logs.
Fix etcd-io#5449.
gyuho added a commit to gyuho/etcd that referenced this issue Jun 15, 2016
So we can choose where to write logs.
Fix etcd-io#5449.
@gyuho
Copy link
Contributor

gyuho commented Jun 15, 2016

@alban Sorry, I think I mis-read the issue:

Could we have a flag to force output to stdout as well

Do you mean forwarding logs to journald and stdout at the same time?

I don't think we should do that, since it's another write overhead? etcd will only log to stderr or journald when systemd is detected and this is handled by capnslog package. etcd doesn't provide any log configuration.

@xiang90
Copy link
Contributor

xiang90 commented Jun 30, 2016

@jonboulle @alban I think it is fine for us to add a flag to force writing log to stdout by default. When set it to false, we will still let the logging pkg to decide where to log. Adding a flag to support logging to multiple sinks seems to be over-kill.

@euank
Copy link

euank commented Sep 6, 2016

I like the idea of defaulting to stdout based entirely on the number of times I've personally had etcd drop out from under me and leave no trace of output on my terminal.

@GreatSUN
Copy link

If the setup outside of a container is the default, I would stay with journal but offer at least an option to be able to switch either entirely or in addtion to stdout. Currently I always need to test stuffs manually after errors with the containers, because they drop away and I can not read the logs anymore.

@philips
Copy link
Contributor

philips commented Sep 14, 2016

Can we just dump journal support? It seems like it adds little value and causes confusion.

@jonboulle
Copy link
Contributor

+1, I'm reaching a similar conclusion

On 14 September 2016 at 21:42, Brandon Philips [email protected]
wrote:

Can we just dump journal support? It seems like it adds little value and
causes confusion.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5449 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACewN3Zgmu5FHDwrWG92YkUFdpcBcm-pks5qqE4ZgaJpZM4ImhQU
.

@crawford
Copy link
Contributor

Keep in mind that if you drop journal support, you lose the ability to look at the full logs when run under systemd. We depend on the syslog identifier for filtering logs since there is a race on service termination.

@jonboulle
Copy link
Contributor

Oh damn, I thought that was fixed.

Alex Crawford [email protected] schrieb am Fr., 16. Sep. 2016,
19:25:

Keep in mind that if you drop journal support, you lose the ability to
look at the full logs when run under systemd. We depend on the syslog
identifier for filtering logs since there is a race on service termination.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#5449 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACewN2IXFf3U35Mh6fBtc8Fizp-7sJeMks5qqtEUgaJpZM4ImhQU
.

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@crawford @jonboulle @heyitsanthony

Well... How should we move this forward?

Breaking journald == tons of github issues without necessary logging information.

@crawford
Copy link
Contributor

crawford commented Nov 3, 2016

If you are pid 1, log to stdout. If not, use the journal.

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@crawford

The logging lib we use only write to journald when the ppid is 1 (pid 1's ppid is not 1): https://github.com/coreos/pkg/blob/master/capnslog/init.go#L41

So if the pid is 1, we will not log to journal now since its ppid is not1. The current behavior is what you just describe.

I guess it wont make a difference?

@crawford
Copy link
Contributor

crawford commented Nov 3, 2016

Was that added recently? The earlier comments in this report imply that this is not how it behaves.

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@crawford No. It added quite a while ago.

@crawford
Copy link
Contributor

crawford commented Nov 3, 2016

Sounds like it isn't working properly then, otherwise I don't think this issue would have come up. Can you double check it?

@rboyer
Copy link
Contributor

rboyer commented Nov 3, 2016

Docker added optional support to run the contained program as not PID 1 recently, so that might complicate that proposal: moby/moby#26061

@crawford
Copy link
Contributor

crawford commented Nov 3, 2016

I talked to @xiang90 offline. The plan is to leave the existing logic in place (detects the PPID of etcd to determine whether it should use the journal or stdout) and add a flag to override the behavior. Within rkt, the PPID of etcd is 1, so it logs to the journal. The rkt team would like to be able to override this so that it logs to stdout instead, allowing rkt to capture the logs.

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@jonboulle @philips @gyuho

Can we add a flag to make etcd log force to stdout only? This is not beautiful, but it does resolve the rkt issue.

@gyuho
Copy link
Contributor

gyuho commented Nov 3, 2016

I am fine with it. Maybe this #5449 (comment)?

I think etcd just needs to make capnslog formatter configurable as capnslog.SetFormatter(capnslog.NewPrettyFormatter(os.Stdout, false)).
Maybe add --log-output=[stdout,stderr,journald] flag to etcd?

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@gyuho Can you just have a flag --log-to-stdout? /cc @heyitsanthony

@heyitsanthony
Copy link
Contributor

--log-output with a comma separated list would have the best future-proofing

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

@heyitsanthony

Why do we want etcd to log to N places?

@heyitsanthony
Copy link
Contributor

@xiang90 the issue is asking logging to two places but I don't have a strong opinion on it. Multi-logging can be added later so long as there's --log-output=<somestring> in the first place.

@xiang90
Copy link
Contributor

xiang90 commented Nov 3, 2016

the issue is asking logging to two places

Yes, I know (#5449 (comment)). But I do not see why it is needed. All I can see is they want a way to force logging to stdout.

gyuho added a commit to gyuho/etcd that referenced this issue Nov 3, 2016
gyuho added a commit to gyuho/etcd that referenced this issue Nov 3, 2016
gyuho added a commit to gyuho/etcd that referenced this issue Nov 3, 2016
gyuho added a commit to gyuho/etcd that referenced this issue Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests