[lxc-devel] [lxd/master] exec: remove ExecNoWait() and improve Exec()
brauner on Github
lxc-bot at linuxcontainers.org
Thu Nov 3 15:35:15 UTC 2016
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1332 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20161103/df4d9e70/attachment.bin>
-------------- next part --------------
From 58cd1899532c5c85c2dc29f0adcd2a7dbdcea284 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at canonical.com>
Date: Thu, 3 Nov 2016 16:26:31 +0100
Subject: [PATCH] exec: remove ExecNoWait() and improve Exec()
ExecNoWait() was an unnecessary method addition to the containerLXC struct. It
also complicated the lxd forkexec command. I mainly added it because I
considered the methods defined on the containerLXC struct to be part of our API.
Now that I know better, let's simplify this code.
- Exec() gains an additional int pointer argument attachedPid.
1. passing in nil for attachedPid
- equivalent to calling cmd.Run()
2. passing in non-nil for attachedPid
- return PID of the lxd forkexec command and store the PID of the
executing process in attachedPid. It's the callers responsibility to
wait on the command. (Note. The PID returned in attachedPid can not be
waited upon since it's a child of the lxd forkexec command. It can
however be used to e.g. forward signals.)
- Move all the pipe magic into Exec() itself. This way we avoid exposing callers
to internals and take away the responsibility to retrieve the PID themselves.
Signed-off-by: Christian Brauner <christian.brauner at canonical.com>
---
lxd/container.go | 23 +++++++-------
lxd/container_exec.go | 54 +++++++++++++++----------------
lxd/container_lxc.go | 56 ++++++++++++++++----------------
lxd/containers.go | 88 ++++++++++++++++++++++-----------------------------
4 files changed, 102 insertions(+), 119 deletions(-)
diff --git a/lxd/container.go b/lxd/container.go
index a9df86c..94a0410 100644
--- a/lxd/container.go
+++ b/lxd/container.go
@@ -4,7 +4,6 @@ import (
"fmt"
"io"
"os"
- "os/exec"
"strings"
"time"
@@ -351,16 +350,18 @@ type container interface {
FilePull(srcpath string, dstpath string) (int, int, os.FileMode, string, []string, error)
FilePush(srcpath string, dstpath string, uid int, gid int, mode int) error
- // Command execution: Execute command and wait for it to exit.
- Exec(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File) (int, error)
-
- // Command execution: Returns a command. If called as exec.Cmd.Start()
- // ExecNoWait() does not wait for the process to exit. Callers are
- // expected to pass the write end of a pipe to the pidPipe argument. So
- // they can read the PID of the executing process from the read end. The
- // PID read cannot be directly waited upon since it is a child of lxd
- // forkexec. It can however be used to e.g. send signals.
- ExecNoWait(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File, pidPipe *os.File) (*exec.Cmd, error)
+ /* Command execution:
+ * 1. passing in nil for attachedPid
+ * - equivalent to calling cmd.Run()
+ * 2. passing in non-nil for attachedPid
+ * - return PID of the lxd forkexec command and store the PID of the
+ * executing process in attachedPid. It's the callers
+ * responsibility to wait on the command. (Note. The PID returned
+ * in attachedPid can not be waited upon since it's a child of the
+ * lxd forkexec command. It can however be used to e.g. forward
+ * signals.)
+ */
+ Exec(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File, attachedPid *int) (int, error)
// Status
Render() (interface{}, interface{}, error)
diff --git a/lxd/container_exec.go b/lxd/container_exec.go
index 4272b4e..4cce303 100644
--- a/lxd/container_exec.go
+++ b/lxd/container_exec.go
@@ -6,7 +6,6 @@ import (
"io/ioutil"
"net/http"
"os"
- "os/exec"
"path/filepath"
"strconv"
"strings"
@@ -254,48 +253,45 @@ func (s *execWs) Do(op *operation) error {
return cmdErr
}
- r, w, err := shared.Pipe()
- defer r.Close()
+ attachedPid := -1
+ pid, err := s.container.Exec(s.command, s.env, stdin, stdout, stderr, &attachedPid)
if err != nil {
- shared.LogErrorf("s", err)
return err
}
- cmd, err := s.container.ExecNoWait(s.command, s.env, stdin, stdout, stderr, w)
- if err != nil {
- w.Close()
- return err
+ if s.interactive {
+ receivePid <- attachedPid
}
- err = cmd.Start()
+ proc, err := os.FindProcess(pid)
if err != nil {
- w.Close()
- return err
+ return finisher(-1, fmt.Errorf("Failed finding process: %q", err))
}
- w.Close()
- attachedPid := -1
- if err := json.NewDecoder(r).Decode(&attachedPid); err != nil {
- shared.LogErrorf("Failed to retrieve PID of executing child process: %s", err)
- return finisher(-1, err)
+ procState, err := proc.Wait()
+ if err != nil {
+ return finisher(-1, fmt.Errorf("Failed waiting on process %d: %q", pid, err))
}
- if s.interactive {
- receivePid <- attachedPid
+ if procState.Success() {
+ return finisher(0, nil)
}
- err = cmd.Wait()
- if err != nil {
- exitErr, ok := err.(*exec.ExitError)
- if ok {
- status, ok := exitErr.Sys().(syscall.WaitStatus)
- if ok {
- return finisher(status.ExitStatus(), nil)
- }
+ status, ok := procState.Sys().(syscall.WaitStatus)
+ if ok {
+ if status.Exited() {
+ return finisher(status.ExitStatus(), nil)
+ }
+ // Backwards compatible behavior. Report success when we exited
+ // due to a signal. Otherwise this may break Jenkins, e.g. when
+ // lxc exec foo reboot receives SIGTERM and status.Exitstats()
+ // would report -1.
+ if status.Signaled() {
+ return finisher(0, nil)
}
}
- return finisher(0, nil)
+ return finisher(-1, nil)
}
func containerExecPost(d *Daemon, r *http.Request) Response {
@@ -407,7 +403,7 @@ func containerExecPost(d *Daemon, r *http.Request) Response {
defer stderr.Close()
// Run the command
- cmdResult, cmdErr = c.Exec(post.Command, env, nil, stdout, stderr)
+ cmdResult, cmdErr = c.Exec(post.Command, env, nil, stdout, stderr, nil)
// Update metadata with the right URLs
metadata["return"] = cmdResult
@@ -416,7 +412,7 @@ func containerExecPost(d *Daemon, r *http.Request) Response {
"2": fmt.Sprintf("/%s/containers/%s/logs/%s", shared.APIVersion, c.Name(), filepath.Base(stderr.Name())),
}
} else {
- cmdResult, cmdErr = c.Exec(post.Command, env, nil, nil, nil)
+ cmdResult, cmdErr = c.Exec(post.Command, env, nil, nil, nil, nil)
metadata["return"] = cmdResult
}
diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 640618e..d48a8d1 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -3846,24 +3846,14 @@ func (c *containerLXC) FilePush(srcpath string, dstpath string, uid int, gid int
return nil
}
-func (c *containerLXC) prepareExec(waitOnPid bool, command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File) ([]string, *exec.Cmd) {
+func (c *containerLXC) Exec(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File, attachedPid *int) (int, error) {
envSlice := []string{}
for k, v := range env {
envSlice = append(envSlice, fmt.Sprintf("%s=%s", k, v))
}
- // Setting "wait"/"nowait" as the second argument in cmd.Args allows us
- // to tell execContainer() that it should call an appropriate go-lxc
- // wrapper for c->attach() that executes the command we requested in the
- // container and, depending on whether we passed "wait" or "nowait" does
- // wait or not wait for it to exit.
- var args []string
- if waitOnPid {
- args = []string{execPath, "forkexec", "wait", c.name, c.daemon.lxcpath, filepath.Join(c.LogPath(), "lxc.conf")}
- } else {
- args = []string{execPath, "forkexec", "nowait", c.name, c.daemon.lxcpath, filepath.Join(c.LogPath(), "lxc.conf")}
- }
+ args := []string{execPath, "forkexec", c.name, c.daemon.lxcpath, filepath.Join(c.LogPath(), "lxc.conf")}
args = append(args, "--")
args = append(args, "env")
@@ -3880,37 +3870,47 @@ func (c *containerLXC) prepareExec(waitOnPid bool, command []string, env map[str
cmd.Stdout = stdout
cmd.Stderr = stderr
- shared.LogInfo("Executing command", log.Ctx{"environment": envSlice, "args": args})
- return envSlice, &cmd
-}
+ r, w, err := shared.Pipe()
+ defer r.Close()
+ if err != nil {
+ shared.LogErrorf("s", err)
+ return -1, err
+ }
-func (c *containerLXC) Exec(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File) (int, error) {
- envSlice, cmd := c.prepareExec(true, command, env, stdin, stdout, stderr)
- err := cmd.Run()
+ cmd.ExtraFiles = []*os.File{w}
+ err = cmd.Start()
+ if err != nil {
+ w.Close()
+ return -1, err
+ }
+ w.Close()
+ pid := -1
+ if err := json.NewDecoder(r).Decode(&pid); err != nil {
+ shared.LogErrorf("Failed to retrieve PID of executing child process: %s", err)
+ return -1, err
+ }
+
+ // It's the callers responsibility to wait or not wait.
+ if attachedPid != nil {
+ *attachedPid = pid
+ return cmd.Process.Pid, nil
+ }
+
+ err = cmd.Wait()
if err != nil {
exitErr, ok := err.(*exec.ExitError)
if ok {
status, ok := exitErr.Sys().(syscall.WaitStatus)
if ok {
- shared.LogInfo("Executed command", log.Ctx{"environment": envSlice, "args": cmd.Args, "exit_status": status.ExitStatus()})
return status.ExitStatus(), nil
}
}
-
- shared.LogInfo("Failed executing command", log.Ctx{"environment": envSlice, "args": cmd.Args, "err": err})
return -1, err
}
- shared.LogInfo("Executed command", log.Ctx{"environment": envSlice, "args": cmd.Args})
return 0, nil
}
-func (c *containerLXC) ExecNoWait(command []string, env map[string]string, stdin *os.File, stdout *os.File, stderr *os.File, pidPipe *os.File) (*exec.Cmd, error) {
- _, cmd := c.prepareExec(false, command, env, stdin, stdout, stderr)
- cmd.ExtraFiles = []*os.File{pidPipe}
- return cmd, nil
-}
-
func (c *containerLXC) cpuState() shared.ContainerStateCPU {
cpu := shared.ContainerStateCPU{}
diff --git a/lxd/containers.go b/lxd/containers.go
index de576b7..206456c 100644
--- a/lxd/containers.go
+++ b/lxd/containers.go
@@ -282,13 +282,9 @@ func execContainer(args []string) (int, error) {
return -1, fmt.Errorf("Bad arguments: %q", args)
}
- wait := true
- if args[1] == "nowait" {
- wait = false
- }
- name := args[2]
- lxcpath := args[3]
- configPath := args[4]
+ name := args[1]
+ lxcpath := args[2]
+ configPath := args[3]
c, err := lxc.NewContainer(name, lxcpath)
if err != nil {
@@ -356,56 +352,46 @@ func execContainer(args []string) (int, error) {
opts.Env = env
- var status int
- if wait {
- status, err = c.RunCommandStatus(cmd, opts)
- if err != nil {
- return -1, fmt.Errorf("Failed running command and waiting for it to exit: %q", err)
- }
- } else {
- status, err = c.RunCommandNoWait(cmd, opts)
- if err != nil {
- return -1, fmt.Errorf("Failed running command: %q", err)
- }
- // Send the PID of the executing process.
- w := os.NewFile(uintptr(3), "attachedPid")
- defer w.Close()
+ status, err := c.RunCommandNoWait(cmd, opts)
+ if err != nil {
+ return -1, fmt.Errorf("Failed running command: %q", err)
+ }
+ // Send the PID of the executing process.
+ w := os.NewFile(uintptr(3), "attachedPid")
+ defer w.Close()
- err = json.NewEncoder(w).Encode(status)
- if err != nil {
- return -1, fmt.Errorf("Failed sending PID of executing command: %q", err)
- }
+ err = json.NewEncoder(w).Encode(status)
+ if err != nil {
+ return -1, fmt.Errorf("Failed sending PID of executing command: %q", err)
+ }
- proc, err := os.FindProcess(status)
- if err != nil {
- return -1, fmt.Errorf("Failed finding process: %q", err)
- }
+ proc, err := os.FindProcess(status)
+ if err != nil {
+ return -1, fmt.Errorf("Failed finding process: %q", err)
+ }
- procState, err := proc.Wait()
- if err != nil {
- return -1, fmt.Errorf("Failed waiting on process %d: %q", status, err)
- }
+ procState, err := proc.Wait()
+ if err != nil {
+ return -1, fmt.Errorf("Failed waiting on process %d: %q", status, err)
+ }
- if procState.Success() {
- return 0, nil
- }
+ if procState.Success() {
+ return 0, nil
+ }
- status, ok := procState.Sys().(syscall.WaitStatus)
- if ok {
- if status.Exited() {
- return status.ExitStatus(), nil
- }
- // Backwards compatible behavior. Report success when we exited
- // due to a signal. Otherwise this may break Jenkins, e.g. when
- // lxc exec foo reboot receives SIGTERM and status.Exitstats()
- // would report -1.
- if status.Signaled() {
- return 0, nil
- }
+ exCode, ok := procState.Sys().(syscall.WaitStatus)
+ if ok {
+ if exCode.Exited() {
+ return exCode.ExitStatus(), nil
+ }
+ // Backwards compatible behavior. Report success when we exited
+ // due to a signal. Otherwise this may break Jenkins, e.g. when
+ // lxc exec foo reboot receives SIGTERM and exCode.Exitstats()
+ // would report -1.
+ if exCode.Signaled() {
+ return 0, nil
}
-
- return -1, fmt.Errorf("Command failed")
}
- return status >> 8, nil
+ return -1, fmt.Errorf("Command failed")
}
More information about the lxc-devel
mailing list