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

Support for logging from children processes #2034

Merged

Conversation

Projects
None yet
5 participants
@danail-branekov
Copy link
Contributor

commented Apr 4, 2019

This PR is continuation of PR #1861 to address the comments in the original pull request, below is the original description:

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.

cc @marcov @georgethebeatle

@danail-branekov danail-branekov force-pushed the masters-of-cats:pr-child-logging branch from 1fdc31f to e6a8286 Apr 4, 2019

marcov added some commits Aug 3, 2018

Support for logging from children processes
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>
Remove pipe close before exec.
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>

@danail-branekov danail-branekov force-pushed the masters-of-cats:pr-child-logging branch 2 times, most recently from 302cb74 to fa7a3e6 Apr 4, 2019

Address comments in PR 1861
Refactor configuring logging into a reusable component
so that it can be nicely used in both main() and init process init()

Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>
Co-authored-by: Giuseppe Capizzi <gcapizzi@pivotal.io>
Co-authored-by: Claudia Beresford <cberesford@pivotal.io>
Signed-off-by: Danail Branekov <danailster@gmail.com>

@danail-branekov danail-branekov force-pushed the masters-of-cats:pr-child-logging branch from fa7a3e6 to c486e3c Apr 4, 2019

@georgethebeatle

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Any thoughts on this PR? We'd like to get this merged because we are after a D-state caused by runc kill trying to freeze the container cgroup while a runc exec is running. We believe that logs from nsexec would be pretty helpful.

@@ -484,6 +493,12 @@ func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File) (*exec.
fmt.Sprintf("_LIBCONTAINER_INITPIPE=%d", stdioFdCount+len(cmd.ExtraFiles)-1),
fmt.Sprintf("_LIBCONTAINER_STATEDIR=%s", c.root),
)

cmd.ExtraFiles = append(cmd.ExtraFiles, logPipe)

This comment has been minimized.

Copy link
@crosbymichael

crosbymichael Apr 16, 2019

Member

This side of the pipe should be closed after start as it's provided to the new process and dup'd

This comment has been minimized.

Copy link
@cyphar

cyphar Apr 17, 2019

Member

There also needs to be code in init_linux.go to close the pipe before we execve the user code.

This comment has been minimized.

Copy link
@georgethebeatle

georgethebeatle Apr 17, 2019

Contributor

@cyphar Isn't that done generically for all extra fds (except the ones requested by the user) as part of finalizeNamespace that is being called by both init and setns flavours of init lifecycle?

This comment has been minimized.

Copy link
@cyphar

cyphar Apr 20, 2019

Member

Yeah, that make sense.

Show resolved Hide resolved init.go Outdated
Show resolved Hide resolved libcontainer/container_linux.go Outdated
Show resolved Hide resolved libcontainer/logs/logs.go Outdated
Show resolved Hide resolved libcontainer/logs/logs.go Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
@@ -484,6 +493,12 @@ func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File) (*exec.
fmt.Sprintf("_LIBCONTAINER_INITPIPE=%d", stdioFdCount+len(cmd.ExtraFiles)-1),
fmt.Sprintf("_LIBCONTAINER_STATEDIR=%s", c.root),
)

cmd.ExtraFiles = append(cmd.ExtraFiles, logPipe)

This comment has been minimized.

Copy link
@cyphar

cyphar Apr 17, 2019

Member

There also needs to be code in init_linux.go to close the pipe before we execve the user code.

Show resolved Hide resolved init.go Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated

georgethebeatle and others added some commits Apr 19, 2019

Improve nsexec logging
* Simplify logging function
* Logs contain __FUNCTION__:__LINE__
* Bail uses write_log

Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com>
Co-authored-by: Danail Branekov <danailster@gmail.com>
Signed-off-by: Georgi Sabev <georgethebeatle@gmail.com>
Remove redundant log function
Bump logrus so that we can use logrus.StandardLogger().Logf instead

Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com>
Signed-off-by: Georgi Sabev <georgethebeatle@gmail.com>

@georgethebeatle georgethebeatle force-pushed the masters-of-cats:pr-child-logging branch from 549dbe0 to 475aef1 Apr 22, 2019

@cyphar
Copy link
Member

left a comment

Looks much better! Aside from these nits, it looks good -- though I will play around with it a bit before giving it a LGTM.

Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsenter_test.go
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/nsenter/nsexec.c Outdated
Show resolved Hide resolved libcontainer/process_linux.go Outdated
Show resolved Hide resolved libcontainer/container_linux.go Outdated
Show resolved Hide resolved libcontainer/logs/logs.go Outdated
@cyphar

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

One more thing that should be fixed is that the default --log value should not be /dev/null it should be /dev/stderr (but maybe @crosbymichael has an opinion about whether it's a good idea to start outputting runc information into the console). The main issue is that bail currently won't work as-advertised purely because the Fatalf isn't going to the console (it's being thrown into /dev/null).

georgethebeatle and others added some commits Apr 23, 2019

Simplify bail logic & minor nsexec improvements
Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com>
Signed-off-by: Georgi Sabev <georgethebeatle@gmail.com>
Write logs to stderr by default
Minor refactoring to use the filePair struct for both init sock and log pipe

Co-authored-by: Julia Nedialkova <julianedialkova@hotmail.com>
Signed-off-by: Georgi Sabev <georgethebeatle@gmail.com>
@georgethebeatle

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

We addressed your comments, pls take a look

@cyphar

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

LGTM, looks great! I can't wait for runc debugging to get soooooo much easier. Many ❤️s for the really fast turnaround and listening to my nit-picking. 😉

@crosbymichael Can you ACK whether the --log default change is okay for you? Most (good) runc wrappers should be using --console-socket anyway but for terminal users the interleaved output might confuse users.

Approved with PullApprove

@georgethebeatle

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@crosbymichael can you pls take a look?

@crosbymichael

This comment has been minimized.

Copy link
Member

commented May 7, 2019

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 70bc4cd into opencontainers:master May 7, 2019

3 checks passed

DCO DCO
Details
code-review/pullapprove Approved by crosbymichael, cyphar
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.