From 50ed292aee513667684e695b3d4b061fbac3d696 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 14 May 2020 17:32:44 -0400 Subject: Remove duplicated exec handling code During the initial workup of HTTP exec, I duplicated most of the existing exec handling code so I could work on it without breaking normal exec (and compare what I was doing to the nroaml version). Now that it's done and working, we can switch over to the refactored version and ditch the original, removing a lot of duplicated code. Signed-off-by: Matthew Heon --- libpod/oci_conmon_linux.go | 177 ++++----------------------------------------- 1 file changed, 15 insertions(+), 162 deletions(-) (limited to 'libpod') diff --git a/libpod/oci_conmon_linux.go b/libpod/oci_conmon_linux.go index 56cfa8ccd..1ffacfb0c 100644 --- a/libpod/oci_conmon_linux.go +++ b/libpod/oci_conmon_linux.go @@ -636,7 +636,6 @@ func (r *ConmonOCIRuntime) AttachResize(ctr *Container, newSize remotecommand.Te } // ExecContainer executes a command in a running container -// TODO: Split into Create/Start/Attach/Wait func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error) { if options == nil { return -1, nil, errors.Wrapf(define.ErrInvalidArg, "must provide an ExecOptions struct to ExecContainer") @@ -649,178 +648,46 @@ func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options return -1, nil, errors.Wrapf(define.ErrEmptyID, "must provide a session ID for exec") } - // create sync pipe to receive the pid - parentSyncPipe, childSyncPipe, err := newPipe() - if err != nil { - return -1, nil, errors.Wrapf(err, "error creating socket pair") - } - - defer errorhandling.CloseQuiet(parentSyncPipe) - - // create start pipe to set the cgroup before running - // attachToExec is responsible for closing parentStartPipe - childStartPipe, parentStartPipe, err := newPipe() - if err != nil { - return -1, nil, errors.Wrapf(err, "error creating socket pair") - } - - // We want to make sure we close the parent{Start,Attach}Pipes if we fail - // but also don't want to close them after attach to exec is called - attachToExecCalled := false - - defer func() { - if !attachToExecCalled { - errorhandling.CloseQuiet(parentStartPipe) - } - }() - - // create the attach pipe to allow attach socket to be created before - // $RUNTIME exec starts running. This is to make sure we can capture all output - // from the process through that socket, rather than half reading the log, half attaching to the socket - // attachToExec is responsible for closing parentAttachPipe - parentAttachPipe, childAttachPipe, err := newPipe() - if err != nil { - return -1, nil, errors.Wrapf(err, "error creating socket pair") - } - - defer func() { - if !attachToExecCalled { - errorhandling.CloseQuiet(parentAttachPipe) - } - }() - - childrenClosed := false - defer func() { - if !childrenClosed { - errorhandling.CloseQuiet(childSyncPipe) - errorhandling.CloseQuiet(childAttachPipe) - errorhandling.CloseQuiet(childStartPipe) - } - }() - - runtimeDir, err := util.GetRuntimeDir() - if err != nil { - return -1, nil, err - } - - finalEnv := make([]string, 0, len(options.Env)) - for k, v := range options.Env { - finalEnv = append(finalEnv, fmt.Sprintf("%s=%s", k, v)) - } - - processFile, err := prepareProcessExec(c, options.Cmd, finalEnv, options.Terminal, options.Cwd, options.User, sessionID) - if err != nil { - return -1, nil, err + // TODO: Should we default this to false? + // Or maybe make streams mandatory? + attachStdin := true + if streams != nil { + attachStdin = streams.AttachInput } var ociLog string if logrus.GetLevel() != logrus.DebugLevel && r.supportsJSON { ociLog = c.execOCILog(sessionID) } - args := r.sharedConmonArgs(c, sessionID, c.execBundlePath(sessionID), c.execPidPath(sessionID), c.execLogPath(sessionID), c.execExitFileDir(sessionID), ociLog, "") - - if options.PreserveFDs > 0 { - args = append(args, formatRuntimeOpts("--preserve-fds", fmt.Sprintf("%d", options.PreserveFDs))...) - } - - for _, capability := range options.CapAdd { - args = append(args, formatRuntimeOpts("--cap", capability)...) - } - - if options.Terminal { - args = append(args, "-t") - } - - if streams != nil && streams.AttachInput { - args = append(args, "-i") - } - // Append container ID and command - args = append(args, "-e") - // TODO make this optional when we can detach - args = append(args, "--exec-attach") - args = append(args, "--exec-process-spec", processFile.Name()) - - logrus.WithFields(logrus.Fields{ - "args": args, - }).Debugf("running conmon: %s", r.conmonPath) - execCmd := exec.Command(r.conmonPath, args...) - - if streams != nil { - // Don't add the InputStream to the execCmd. Instead, the data should be passed - // through CopyDetachable - if streams.AttachOutput { - execCmd.Stdout = options.Streams.OutputStream - } - if streams.AttachError { - execCmd.Stderr = options.Streams.ErrorStream - } - } - - conmonEnv, extraFiles, err := r.configureConmonEnv(runtimeDir) + execCmd, pipes, err := r.startExec(c, sessionID, options, attachStdin, ociLog) if err != nil { return -1, nil, err } - if options.PreserveFDs > 0 { - for fd := 3; fd < int(3+options.PreserveFDs); fd++ { - execCmd.ExtraFiles = append(execCmd.ExtraFiles, os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd))) - } - } - - // we don't want to step on users fds they asked to preserve - // Since 0-2 are used for stdio, start the fds we pass in at preserveFDs+3 - execCmd.Env = r.conmonEnv - execCmd.Env = append(execCmd.Env, fmt.Sprintf("_OCI_SYNCPIPE=%d", options.PreserveFDs+3), fmt.Sprintf("_OCI_STARTPIPE=%d", options.PreserveFDs+4), fmt.Sprintf("_OCI_ATTACHPIPE=%d", options.PreserveFDs+5)) - execCmd.Env = append(execCmd.Env, conmonEnv...) - - execCmd.ExtraFiles = append(execCmd.ExtraFiles, childSyncPipe, childStartPipe, childAttachPipe) - execCmd.ExtraFiles = append(execCmd.ExtraFiles, extraFiles...) - execCmd.Dir = c.execBundlePath(sessionID) - execCmd.SysProcAttr = &syscall.SysProcAttr{ - Setpgid: true, - } - - err = startCommandGivenSelinux(execCmd) - - // We don't need children pipes on the parent side - errorhandling.CloseQuiet(childSyncPipe) - errorhandling.CloseQuiet(childAttachPipe) - errorhandling.CloseQuiet(childStartPipe) - childrenClosed = true - - if err != nil { - return -1, nil, errors.Wrapf(err, "cannot start container %s", c.ID()) - } - if err := r.moveConmonToCgroupAndSignal(c, execCmd, parentStartPipe); err != nil { - return -1, nil, err - } - - if options.PreserveFDs > 0 { - for fd := 3; fd < int(3+options.PreserveFDs); fd++ { - // These fds were passed down to the runtime. Close them - // and not interfere - if err := os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd)).Close(); err != nil { - logrus.Debugf("unable to close file fd-%d", fd) - } + // Only close sync pipe. Start and attach are consumed in the attach + // goroutine. + defer func() { + if pipes.syncPipe != nil && !pipes.syncClosed { + errorhandling.CloseQuiet(pipes.syncPipe) + pipes.syncClosed = true } - } + }() // TODO Only create if !detach // Attach to the container before starting it attachChan := make(chan error) go func() { // attachToExec is responsible for closing pipes - attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, parentStartPipe, parentAttachPipe) + attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, pipes.startPipe, pipes.attachPipe) close(attachChan) }() - attachToExecCalled = true if err := execCmd.Wait(); err != nil { return -1, nil, errors.Wrapf(err, "cannot run conmon") } - pid, err := readConmonPipeData(parentSyncPipe, ociLog) + pid, err := readConmonPipeData(pipes.syncPipe, ociLog) return pid, attachChan, err } @@ -1949,9 +1816,6 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex } }() - // TODOTODOTODO - //defer errorhandling.CloseQuiet(parentSyncPipe) - // create start pipe to set the cgroup before running // attachToExec is responsible for closing parentStartPipe childStartPipe, parentStartPipe, err := newPipe() @@ -1960,17 +1824,6 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex } pipes.startPipe = parentStartPipe - // We want to make sure we close the parent{Start,Attach}Pipes if we fail - // but also don't want to close them after attach to exec is called - //attachToExecCalled := false - - // TODOTODOTODO - // defer func() { - // if !attachToExecCalled { - // errorhandling.CloseQuiet(parentStartPipe) - // } - // }() - // create the attach pipe to allow attach socket to be created before // $RUNTIME exec starts running. This is to make sure we can capture all output // from the process through that socket, rather than half reading the log, half attaching to the socket -- cgit v1.2.3-54-g00ecf