WIP - Support for logging from children processes#1861
WIP - Support for logging from children processes#1861marcov wants to merge 2 commits intoopencontainers:masterfrom
Conversation
Add support for children processes logging (including nsexec). A pipe is used to send logs from children to parent in JSON. The JSON format used is the same used by logrus JSON formatted, i.e. children process can use standard logrus APIs. Signed-off-by: Marco Vedovati <mvedovati@suse.com>
Pipe close before exec is not necessary as os.Pipe() is calling pipe2 with O_CLOEXEC option. Signed-off-by: Marco Vedovati <mvedovati@suse.com>
| } | ||
| } | ||
|
|
||
| logPipe, _ := (logrus.StandardLogger().Out).(*(os.File)) |
There was a problem hiding this comment.
Three things:
-
I'm not sure it's necessary to do
(a).(*os.File)-- I thinka.(*os.File)will work just as fine (andgofmtshould complain at you because it's not idiomatic). -
You shouldn't ignore the
okvalue here. Even if it is impossible to occur, you should either make it panic or you should check thatlogPipe != nil(which is the error case -- because that would also panic). -
Also we should probably set
logPipeto beO_CLOEXECwhich would remove the requirement to close it like this.
| } | ||
| if !p.Init { | ||
| return c.newSetnsProcess(p, cmd, parentPipe, childPipe) | ||
| return c.newSetnsProcess(p, cmd, parentPipe, childPipe, &logPipe) |
There was a problem hiding this comment.
It feels unnecessary to pass a reference here -- since the members are pointers there's no real risk of accidentally copying something. The later *logPipe just makes it look confusing.
| return nil, newSystemErrorWithCause(err, "including execfifo in cmd.Exec setup") | ||
| } | ||
| return c.newInitProcess(p, cmd, parentPipe, childPipe) | ||
| return c.newInitProcess(p, cmd, parentPipe, childPipe, &logPipe) |
There was a problem hiding this comment.
Ditto on the &logPipe comment here too.
| logrus.WarnLevel: logrus.Warn, | ||
| logrus.InfoLevel: logrus.Info, | ||
| logrus.DebugLevel: logrus.Debug, | ||
| } |
There was a problem hiding this comment.
This is a bit ugly, but it looks like logrus gives us no choice (you can't call Entry.log with an explicit level -- it even looks like there's a race condition in the level check inside logrus!). (Not an actionable complaint, just something I noticed.)
| "{\"level\":\"%s\", \"msg\": \"", strlevel[level]); | ||
|
|
||
| va_start(args, format); | ||
| len += vsnprintf(&jsonbuffer[len], sizeof(jsonbuffer) - len, format, args); |
There was a problem hiding this comment.
While not an immediate issue, this does mean that we cannot deal with " (or any special characters) inside of the JSON string. Hopefully we won't hit that problem, but it is something to keep in mind.
| app.Before = func(context *cli.Context) error { | ||
|
|
||
| // do nothing if logrus was already initialized in init.go | ||
| if logrus.StandardLogger().Out != logrus.New().Out { |
There was a problem hiding this comment.
I'm not really a fan of this -- why not set up logging for init here (or setting a global flag) rather than doing it this way?
|
|
||
| setExternalDescriptors(fds []string) | ||
|
|
||
| getChildLogs() |
There was a problem hiding this comment.
I don't really like this name -- maybe forwardChildLogs or proxyChildLogs to make it clear that this method actually pipes directly to logrus.
marcov
changed the title
2 participants
Add support for children processes logging (including nsexec).
A pipe is used to send logs from children to parent in JSON.
The JSON format used is the same used by logrus JSON formatted,
i.e. children process can use standard logrus APIs.
This improves debugging and can also be used to report warning messages from the children.