Skip to content

Commit

Permalink
Write logs to stderr by default
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@georgethebeatle @yulianedyalkova
georgethebeatle and yulianedyalkova committed Apr 24, 2019
1 parent 68b4ff5 commit a146081
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 55 deletions.
34 changes: 16 additions & 18 deletions libcontainer/container_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,24 +439,24 @@ func (c *linuxContainer) includeExecFifo(cmd *exec.Cmd) error {
}

func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) {
parentPipe, childPipe, err := utils.NewSockPair("init")
parentInitPipe, childInitPipe, err := utils.NewSockPair("init")
if err != nil {
return nil, newSystemErrorWithCause(err, "creating new init pipe")
}
messagePipe := readWritePair{parentPipe, childPipe}
messageSockPair := filePair{parentInitPipe, childInitPipe}

r, w, err := os.Pipe()
parentLogPipe, childLogPipe, err := os.Pipe()
if err != nil {
return nil, fmt.Errorf("Unable to create the log pipe: %s", err)
}
logPipe := readWritePair{r, w}
logFilePair := filePair{parentLogPipe, childLogPipe}

cmd, err := c.commandTemplate(p, childPipe, logPipe.w)
cmd, err := c.commandTemplate(p, childInitPipe, childLogPipe)
if err != nil {
return nil, newSystemErrorWithCause(err, "creating new command template")
}
if !p.Init {
return c.newSetnsProcess(p, cmd, messagePipe, logPipe)
return c.newSetnsProcess(p, cmd, messageSockPair, logFilePair)
}

// We only set up fifoFd if we're not doing a `runc exec`. The historic
Expand All @@ -467,10 +467,10 @@ func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) {
if err := c.includeExecFifo(cmd); err != nil {
return nil, newSystemErrorWithCause(err, "including execfifo in cmd.Exec setup")
}
return c.newInitProcess(p, cmd, messagePipe, logPipe)
return c.newInitProcess(p, cmd, messageSockPair, logFilePair)
}

func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File, logPipe *os.File) (*exec.Cmd, error) {
func (c *linuxContainer) commandTemplate(p *Process, childInitPipe *os.File, childLogPipe *os.File) (*exec.Cmd, error) {
cmd := exec.Command(c.initPath, c.initArgs[1:]...)
cmd.Args[0] = c.initArgs[0]
cmd.Stdin = p.Stdin
Expand All @@ -488,13 +488,13 @@ func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File, logPipe
fmt.Sprintf("_LIBCONTAINER_CONSOLE=%d", stdioFdCount+len(cmd.ExtraFiles)-1),
)
}
cmd.ExtraFiles = append(cmd.ExtraFiles, childPipe)
cmd.ExtraFiles = append(cmd.ExtraFiles, childInitPipe)
cmd.Env = append(cmd.Env,
fmt.Sprintf("_LIBCONTAINER_INITPIPE=%d", stdioFdCount+len(cmd.ExtraFiles)-1),
fmt.Sprintf("_LIBCONTAINER_STATEDIR=%s", c.root),
)

