Fix issue killing container while sending signal

Fix rebase issue
Update docs

api.go

@@ -42,6 +42,9 @@ func hijackServer(w http.ResponseWriter) (io.ReadCloser, io.Writer, error) {
 
 //If we don't do this, POST method without Content-type (even with empty body) will fail
 func parseForm(r *http.Request) error {
+	if r == nil {
+		return nil
+	}
 	if err := r.ParseForm(); err != nil && !strings.HasPrefix(err.Error(), "mime:") {
 		return err
 	}
@@ -139,13 +142,16 @@ func postContainersKill(srv *Server, version float64, w http.ResponseWriter, r *
 		return err
 	}
 	name := vars["name"]
-	s := r.Form.Get("signal")
-	signal := 9
-	if s != "" {
-		if s, err := strconv.Atoi(s); err != nil {
-			return err
-		} else {
-			signal = s
+
+	signal := 0
+	if r != nil {
+		s := r.Form.Get("signal")
+		if s != "" {
+			if s, err := strconv.Atoi(s); err != nil {
+				return err
+			} else {
+				signal = s
+			}
 		}
 	}
 

api_test.go

@@ -347,6 +347,8 @@ func TestGetContainersJSON(t *testing.T) {
 
 	srv := &Server{runtime: runtime}
 
+	beginLen := runtime.containers.Len()
+
 	container, err := runtime.Create(&Config{
 		Image: GetTestImage(runtime).ID,
 		Cmd:   []string{"echo", "test"},
@@ -370,7 +372,7 @@ func TestGetContainersJSON(t *testing.T) {
 		t.Fatal(err)
 	}
 	if len(containers) != 1 {
-		t.Fatalf("Expected %d container, %d found", 1, len(containers))
+		t.Fatalf("Expected %d container, %d found (started with: %d)", 1, len(containers), beginLen)
 	}
 	if containers[0].ID != container.ID {
 		t.Fatalf("Container ID mismatch. Expected: %s, received: %s\n", container.ID, containers[0].ID)

commands.go

@@ -545,6 +545,18 @@ func (cli *DockerCli) CmdRestart(args ...string) error {
 	return nil
 }
 
+func (cli *DockerCli) forwardAllSignals(cid string) {
+	sigc := make(chan os.Signal, 1)
+	utils.CatchAll(sigc)
+	go func() {
+		for s := range sigc {
+			if _, _, err := cli.call("POST", fmt.Sprintf("/containers/%s/kill?signal=%d", cid, s), nil); err != nil {
+				utils.Debugf("Error sending signal: %s", err)
+			}
+		}
+	}()
+}
+
 func (cli *DockerCli) CmdStart(args ...string) error {
 	cmd := Subcmd("start", "CONTAINER [CONTAINER...]", "Restart a stopped container")
 	attach := cmd.Bool("a", false, "Attach container's stdout/stderr and forward all signals to the process")
@@ -575,15 +587,7 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 		}
 
 		if !container.Config.Tty {
-			sigc := make(chan os.Signal, 1)
-			utils.CatchAll(sigc)
-			go func() {
-				for s := range sigc {
-					if _, _, err := cli.call("POST", fmt.Sprintf("/containers/%s/kill?signal=%d", cmd.Arg(0), s), nil); err != nil {
-						utils.Debugf("Error sending signal: %s", err)
-					}
-				}
-			}()
+			cli.forwardAllSignals(cmd.Arg(0))
 		}
 
 		if container.Config.Tty {
@@ -601,7 +605,7 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 		v.Set("stderr", "1")
 
 		cErr = utils.Go(func() error {
-			return cli.hijack("POST", "/containers/"+cmd.Arg(0)+"/attach?"+v.Encode(), container.Config.Tty, cli.in, cli.out)
+			return cli.hijack("POST", "/containers/"+cmd.Arg(0)+"/attach?"+v.Encode(), container.Config.Tty, cli.in, cli.out, cli.err)
 		})
 	}
 
@@ -1301,9 +1305,9 @@ func (cli *DockerCli) CmdLogs(args ...string) error {
 }
 
 func (cli *DockerCli) CmdAttach(args ...string) error {
-	cmd := Subcmd("attach", "CONTAINER", "Attach to a running container")
+	cmd := Subcmd("attach", "[OPTIONS] CONTAINER", "Attach to a running container")
 	noStdin := cmd.Bool("nostdin", false, "Do not attach stdin")
-	proxy := cmd.Bool("proxy", false, "Proxify all received signal to the process (even in non-tty mode)")
+	proxy := cmd.Bool("sig-proxy", false, "Proxify all received signal to the process (even in non-tty mode)")
 	if err := cmd.Parse(args); err != nil {
 		return nil
 	}
@@ -1342,15 +1346,7 @@ func (cli *DockerCli) CmdAttach(args ...string) error {
 	v.Set("stderr", "1")
 
 	if *proxy && !container.Config.Tty {
-		sigc := make(chan os.Signal, 1)
-		utils.CatchAll(sigc)
-		go func() {
-			for s := range sigc {
-				if _, _, err := cli.call("POST", fmt.Sprintf("/containers/%s/kill?signal=%d", cmd.Arg(0), s), nil); err != nil {
-					utils.Debugf("Error sending signal: %s", err)
-				}
-			}
-		}()
+		cli.forwardAllSignals(cmd.Arg(0))
 	}
 
 	if err := cli.hijack("POST", "/containers/"+cmd.Arg(0)+"/attach?"+v.Encode(), container.Config.Tty, cli.in, cli.out, cli.err); err != nil {
@@ -1516,6 +1512,9 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 	flRm := cmd.Lookup("rm")
 	autoRemove, _ := strconv.ParseBool(flRm.Value.String())
 
+	flSigProxy := cmd.Lookup("sig-proxy")
+	sigProxy, _ := strconv.ParseBool(flSigProxy.Value.String())
+
 	var containerIDFile *os.File
 	if len(hostConfig.ContainerIDFile) > 0 {
 		if _, err := ioutil.ReadFile(hostConfig.ContainerIDFile); err == nil {
@@ -1594,6 +1593,10 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 		}
 	}
 
+	if sigProxy {
+		cli.forwardAllSignals(runResult.ID)
+	}
+
 	//start the container
 	if _, _, err = cli.call("POST", "/containers/"+runResult.ID+"/start", hostConfig); err != nil {
 		return err

commands_test.go

@@ -393,7 +393,7 @@ func TestRunDetach(t *testing.T) {
 	container := globalRuntime.List()[0]
 
 	setTimeout(t, "Escape sequence timeout", 5*time.Second, func() {
-		stdinPipe.Write([]byte{'', ''})
+		stdinPipe.Write([]byte{16, 17})
 		if err := stdinPipe.Close(); err != nil {
 			t.Fatal(err)
 		}
@@ -411,7 +411,6 @@ func TestRunDetach(t *testing.T) {
 
 	setTimeout(t, "Waiting for container to die timed out", 20*time.Second, func() {
 		container.Kill()
-		container.Wait()
 	})
 }
 
@@ -451,7 +450,7 @@ func TestAttachDetach(t *testing.T) {
 	})
 
 	setTimeout(t, "Escape sequence timeout", 5*time.Second, func() {
-		stdinPipe.Write([]byte{'', ''})
+		stdinPipe.Write([]byte{16, 17})
 		if err := stdinPipe.Close(); err != nil {
 			t.Fatal(err)
 		}
@@ -469,7 +468,6 @@ func TestAttachDetach(t *testing.T) {
 
 	setTimeout(t, "Waiting for container to die timedout", 5*time.Second, func() {
 		container.Kill()
-		container.Wait()
 	})
 }
 

container.go

@@ -99,7 +99,10 @@ type BindMap struct {
 }
 
 var (
-	ErrInvaidWorikingDirectory = errors.New("The working directory is invalid. It needs to be an absolute path.")
+	ErrInvalidWorikingDirectory = errors.New("The working directory is invalid. It needs to be an absolute path.")
+	ErrConflictTtySigProxy      = errors.New("TTY mode (-t) already imply signal proxying (-sig-proxy)")
+	ErrConflictAttachDetach     = errors.New("Conflicting options: -a and -d")
+	ErrConflictDetachAutoRemove = errors.New("Conflicting options: -rm and -d")
 )
 
 type KeyValuePair struct {
@@ -127,6 +130,7 @@ func ParseRun(args []string, capabilities *Capabilities) (*Config, *HostConfig,
 	flNetwork := cmd.Bool("n", true, "Enable networking for this container")
 	flPrivileged := cmd.Bool("privileged", false, "Give extended privileges to this container")
 	flAutoRemove := cmd.Bool("rm", false, "Automatically remove the container when it exits (incompatible with -d)")
+	flSigProxy := cmd.Bool("sig-proxy", false, "Proxify all received signal to the process (even in non-tty mode)")
 
 	if capabilities != nil && *flMemory > 0 && !capabilities.MemoryLimit {
 		//fmt.Fprintf(stdout, "WARNING: Your kernel does not support memory limit capabilities. Limitation discarded.\n")
@@ -159,11 +163,18 @@ func ParseRun(args []string, capabilities *Capabilities) (*Config, *HostConfig,
 		return nil, nil, cmd, err
 	}
 	if *flDetach && len(flAttach) > 0 {
-		return nil, nil, cmd, fmt.Errorf("Conflicting options: -a and -d")
+		return nil, nil, cmd, ErrConflictAttachDetach
 	}
 	if *flWorkingDir != "" && !path.IsAbs(*flWorkingDir) {
-		return nil, nil, cmd, ErrInvaidWorikingDirectory
+		return nil, nil, cmd, ErrInvalidWorikingDirectory
 	}
+	if *flTty && *flSigProxy {
+		return nil, nil, cmd, ErrConflictTtySigProxy
+	}
+	if *flDetach && *flAutoRemove {
+		return nil, nil, cmd, ErrConflictDetachAutoRemove
+	}
+
 	// If neither -d or -a are set, attach to everything by default
 	if len(flAttach) == 0 && !*flDetach {
 		if !*flDetach {
@@ -175,10 +186,6 @@ func ParseRun(args []string, capabilities *Capabilities) (*Config, *HostConfig,
 		}
 	}
 
-	if *flDetach && *flAutoRemove {
-		return nil, nil, cmd, fmt.Errorf("Conflicting options: -rm and -d")
-	}
-
 	var binds []string
 
 	// add any bind targets to the list of container volumes
@@ -1045,51 +1052,54 @@ func (container *Container) cleanup() {
 }
 
 func (container *Container) kill(sig int) error {
+	container.State.Lock()
+	defer container.State.Unlock()
+
 	if !container.State.Running {
 		return nil
 	}
 
-	// Sending SIGKILL to the process via lxc
-	output, err := exec.Command("lxc-kill", "-n", container.ID, strconv.Itoa(sig)).CombinedOutput()
-	if err != nil {
-		log.Printf("error killing container %s (%s, %s)", container.ID, output, err)
+	if output, err := exec.Command("lxc-kill", "-n", container.ID, strconv.Itoa(sig)).CombinedOutput(); err != nil {
+		log.Printf("error killing container %s (%s, %s)", container.ShortID(), output, err)
+		return err
+	}
+
+	return nil
+}
+
+func (container *Container) Kill() error {
+	if !container.State.Running {
+		return nil
+	}
+
+	// 1. Send SIGKILL
+	if err := container.kill(9); err != nil {
+		return err
 	}
 
 	// 2. Wait for the process to die, in last resort, try to kill the process directly
 	if err := container.WaitTimeout(10 * time.Second); err != nil {
 		if container.cmd == nil {
-			return fmt.Errorf("lxc-kill failed, impossible to kill the container %s", container.ID)
+			return fmt.Errorf("lxc-kill failed, impossible to kill the container %s", container.ShortID())
 		}
-		log.Printf("Container %s failed to exit within 10 seconds of lxc-kill %d  - trying direct SIGKILL", sig, container.ID)
+		log.Printf("Container %s failed to exit within 10 seconds of lxc-kill %s - trying direct SIGKILL", "SIGKILL", container.ShortID())
 		if err := container.cmd.Process.Kill(); err != nil {
 			return err
 		}
 	}
 
-	// Wait for the container to be actually stopped
 	container.Wait()
 	return nil
 }
 
-func (container *Container) Kill(sig int) error {
-	container.State.Lock()
-	defer container.State.Unlock()
-	if !container.State.Running {
-		return nil
-	}
-	return container.kill(sig)
-}
-
 func (container *Container) Stop(seconds int) error {
-	container.State.Lock()
-	defer container.State.Unlock()
 	if !container.State.Running {
 		return nil
 	}
 
 	// 1. Send a SIGTERM
-	if output, err := exec.Command("lxc-kill", "-n", container.ID, "15").CombinedOutput(); err != nil {
-		log.Print(string(output))
+	if err := container.kill(15); err != nil {
+		utils.Debugf("Error sending kill SIGTERM: %s", err)
 		log.Print("Failed to send SIGTERM to the process, force killing")
 		if err := container.kill(9); err != nil {
 			return err
@@ -1099,7 +1109,8 @@ func (container *Container) Stop(seconds int) error {
 	// 2. Wait for the process to exit on its own
 	if err := container.WaitTimeout(time.Duration(seconds) * time.Second); err != nil {
 		log.Printf("Container %v failed to exit within %d seconds of SIGTERM - using the force", container.ID, seconds)
-		if err := container.kill(9); err != nil {
+		// 3. If it doesn't, then send SIGKILL
+		if err := container.Kill(); err != nil {
 			return err
 		}
 	}

docs/sources/api/docker_remote_api_v1.6.rst

@@ -443,6 +443,7 @@ Kill a container
 
 	   HTTP/1.1 204 OK
 
+	:query signal: Signal to send to the container (integer). When not set, SIGKILL is assumed and the call will waits for the container to exit.
 	:statuscode 204: no error
 	:statuscode 404: no such container
 	:statuscode 500: server error

docs/sources/commandline/cli.rst

@@ -29,6 +29,9 @@ To list available commands, either run ``docker`` with no parameters or execute
 
     Attach to a running container.
 
+      -nostdin=false: Do not attach stdin
+      -sig-proxy=false: Proxify all received signal to the process (even in non-tty mode)
+
 You can detach from the container again (and leave it running) with
 ``CTRL-c`` (for a quiet exit) or ``CTRL-\`` to get a stacktrace of
 the Docker client when it quits.
@@ -396,7 +399,7 @@ Insert file from github
 
 ::
 
-    Usage: docker kill [OPTIONS] CONTAINER [CONTAINER...]
+    Usage: docker kill CONTAINER [CONTAINER...]
 
     Kill a running container
 
@@ -550,6 +553,7 @@ Insert file from github
       -entrypoint="": Overwrite the default entrypoint set by the image.
       -w="": Working directory inside the container
       -lxc-conf=[]: Add custom lxc options -lxc-conf="lxc.cgroup.cpuset.cpus = 0,1"
+      -sig-proxy=false: Proxify all received signal to the process (even in non-tty mode)
 
 Examples
 ~~~~~~~~
@@ -623,6 +627,9 @@ using the container, but inside the current working directory.
 
     Start a stopped container
 
+      -a=false: Attach container's stdout/stderr and forward all signals to the process
+      -i=false: Attach container's stdin
+
 .. _cli_stop:
 
 ``stop``

server.go

@@ -55,8 +55,7 @@ func (v *simpleVersionInfo) Version() string {
 // Such information will be used on call to NewRegistry().
 func (srv *Server) versionInfos() []utils.VersionInfo {
 	v := srv.DockerVersion()
-	ret := make([]utils.VersionInfo, 0, 4)
-	ret = append(ret, &simpleVersionInfo{"docker", v.Version})
+	ret := append(make([]utils.VersionInfo, 0, 4), &simpleVersionInfo{"docker", v.Version})
 
 	if len(v.GoVersion) > 0 {
 		ret = append(ret, &simpleVersionInfo{"go", v.GoVersion})
@@ -64,20 +63,32 @@ func (srv *Server) versionInfos() []utils.VersionInfo {
 	if len(v.GitCommit) > 0 {
 		ret = append(ret, &simpleVersionInfo{"git-commit", v.GitCommit})
 	}
-	kernelVersion, err := utils.GetKernelVersion()
-	if err == nil {
+	if kernelVersion, err := utils.GetKernelVersion(); err == nil {
 		ret = append(ret, &simpleVersionInfo{"kernel", kernelVersion.String()})
 	}
 
 	return ret
 }
 
+// ContainerKill send signal to the container
+// If no signal is given (sig 0), then Kill with SIGKILL and wait
+// for the container to exit.
+// If a signal is given, then just send it to the container and return.
 func (srv *Server) ContainerKill(name string, sig int) error {
 	if container := srv.runtime.Get(name); container != nil {
-		if err := container.Kill(sig); err != nil {
-			return fmt.Errorf("Error killing container %s: %s", name, err)
+		// If no signal is passed, perform regular Kill (SIGKILL + wait())
+		if sig == 0 {
+			if err := container.Kill(); err != nil {
+				return fmt.Errorf("Error killing container %s: %s", name, err)
+			}
+			srv.LogEvent("kill", container.ShortID(), srv.runtime.repositories.ImageName(container.Image))
+		} else {
+			// Otherwise, just send the requested signal
+			if err := container.kill(sig); err != nil {
+				return fmt.Errorf("Error killing container %s: %s", name, err)
+			}
+			// FIXME: Add event for signals
 		}
-		srv.LogEvent("kill", container.ShortID(), srv.runtime.repositories.ImageName(container.Image))
 	} else {
 		return fmt.Errorf("No such container: %s", name)
 	}

server_test.go

@@ -188,33 +188,28 @@ func TestCreateStartRestartStopStartKillRm(t *testing.T) {
 		t.Errorf("Expected 1 container, %v found", len(runtime.List()))
 	}
 
-	err = srv.ContainerStart(id, hostConfig)
-	if err != nil {
+	if err := srv.ContainerStart(id, hostConfig); err != nil {
 		t.Fatal(err)
 	}
 
-	err = srv.ContainerRestart(id, 1)
-	if err != nil {
+	if err := srv.ContainerRestart(id, 1); err != nil {
 		t.Fatal(err)
 	}
 
-	err = srv.ContainerStop(id, 1)
-	if err != nil {
+	if err := srv.ContainerStop(id, 1); err != nil {
 		t.Fatal(err)
 	}
 
-	err = srv.ContainerStart(id, hostConfig)
-	if err != nil {
+	if err := srv.ContainerStart(id, hostConfig); err != nil {
 		t.Fatal(err)
 	}
 
-	err = srv.ContainerKill(id)
-	if err != nil {
+	if err := srv.ContainerKill(id, 0); err != nil {
 		t.Fatal(err)
 	}
 
 	// FIXME: this failed once with a race condition ("Unable to remove filesystem for xxx: directory not empty")
-	if err = srv.ContainerDestroy(id, true); err != nil {
+	if err := srv.ContainerDestroy(id, true); err != nil {
 		t.Fatal(err)
 	}
 
@@ -225,20 +220,18 @@ func TestCreateStartRestartStopStartKillRm(t *testing.T) {
 }
 
 func TestRunWithTooLowMemoryLimit(t *testing.T) {
-	var err error
 	runtime := mkRuntime(t)
-	srv := &Server{runtime: runtime}
 	defer nuke(runtime)
+
 	// Try to create a container with a memory limit of 1 byte less than the minimum allowed limit.
-	_, err = srv.ContainerCreate(
+	if _, err := (*Server).ContainerCreate(&Server{runtime: runtime},
 		&Config{
 			Image:     GetTestImage(runtime).ID,
 			Memory:    524287,
 			CpuShares: 1000,
 			Cmd:       []string{"/bin/cat"},
 		},
-	)
-	if err == nil {
+	); err == nil {
 		t.Errorf("Memory limit is smaller than the allowed limit. Container creation should've failed!")
 	}
 
@@ -246,10 +239,12 @@ func TestRunWithTooLowMemoryLimit(t *testing.T) {
 
 func TestContainerTop(t *testing.T) {
 	t.Skip("Fixme. Skipping test for now. Reported error: 'server_test.go:236: Expected 2 processes, found 1.'")
+
 	runtime := mkRuntime(t)
-	srv := &Server{runtime: runtime}
 	defer nuke(runtime)
 
+	srv := &Server{runtime: runtime}
+
 	c, hostConfig, _ := mkContainer(runtime, []string{"_", "/bin/sh", "-c", "sleep 2"}, t)
 	c, hostConfig, err := mkContainer(runtime, []string{"_", "/bin/sh", "-c", "sleep 2"}, t)
 	if err != nil {