cmd.ExtraFiles = append(cmd.ExtraFiles, logPipe)
cmd.ExtraFiles = append(cmd.ExtraFiles, childLogPipe)
cmd.Env = append(cmd.Env,
fmt.Sprintf("_LIBCONTAINER_LOGPIPE=%d", stdioFdCount+len(cmd.ExtraFiles)-1),
fmt.Sprintf("_LIBCONTAINER_LOGLEVEL=%s", p.LogLevel),
Expand All @@ -509,7 +509,7 @@ func (c *linuxContainer) commandTemplate(p *Process, childPipe *os.File, logPipe
return cmd, nil
}

func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, initPipe, logPipe readWritePair) (*initProcess, error) {
func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, messageSockPair, logFilePair filePair) (*initProcess, error) {
cmd.Env = append(cmd.Env, "_LIBCONTAINER_INITTYPE="+string(initStandard))
nsMaps := make(map[configs.NamespaceType]string)
for _, ns := range c.config.Namespaces {
Expand All @@ -524,9 +524,8 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, initPipe, log
}
init := &initProcess{
cmd: cmd,
childPipe: initPipe.w,
parentPipe: initPipe.r,
logPipe: logPipe,
messageSockPair: messageSockPair,
logFilePair: logFilePair,
manager: c.cgroupManager,
intelRdtManager: c.intelRdtManager,
config: c.newInitConfig(p),
Expand All @@ -539,7 +538,7 @@ func (c *linuxContainer) newInitProcess(p *Process, cmd *exec.Cmd, initPipe, log
return init, nil
}

func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, nsPipe, logPipe readWritePair) (*setnsProcess, error) {
func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, messageSockPair, logFilePair filePair) (*setnsProcess, error) {
cmd.Env = append(cmd.Env, "_LIBCONTAINER_INITTYPE="+string(initSetns))
state, err := c.currentState()
if err != nil {
Expand All @@ -556,9 +555,8 @@ func (c *linuxContainer) newSetnsProcess(p *Process, cmd *exec.Cmd, nsPipe, logP
cgroupPaths: c.cgroupManager.GetPaths(),
rootlessCgroups: c.config.RootlessCgroups,
intelRdtPath: state.IntelRdtPath,
childPipe: nsPipe.w,
parentPipe: nsPipe.r,
logPipe: logPipe,
messageSockPair: messageSockPair,
logFilePair: logFilePair,
config: c.newInitConfig(p),
process: p,
bootstrapData: data,
Expand Down
6 changes: 3 additions & 3 deletions libcontainer/logs/logs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
)

var (
mutex = &sync.Mutex{}
configureMutex = sync.Mutex{}
// loggingConfigured will be set once logging has been configured via invoking `ConfigureLogging`.
// Subsequent invocations of `ConfigureLogging` would be no-op
loggingConfigured = false
Expand Down Expand Up @@ -64,8 +64,8 @@ func processEntry(text []byte) {
}

func ConfigureLogging(config Config) error {
mutex.Lock()
defer mutex.Unlock()
configureMutex.Lock()
defer configureMutex.Unlock()

if loggingConfigured {
logrus.Debug("logging has already been configured")
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/nsenter/nsenter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ type pid struct {
}

type logentry struct {
Msg string
Level string
Msg string `json:"msg"`
Level string `json:"level"`
}

func TestNsenterValidPaths(t *testing.T) {
Expand Down
58 changes: 28 additions & 30 deletions libcontainer/process_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,15 @@ type parentProcess interface {
forwardChildLogs()
}

type readWritePair struct {
r *os.File
w *os.File
type filePair struct {
parent *os.File
child *os.File
}

type setnsProcess struct {
cmd *exec.Cmd
parentPipe *os.File
childPipe *os.File
logPipe readWritePair
messageSockPair filePair
logFilePair filePair
cgroupPaths map[string]string
rootlessCgroups bool
intelRdtPath string
Expand All @@ -85,16 +84,16 @@ func (p *setnsProcess) signal(sig os.Signal) error {
}

func (p *setnsProcess) start() (err error) {
defer p.parentPipe.Close()
defer p.messageSockPair.parent.Close()
err = p.cmd.Start()
// close the write-side of the pipes (controlled by child)
p.childPipe.Close()
p.logPipe.w.Close()
p.messageSockPair.child.Close()
p.logFilePair.child.Close()
if err != nil {
return newSystemErrorWithCause(err, "starting setns process")
}
if p.bootstrapData != nil {
if _, err := io.Copy(p.parentPipe, p.bootstrapData); err != nil {
if _, err := io.Copy(p.messageSockPair.parent, p.bootstrapData); err != nil {
return newSystemErrorWithCause(err, "copying bootstrap data to pipe")
}
}
Expand All @@ -120,11 +119,11 @@ func (p *setnsProcess) start() (err error) {
if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil {
return newSystemErrorWithCause(err, "setting rlimits for process")
}
if err := utils.WriteJSON(p.parentPipe, p.config); err != nil {
if err := utils.WriteJSON(p.messageSockPair.parent, p.config); err != nil {
return newSystemErrorWithCause(err, "writing config to pipe")
}

ierr := parseSync(p.parentPipe, func(sync *syncT) error {
ierr := parseSync(p.messageSockPair.parent, func(sync *syncT) error {
switch sync.Type {
case procReady:
// This shouldn't happen.
Expand All @@ -137,7 +136,7 @@ func (p *setnsProcess) start() (err error) {
}
})

if err := unix.Shutdown(int(p.parentPipe.Fd()), unix.SHUT_WR); err != nil {
if err := unix.Shutdown(int(p.messageSockPair.parent.Fd()), unix.SHUT_WR); err != nil {
return newSystemErrorWithCause(err, "calling shutdown on init pipe")
}
// Must be done after Shutdown so the child will exit and we can wait for it.
Expand All @@ -163,7 +162,7 @@ func (p *setnsProcess) execSetns() error {
return newSystemError(&exec.ExitError{ProcessState: status})
}
var pid *pid
if err := json.NewDecoder(p.parentPipe).Decode(&pid); err != nil {
if err := json.NewDecoder(p.messageSockPair.parent).Decode(&pid); err != nil {
p.cmd.Wait()
return newSystemErrorWithCause(err, "reading pid from init pipe")
}
Expand Down Expand Up @@ -217,14 +216,13 @@ func (p *setnsProcess) setExternalDescriptors(newFds []string) {
}

func (p *setnsProcess) forwardChildLogs() {
go logs.ForwardLogs(p.logPipe.r)
go logs.ForwardLogs(p.logFilePair.parent)
}

type initProcess struct {
cmd *exec.Cmd
parentPipe *os.File
childPipe *os.File
logPipe readWritePair
messageSockPair filePair
logFilePair filePair
config *initConfig
manager cgroups.Manager
intelRdtManager intelrdt.Manager
Expand All @@ -246,7 +244,7 @@ func (p *initProcess) externalDescriptors() []string {
// getChildPid receives the final child's pid over the provided pipe.
func (p *initProcess) getChildPid() (int, error) {
var pid pid
if err := json.NewDecoder(p.parentPipe).Decode(&pid); err != nil {
if err := json.NewDecoder(p.messageSockPair.parent).Decode(&pid); err != nil {
p.cmd.Wait()
return -1, err
}
Expand Down Expand Up @@ -282,12 +280,12 @@ func (p *initProcess) waitForChildExit(childPid int) error {
}

func (p *initProcess) start() error {
defer p.parentPipe.Close()
defer p.messageSockPair.parent.Close()
err := p.cmd.Start()
p.process.ops = p
// close the write-side of the pipes (controlled by child)
p.childPipe.Close()
p.logPipe.w.Close()
p.messageSockPair.child.Close()
p.logFilePair.child.Close()
if err != nil {
p.process.ops = nil
return newSystemErrorWithCause(err, "starting init process command")
Expand All @@ -313,7 +311,7 @@ func (p *initProcess) start() error {
}
}()

if _, err := io.Copy(p.parentPipe, p.bootstrapData); err != nil {
if _, err := io.Copy(p.messageSockPair.parent, p.bootstrapData); err != nil {
return newSystemErrorWithCause(err, "copying bootstrap data to pipe")
}
childPid, err := p.getChildPid()
Expand Down Expand Up @@ -341,7 +339,7 @@ func (p *initProcess) start() error {
}
// Now it's time to setup cgroup namesapce
if p.config.Config.Namespaces.Contains(configs.NEWCGROUP) && p.config.Config.Namespaces.PathOf(configs.NEWCGROUP) == "" {
if _, err := p.parentPipe.Write([]byte{createCgroupns}); err != nil {
if _, err := p.messageSockPair.parent.Write([]byte{createCgroupns}); err != nil {
return newSystemErrorWithCause(err, "sending synchronization value to init process")
}
}
Expand All @@ -368,7 +366,7 @@ func (p *initProcess) start() error {
sentResume bool
)

ierr := parseSync(p.parentPipe, func(sync *syncT) error {
ierr := parseSync(p.messageSockPair.parent, func(sync *syncT) error {
switch sync.Type {
case procReady:
// set rlimits, this has to be done here because we lose permissions
Expand Down Expand Up @@ -404,7 +402,7 @@ func (p *initProcess) start() error {
}
}
// Sync with child.
if err := writeSync(p.parentPipe, procRun); err != nil {
if err := writeSync(p.messageSockPair.parent, procRun); err != nil {
return newSystemErrorWithCause(err, "writing syncT 'run'")
}
sentRun = true
Expand Down Expand Up @@ -433,7 +431,7 @@ func (p *initProcess) start() error {
}
}
// Sync with child.
if err := writeSync(p.parentPipe, procResume); err != nil {
if err := writeSync(p.messageSockPair.parent, procResume); err != nil {
return newSystemErrorWithCause(err, "writing syncT 'resume'")
}
sentResume = true
Expand All @@ -450,7 +448,7 @@ func (p *initProcess) start() error {
if p.config.Config.Namespaces.Contains(configs.NEWNS) && !sentResume {
return newSystemError(fmt.Errorf("could not synchronise after executing prestart hooks with container process"))
}
if err := unix.Shutdown(int(p.parentPipe.Fd()), unix.SHUT_WR); err != nil {
if err := unix.Shutdown(int(p.messageSockPair.parent.Fd()), unix.SHUT_WR); err != nil {
return newSystemErrorWithCause(err, "shutting down init pipe")
}

Expand Down Expand Up @@ -494,7 +492,7 @@ func (p *initProcess) sendConfig() error {
// send the config to the container's init process, we don't use JSON Encode
// here because there might be a problem in JSON decoder in some cases, see:
// https://github.com/docker/docker/issues/14203#issuecomment-174177790
return utils.WriteJSON(p.parentPipe, p.config)
return utils.WriteJSON(p.messageSockPair.parent, p.config)
}

func (p *initProcess) createNetworkInterfaces() error {
Expand Down Expand Up @@ -527,7 +525,7 @@ func (p *initProcess) setExternalDescriptors(newFds []string) {
}

func (p *initProcess) forwardChildLogs() {
go logs.ForwardLogs(p.logPipe.r)
go logs.ForwardLogs(p.logFilePair.parent)
}

func getPipeFds(pid int) ([]string, error) {
Expand Down
2 changes: 1 addition & 1 deletion main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func main() {
},
cli.StringFlag{
Name: "log",
Value: "/dev/null",
Value: "/dev/stderr",
Usage: "set the log file path where internal debug information is written",
},
cli.StringFlag{
Expand Down
5 changes: 5 additions & 0 deletions tests/integration/debug.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ function teardown() {
runc --debug run test_hello
echo "${output}"
[ "$status" -eq 0 ]

# check expected debug output was sent to stderr
[[ "${output}" == *"level=debug"* ]]
[[ "${output}" == *"nsexec started"* ]]
[[ "${output}" == *"child process in init()"* ]]
}

@test "global --debug to --log" {
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/helpers.bash
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ function runc() {

# Raw wrapper for runc.
function __runc() {
"$RUNC" ${RUNC_USE_SYSTEMD+--systemd-cgroup} --log /proc/self/fd/2 --root "$ROOT" "$@"
"$RUNC" ${RUNC_USE_SYSTEMD+--systemd-cgroup} --root "$ROOT" "$@"
}

# Wrapper for runc spec, which takes only one argument (the bundle path).
Expand Down

0 comments on commit a146081

Please sign in to comment.