From ad43d88af5bda8dc5b3d06f64de380bb985191ba Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Thu, 28 Nov 2013 16:12:45 -0800 Subject: [PATCH 01/19] Make race condition more obvious by performing more asserts --- integration/api_test.go | 6 +++--- integration/commands_test.go | 14 +++++++------- integration/container_test.go | 2 +- integration/server_test.go | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/integration/api_test.go b/integration/api_test.go index a66cbe561f..bd6280af8b 100644 --- a/integration/api_test.go +++ b/integration/api_test.go @@ -454,7 +454,7 @@ func TestGetContainersTop(t *testing.T) { // Make sure sh spawn up cat setTimeout(t, "read/write assertion timed out", 2*time.Second, func() { in, out := containerAttach(eng, containerID, t) - if err := assertPipe("hello\n", "hello", out, in, 15); err != nil { + if err := assertPipe("hello\n", "hello", out, in, 150); err != nil { t.Fatal(err) } }) @@ -877,7 +877,7 @@ func TestPostContainersAttach(t *testing.T) { }) setTimeout(t, "read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", string([]byte{1, 0, 0, 0, 0, 0, 0, 6})+"hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", string([]byte{1, 0, 0, 0, 0, 0, 0, 6})+"hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) @@ -956,7 +956,7 @@ func TestPostContainersAttachStderr(t *testing.T) { }) setTimeout(t, "read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", string([]byte{2, 0, 0, 0, 0, 0, 0, 6})+"hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", string([]byte{2, 0, 0, 0, 0, 0, 0, 6})+"hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) diff --git a/integration/commands_test.go b/integration/commands_test.go index 37bedf7f0c..7daebf3cd2 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -213,7 +213,7 @@ func TestRunExit(t *testing.T) { }() setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) @@ -268,7 +268,7 @@ func TestRunDisconnect(t *testing.T) { }() setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) @@ -330,7 +330,7 @@ func TestRunDisconnectTty(t *testing.T) { container := globalRuntime.List()[0] setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) @@ -432,7 +432,7 @@ func TestRunDetach(t *testing.T) { }() setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) @@ -513,7 +513,7 @@ func TestAttachDetach(t *testing.T) { }() setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { if err != io.ErrClosedPipe { t.Fatal(err) } @@ -575,7 +575,7 @@ func TestAttachDetachTruncatedID(t *testing.T) { }() setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { if err != io.ErrClosedPipe { t.Fatal(err) } @@ -648,7 +648,7 @@ func TestAttachDisconnect(t *testing.T) { }() setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() { - if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) diff --git a/integration/container_test.go b/integration/container_test.go index 93a00a7286..05eb48728c 100644 --- a/integration/container_test.go +++ b/integration/container_test.go @@ -462,7 +462,7 @@ func TestKillDifferentUser(t *testing.T) { setTimeout(t, "read/write assertion timed out", 2*time.Second, func() { out, _ := container.StdoutPipe() in, _ := container.StdinPipe() - if err := assertPipe("hello\n", "hello", out, in, 15); err != nil { + if err := assertPipe("hello\n", "hello", out, in, 150); err != nil { t.Fatal(err) } }) diff --git a/integration/server_test.go b/integration/server_test.go index 494e23fef3..24e109ab76 100644 --- a/integration/server_test.go +++ b/integration/server_test.go @@ -183,11 +183,11 @@ func TestCreateStartRestartStopStartKillRm(t *testing.T) { t.Fatal(err) } - if err := srv.ContainerRestart(id, 15); err != nil { + if err := srv.ContainerRestart(id, 150); err != nil { t.Fatal(err) } - if err := srv.ContainerStop(id, 15); err != nil { + if err := srv.ContainerStop(id, 150); err != nil { t.Fatal(err) } From 77c94175bdc387e7f43876f7097b970f67116054 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Thu, 28 Nov 2013 16:57:51 -0800 Subject: [PATCH 02/19] Make CopyEscapable consistent with Copy and return `nil` in case of success instead of io.EOF --- utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/utils.go b/utils/utils.go index cfdc73bb2e..f62aa12ff5 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -543,7 +543,7 @@ func CopyEscapable(dst io.Writer, src io.ReadCloser) (written int64, err error) if err := src.Close(); err != nil { return 0, err } - return 0, io.EOF + return 0, nil } } // ---- End of docker From fe727e2a87fa086d728664c396fd44f4be6d6afd Mon Sep 17 00:00:00 2001 From: cressie176 Date: Fri, 29 Nov 2013 10:02:53 +0000 Subject: [PATCH 03/19] Closing connection after ping --- registry/registry.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/registry/registry.go b/registry/registry.go index f02e3cf477..d3d9f2be54 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -47,6 +47,8 @@ func pingRegistryEndpoint(endpoint string) error { if err != nil { return err } + defer resp.Body.Close() + if resp.Header.Get("X-Docker-Registry-Version") == "" { return errors.New("This does not look like a Registry server (\"X-Docker-Registry-Version\" header not found in the response)") } From e535f544c7bc9c32b23e5505110a5513ff36be5a Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 07:39:51 -0800 Subject: [PATCH 04/19] Make sure the container is running before testing against it (TestAttachDetach) --- integration/commands_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/integration/commands_test.go b/integration/commands_test.go index 7daebf3cd2..8bc1c99ec7 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -481,6 +481,17 @@ func TestAttachDetach(t *testing.T) { var container *docker.Container + setTimeout(t, "Waiting for the container to be started timed out", 10*time.Second, func() { + for { + l := globalRuntime.List() + if len(l) == 1 && l[0].State.IsRunning() { + container = l[0] + break + } + time.Sleep(10 * time.Millisecond) + } + }) + setTimeout(t, "Reading container's id timed out", 10*time.Second, func() { buf := make([]byte, 1024) n, err := stdout.Read(buf) @@ -488,8 +499,6 @@ func TestAttachDetach(t *testing.T) { t.Fatal(err) } - container = globalRuntime.List()[0] - if strings.Trim(string(buf[:n]), " \r\n") != container.ID { t.Fatalf("Wrong ID received. Expect %s, received %s", container.ID, buf[:n]) } From fbebe20bc69648c046e3818ca744ae246092a782 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 07:40:44 -0800 Subject: [PATCH 05/19] Add a GetPtyMaster() method to container to retrieve the pty from an other package. We could also have put ptyMaster public, but then we need to ignore it in json otherwise the Marshalling fails. I think it is cleaner that way. --- container.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/container.go b/container.go index 49cb33b536..3e5f8a8d74 100644 --- a/container.go +++ b/container.go @@ -24,6 +24,11 @@ import ( "time" ) +var ( + ErrNotATTY = errors.New("The PTY is not a file") + ErrNoTTY = errors.New("No PTY found") +) + type Container struct { sync.Mutex root string // Path to the "home" of the container, including metadata. @@ -1405,3 +1410,13 @@ func (container *Container) Exposes(p Port) bool { _, exists := container.Config.ExposedPorts[p] return exists } + +func (container *Container) GetPtyMaster() (*os.File, error) { + if container.ptyMaster == nil { + return nil, ErrNoTTY + } + if pty, ok := container.ptyMaster.(*os.File); ok { + return pty, nil + } + return nil, ErrNotATTY +} From 67e9e0e11bb932ef9113ac91b2c1b0af6ee4db6d Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 07:42:26 -0800 Subject: [PATCH 06/19] Make the PTY in raw mode before assert test (TestAttachDetach) --- integration/commands_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/integration/commands_test.go b/integration/commands_test.go index 8bc1c99ec7..f2b14482b2 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/dotcloud/docker" "github.com/dotcloud/docker/engine" + "github.com/dotcloud/docker/term" "github.com/dotcloud/docker/utils" "io" "io/ioutil" @@ -507,6 +508,17 @@ func TestAttachDetach(t *testing.T) { <-ch }) + pty, err := container.GetPtyMaster() + if err != nil { + t.Fatal(err) + } + + state, err := term.MakeRaw(pty.Fd()) + if err != nil { + t.Fatal(err) + } + defer term.RestoreTerminal(pty.Fd(), state) + stdin, stdinPipe = io.Pipe() stdout, stdoutPipe = io.Pipe() cli = docker.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, testDaemonProto, testDaemonAddr) From 63d6cbe3e4d6f29c2491b0f1f505ef79b7191d8e Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 09:11:20 -0800 Subject: [PATCH 07/19] Actually test the detach (was not the case before) --- commands.go | 6 ++++++ integration/commands_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/commands.go b/commands.go index db752447f0..24218d284c 100644 --- a/commands.go +++ b/commands.go @@ -2394,6 +2394,12 @@ func (cli *DockerCli) hijack(method, path string, setRawTerminal bool, in io.Rea if stdout != nil { receiveStdout = utils.Go(func() (err error) { + defer func() { + if in != nil { + in.Close() + } + }() + // When TTY is ON, use regular copy if setRawTerminal { _, err = io.Copy(stdout, br) diff --git a/integration/commands_test.go b/integration/commands_test.go index f2b14482b2..75d09facb4 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -542,18 +542,18 @@ func TestAttachDetach(t *testing.T) { }) setTimeout(t, "Escape sequence timeout", 5*time.Second, func() { - stdinPipe.Write([]byte{16, 17}) - if err := stdinPipe.Close(); err != nil { - t.Fatal(err) - } + stdinPipe.Write([]byte{16}) + time.Sleep(100 * time.Millisecond) + stdinPipe.Write([]byte{17}) }) - closeWrap(stdin, stdinPipe, stdout, stdoutPipe) // wait for CmdRun to return setTimeout(t, "Waiting for CmdAttach timed out", 15*time.Second, func() { <-ch }) + closeWrap(stdin, stdinPipe, stdout, stdoutPipe) + time.Sleep(500 * time.Millisecond) if !container.State.IsRunning() { t.Fatal("The detached container should be still running") From aa68656cd3aefe2a64dc259e8d19c72010e4f85b Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 09:52:44 -0800 Subject: [PATCH 08/19] Fix term.RestoreTerminal behavior --- term/term.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/term/term.go b/term/term.go index 8c53a20ca6..50425a8602 100644 --- a/term/term.go +++ b/term/term.go @@ -1,12 +1,17 @@ package term import ( + "errors" "os" "os/signal" "syscall" "unsafe" ) +var ( + ErrInvalidState = errors.New("Invlide terminal state") +) + type State struct { termios Termios } @@ -47,8 +52,14 @@ func IsTerminal(fd uintptr) bool { // Restore restores the terminal connected to the given file descriptor to a // previous state. func RestoreTerminal(fd uintptr, state *State) error { + if state == nil { + return ErrInvalidState + } _, _, err := syscall.Syscall(syscall.SYS_IOCTL, fd, uintptr(setTermios), uintptr(unsafe.Pointer(&state.termios))) - return err + if err != 0 { + return err + } + return nil } func SaveState(fd uintptr) (*State, error) { From c13821ad0bfa596a8bbc06a494500ee39a35e429 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 09:55:15 -0800 Subject: [PATCH 09/19] Make sure the termcaps are restored after hijack --- commands.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/commands.go b/commands.go index 24218d284c..dbf295cf13 100644 --- a/commands.go +++ b/commands.go @@ -2392,10 +2392,23 @@ func (cli *DockerCli) hijack(method, path string, setRawTerminal bool, in io.Rea var receiveStdout chan error + var oldState *term.State + + if in != nil && setRawTerminal && cli.isTerminal && os.Getenv("NORAW") == "" { + oldState, err = term.SetRawTerminal(cli.terminalFd) + if err != nil { + return err + } + defer term.RestoreTerminal(cli.terminalFd, oldState) + } + if stdout != nil { receiveStdout = utils.Go(func() (err error) { defer func() { if in != nil { + if setRawTerminal && cli.isTerminal { + term.RestoreTerminal(cli.terminalFd, oldState) + } in.Close() } }() @@ -2411,14 +2424,6 @@ func (cli *DockerCli) hijack(method, path string, setRawTerminal bool, in io.Rea }) } - if in != nil && setRawTerminal && cli.isTerminal && os.Getenv("NORAW") == "" { - oldState, err := term.SetRawTerminal(cli.terminalFd) - if err != nil { - return err - } - defer term.RestoreTerminal(cli.terminalFd, oldState) - } - sendStdin := utils.Go(func() error { if in != nil { io.Copy(rwc, in) From 697be6aaa009cd2bea5f07ae0b0780703e6565e1 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 09:57:59 -0800 Subject: [PATCH 10/19] Create helper function for tests --- integration/commands_test.go | 66 ++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/integration/commands_test.go b/integration/commands_test.go index 75d09facb4..fa8f25836c 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -32,6 +32,47 @@ func closeWrap(args ...io.Closer) error { return nil } +func setRaw(t *testing.T, c *docker.Container) *term.State { + pty, err := c.GetPtyMaster() + if err != nil { + t.Fatal(err) + } + state, err := term.MakeRaw(pty.Fd()) + if err != nil { + t.Fatal(err) + } + return state +} + +func unsetRaw(t *testing.T, c *docker.Container, state *term.State) { + pty, err := c.GetPtyMaster() + if err != nil { + t.Fatal(err) + } + term.RestoreTerminal(pty.Fd(), state) +} + +func waitContainerStart(t *testing.T, timeout time.Duration) *docker.Container { + var container *docker.Container + + setTimeout(t, "Waiting for the container to be started timed out", timeout, func() { + for { + l := globalRuntime.List() + if len(l) == 1 && l[0].State.IsRunning() { + container = l[0] + break + } + time.Sleep(10 * time.Millisecond) + } + }) + + if container == nil { + t.Fatal("An error occured while waiting for the container to start") + } + + return container +} + func setTimeout(t *testing.T, msg string, d time.Duration, f func()) { c := make(chan bool) @@ -480,18 +521,7 @@ func TestAttachDetach(t *testing.T) { } }() - var container *docker.Container - - setTimeout(t, "Waiting for the container to be started timed out", 10*time.Second, func() { - for { - l := globalRuntime.List() - if len(l) == 1 && l[0].State.IsRunning() { - container = l[0] - break - } - time.Sleep(10 * time.Millisecond) - } - }) + container := waitContainerStart(t, 10*time.Second) setTimeout(t, "Reading container's id timed out", 10*time.Second, func() { buf := make([]byte, 1024) @@ -508,16 +538,8 @@ func TestAttachDetach(t *testing.T) { <-ch }) - pty, err := container.GetPtyMaster() - if err != nil { - t.Fatal(err) - } - - state, err := term.MakeRaw(pty.Fd()) - if err != nil { - t.Fatal(err) - } - defer term.RestoreTerminal(pty.Fd(), state) + state := setRaw(t, container) + defer unsetRaw(t, container, state) stdin, stdinPipe = io.Pipe() stdout, stdoutPipe = io.Pipe() From 2e6a958612d65a0665a9396372fe82706987d085 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 10:03:36 -0800 Subject: [PATCH 11/19] Fix TestAttachDetachTruncatedID (behavior + tty issue) --- integration/commands_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/integration/commands_test.go b/integration/commands_test.go index fa8f25836c..90333a052c 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -601,7 +601,10 @@ func TestAttachDetachTruncatedID(t *testing.T) { } }) - container := globalRuntime.List()[0] + container := waitContainerStart(t, 10*time.Second) + + state := setRaw(t, container) + defer unsetRaw(t, container, state) stdin, stdinPipe = io.Pipe() stdout, stdoutPipe = io.Pipe() @@ -626,17 +629,16 @@ func TestAttachDetachTruncatedID(t *testing.T) { }) setTimeout(t, "Escape sequence timeout", 5*time.Second, func() { - stdinPipe.Write([]byte{16, 17}) - if err := stdinPipe.Close(); err != nil { - t.Fatal(err) - } + stdinPipe.Write([]byte{16}) + time.Sleep(100 * time.Millisecond) + stdinPipe.Write([]byte{17}) }) - closeWrap(stdin, stdinPipe, stdout, stdoutPipe) // wait for CmdRun to return setTimeout(t, "Waiting for CmdAttach timed out", 15*time.Second, func() { <-ch }) + closeWrap(stdin, stdinPipe, stdout, stdoutPipe) time.Sleep(500 * time.Millisecond) if !container.State.IsRunning() { From 2ec1146679598837cd8bab62dc672bcda2a9610c Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 10:17:04 -0800 Subject: [PATCH 12/19] Remove an unit test from integrations test --- integration/commands_test.go | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/integration/commands_test.go b/integration/commands_test.go index 90333a052c..ba9399218a 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -774,25 +774,6 @@ func TestCmdLogs(t *testing.T) { } } -// Expected behaviour: using / as a bind mount source should throw an error -func TestRunErrorBindMountRootSource(t *testing.T) { - - cli := docker.NewDockerCli(nil, nil, ioutil.Discard, testDaemonProto, testDaemonAddr) - defer cleanup(globalEngine, t) - - c := make(chan struct{}) - go func() { - defer close(c) - if err := cli.CmdRun("-v", "/:/tmp", unitTestImageID, "echo 'should fail'"); err == nil { - t.Fatal("should have failed to run when using / as a source for the bind mount") - } - }() - - setTimeout(t, "CmdRun timed out", 5*time.Second, func() { - <-c - }) -} - // Expected behaviour: error out when attempting to bind mount non-existing source paths func TestRunErrorBindNonExistingSource(t *testing.T) { @@ -802,6 +783,7 @@ func TestRunErrorBindNonExistingSource(t *testing.T) { c := make(chan struct{}) go func() { defer close(c) + // This check is made at runtime, can't be "unit tested" if err := cli.CmdRun("-v", "/i/dont/exist:/tmp", unitTestImageID, "echo 'should fail'"); err == nil { t.Fatal("should have failed to run when using /i/dont/exist as a source for the bind mount") } From 86c00be180f1e6831ca426576a55f5106f156448 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 10:17:25 -0800 Subject: [PATCH 13/19] Fix behavior of tty tests --- integration/commands_test.go | 47 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/integration/commands_test.go b/integration/commands_test.go index ba9399218a..50cd230ce9 100644 --- a/integration/commands_test.go +++ b/integration/commands_test.go @@ -337,7 +337,8 @@ func TestRunDisconnect(t *testing.T) { }) } -// Expected behaviour: the process dies when the client disconnects +// Expected behaviour: the process stay alive when the client disconnects +// but the client detaches. func TestRunDisconnectTty(t *testing.T) { stdin, stdinPipe := io.Pipe() @@ -348,29 +349,20 @@ func TestRunDisconnectTty(t *testing.T) { c1 := make(chan struct{}) go func() { + defer close(c1) // We're simulating a disconnect so the return value doesn't matter. What matters is the // fact that CmdRun returns. if err := cli.CmdRun("-i", "-t", unitTestImageID, "/bin/cat"); err != nil { utils.Debugf("Error CmdRun: %s", err) } - - close(c1) }() - setTimeout(t, "Waiting for the container to be started timed out", 10*time.Second, func() { - for { - // Client disconnect after run -i should keep stdin out in TTY mode - l := globalRuntime.List() - if len(l) == 1 && l[0].State.IsRunning() { - break - } - time.Sleep(10 * time.Millisecond) - } - }) + container := waitContainerStart(t, 10*time.Second) + + state := setRaw(t, container) + defer unsetRaw(t, container, state) // Client disconnect after run -i should keep stdin out in TTY mode - container := globalRuntime.List()[0] - setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() { if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) @@ -382,8 +374,12 @@ func TestRunDisconnectTty(t *testing.T) { t.Fatal(err) } + // wait for CmdRun to return + setTimeout(t, "Waiting for CmdRun timed out", 5*time.Second, func() { + <-c1 + }) + // In tty mode, we expect the process to stay alive even after client's stdin closes. - // Do not wait for run to finish // Give some time to monitor to do his thing container.WaitTimeout(500 * time.Millisecond) @@ -473,27 +469,28 @@ func TestRunDetach(t *testing.T) { cli.CmdRun("-i", "-t", unitTestImageID, "cat") }() + container := waitContainerStart(t, 10*time.Second) + + state := setRaw(t, container) + defer unsetRaw(t, container, state) + setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() { if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil { t.Fatal(err) } }) - container := globalRuntime.List()[0] - setTimeout(t, "Escape sequence timeout", 5*time.Second, func() { - stdinPipe.Write([]byte{16, 17}) - if err := stdinPipe.Close(); err != nil { - t.Fatal(err) - } + stdinPipe.Write([]byte{16}) + time.Sleep(100 * time.Millisecond) + stdinPipe.Write([]byte{17}) }) - closeWrap(stdin, stdinPipe, stdout, stdoutPipe) - // wait for CmdRun to return setTimeout(t, "Waiting for CmdRun timed out", 15*time.Second, func() { <-ch }) + closeWrap(stdin, stdinPipe, stdout, stdoutPipe) time.Sleep(500 * time.Millisecond) if !container.State.IsRunning() { @@ -594,6 +591,7 @@ func TestAttachDetachTruncatedID(t *testing.T) { cli := docker.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, testDaemonProto, testDaemonAddr) defer cleanup(globalEngine, t) + // Discard the CmdRun output go stdout.Read(make([]byte, 1024)) setTimeout(t, "Starting container timed out", 2*time.Second, func() { if err := cli.CmdRun("-i", "-t", "-d", unitTestImageID, "cat"); err != nil { @@ -759,6 +757,7 @@ func TestRunAutoRemove(t *testing.T) { } func TestCmdLogs(t *testing.T) { + t.Skip("Test not impemented") cli := docker.NewDockerCli(nil, ioutil.Discard, ioutil.Discard, testDaemonProto, testDaemonAddr) defer cleanup(globalEngine, t) From 34353e782e1cdbd6aae078b3e660875e703d35ff Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 10:32:52 -0800 Subject: [PATCH 14/19] Reduce the timeout for restart/stop --- integration/server_test.go | 4 ++-- term/term.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/integration/server_test.go b/integration/server_test.go index 24e109ab76..494e23fef3 100644 --- a/integration/server_test.go +++ b/integration/server_test.go @@ -183,11 +183,11 @@ func TestCreateStartRestartStopStartKillRm(t *testing.T) { t.Fatal(err) } - if err := srv.ContainerRestart(id, 150); err != nil { + if err := srv.ContainerRestart(id, 15); err != nil { t.Fatal(err) } - if err := srv.ContainerStop(id, 150); err != nil { + if err := srv.ContainerStop(id, 15); err != nil { t.Fatal(err) } diff --git a/term/term.go b/term/term.go index 50425a8602..f7d9930ad0 100644 --- a/term/term.go +++ b/term/term.go @@ -9,7 +9,7 @@ import ( ) var ( - ErrInvalidState = errors.New("Invlide terminal state") + ErrInvalidState = errors.New("Invlid terminal state") ) type State struct { From ab35aef6b5b15c7c2d7aff4315025563b93ee379 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 29 Nov 2013 13:43:37 -0800 Subject: [PATCH 15/19] Add unit test to check bind / server side --- integration/api_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/integration/api_test.go b/integration/api_test.go index bd6280af8b..3d5a2e42e0 100644 --- a/integration/api_test.go +++ b/integration/api_test.go @@ -743,6 +743,43 @@ func TestPostContainersStart(t *testing.T) { containerKill(eng, containerID, t) } +// Expected behaviour: using / as a bind mount source should throw an error +func TestRunErrorBindMountRootSource(t *testing.T) { + eng := NewTestEngine(t) + defer mkRuntimeFromEngine(eng, t).Nuke() + srv := mkServerFromEngine(eng, t) + + containerID := createTestContainer( + eng, + &docker.Config{ + Image: unitTestImageID, + Cmd: []string{"/bin/cat"}, + OpenStdin: true, + }, + t, + ) + + hostConfigJSON, err := json.Marshal(&docker.HostConfig{ + Binds: []string{"/:/tmp"}, + }) + + req, err := http.NewRequest("POST", "/containers/"+containerID+"/start", bytes.NewReader(hostConfigJSON)) + if err != nil { + t.Fatal(err) + } + + req.Header.Set("Content-Type", "application/json") + + r := httptest.NewRecorder() + if err := docker.ServeRequest(srv, docker.APIVERSION, r, req); err != nil { + t.Fatal(err) + } + if r.Code != http.StatusInternalServerError { + containerKill(eng, containerID, t) + t.Fatal("should have failed to run when using / as a source for the bind mount") + } +} + func TestPostContainersStop(t *testing.T) { eng := NewTestEngine(t) defer mkRuntimeFromEngine(eng, t).Nuke() From fe72f15e4ab25cc6e96c76d2da2c379569756843 Mon Sep 17 00:00:00 2001 From: Andrews Medina Date: Fri, 29 Nov 2013 22:20:59 -0200 Subject: [PATCH 16/19] go fmt. result of `gofmt -w -s .` without vendors. --- integration/graph_test.go | 26 +++++++++++++------------- registry/registry.go | 2 +- utils_test.go | 30 +++++++++++++++--------------- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/integration/graph_test.go b/integration/graph_test.go index 118ec17863..eec4c5c7dc 100644 --- a/integration/graph_test.go +++ b/integration/graph_test.go @@ -287,19 +287,19 @@ func assertNImages(graph *docker.Graph, t *testing.T, n int) { } func tempGraph(t *testing.T) (*docker.Graph, graphdriver.Driver) { - tmp, err := ioutil.TempDir("", "docker-graph-") - if err != nil { - t.Fatal(err) - } - driver, err := graphdriver.New(tmp) - if err != nil { - t.Fatal(err) - } - graph, err := docker.NewGraph(tmp, driver) - if err != nil { - t.Fatal(err) - } - return graph, driver + tmp, err := ioutil.TempDir("", "docker-graph-") + if err != nil { + t.Fatal(err) + } + driver, err := graphdriver.New(tmp) + if err != nil { + t.Fatal(err) + } + graph, err := docker.NewGraph(tmp, driver) + if err != nil { + t.Fatal(err) + } + return graph, driver } func nukeGraph(graph *docker.Graph) { diff --git a/registry/registry.go b/registry/registry.go index d3d9f2be54..6aea458e99 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -47,7 +47,7 @@ func pingRegistryEndpoint(endpoint string) error { if err != nil { return err } - defer resp.Body.Close() + defer resp.Body.Close() if resp.Header.Get("X-Docker-Registry-Version") == "" { return errors.New("This does not look like a Registry server (\"X-Docker-Registry-Version\" header not found in the response)") diff --git a/utils_test.go b/utils_test.go index f4fa80a5f1..4b8cfba39f 100644 --- a/utils_test.go +++ b/utils_test.go @@ -1,24 +1,24 @@ package docker import ( - "io" "archive/tar" "bytes" + "io" ) func fakeTar() (io.Reader, error) { - content := []byte("Hello world!\n") - buf := new(bytes.Buffer) - tw := tar.NewWriter(buf) - for _, name := range []string{"/etc/postgres/postgres.conf", "/etc/passwd", "/var/log/postgres/postgres.conf"} { - hdr := new(tar.Header) - hdr.Size = int64(len(content)) - hdr.Name = name - if err := tw.WriteHeader(hdr); err != nil { - return nil, err - } - tw.Write([]byte(content)) - } - tw.Close() - return buf, nil + content := []byte("Hello world!\n") + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + for _, name := range []string{"/etc/postgres/postgres.conf", "/etc/passwd", "/var/log/postgres/postgres.conf"} { + hdr := new(tar.Header) + hdr.Size = int64(len(content)) + hdr.Name = name + if err := tw.WriteHeader(hdr); err != nil { + return nil, err + } + tw.Write([]byte(content)) + } + tw.Close() + return buf, nil } From a4f8a2494b0fb755db52c68cf61ddc8ff52d2965 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Wed, 20 Nov 2013 07:37:03 +0000 Subject: [PATCH 17/19] Engine: integer job status, improved stream API * Jobs return an integer status instead of a string * Status convention mimics unix process execution: 0=success, 1=generic error, 127="no such command" * Stdout and Stderr support multiple thread-safe data receivers and ring buffer filtering --- api.go | 12 ++- engine/engine.go | 10 +- engine/engine_test.go | 7 +- engine/helpers_test.go | 18 +--- engine/job.go | 147 ++++++++++-------------------- engine/streams.go | 166 ++++++++++++++++++++++++++++++++++ integration/api_test.go | 4 + integration/container_test.go | 4 +- integration/runtime_test.go | 2 +- integration/server_test.go | 2 +- integration/utils_test.go | 2 +- server.go | 72 +++++++++------ 12 files changed, 291 insertions(+), 155 deletions(-) create mode 100644 engine/streams.go diff --git a/api.go b/api.go index aadd79e3c8..1708420e12 100644 --- a/api.go +++ b/api.go @@ -1,6 +1,8 @@ package docker import ( + "bufio" + "bytes" "code.google.com/p/go.net/websocket" "encoding/base64" "encoding/json" @@ -565,12 +567,18 @@ func postContainersCreate(srv *Server, version float64, w http.ResponseWriter, r job.SetenvList("Dns", defaultDns) } // Read container ID from the first line of stdout - job.StdoutParseString(&out.ID) + job.Stdout.AddString(&out.ID) // Read warnings from stderr - job.StderrParseLines(&out.Warnings, 0) + warnings := &bytes.Buffer{} + job.Stderr.Add(warnings) if err := job.Run(); err != nil { return err } + // Parse warnings from stderr + scanner := bufio.NewScanner(warnings) + for scanner.Scan() { + out.Warnings = append(out.Warnings, scanner.Text()) + } if job.GetenvInt("Memory") > 0 && !srv.runtime.capabilities.MemoryLimit { log.Println("WARNING: Your kernel does not support memory limit capabilities. Limitation discarded.") out.Warnings = append(out.Warnings, "Your kernel does not support memory limit capabilities. Limitation discarded.") diff --git a/engine/engine.go b/engine/engine.go index edd04fc5c0..5a411e8cc2 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -9,7 +9,7 @@ import ( "strings" ) -type Handler func(*Job) string +type Handler func(*Job) Status var globalHandlers map[string]Handler @@ -99,10 +99,12 @@ func (eng *Engine) Job(name string, args ...string) *Job { Eng: eng, Name: name, Args: args, - Stdin: os.Stdin, - Stdout: os.Stdout, - Stderr: os.Stderr, + Stdin: NewInput(), + Stdout: NewOutput(), + Stderr: NewOutput(), } + job.Stdout.Add(utils.NopWriteCloser(os.Stdout)) + job.Stderr.Add(utils.NopWriteCloser(os.Stderr)) handler, exists := eng.handlers[name] if exists { job.handler = handler diff --git a/engine/engine_test.go b/engine/engine_test.go index fdc0b0ec7f..f877a3e4d5 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -38,8 +38,9 @@ func TestJob(t *testing.T) { t.Fatalf("job1.handler should be empty") } - h := func(j *Job) string { - return j.Name + h := func(j *Job) Status { + j.Printf("%s\n", j.Name) + return 42 } eng.Register("dummy2", h) @@ -49,7 +50,7 @@ func TestJob(t *testing.T) { t.Fatalf("job2.handler shouldn't be nil") } - if job2.handler(job2) != job2.Name { + if job2.handler(job2) != 42 { t.Fatalf("handler dummy2 was not found in job2") } } diff --git a/engine/helpers_test.go b/engine/helpers_test.go index 5b1c0baf60..488529fc7f 100644 --- a/engine/helpers_test.go +++ b/engine/helpers_test.go @@ -1,32 +1,18 @@ package engine import ( - "fmt" "github.com/dotcloud/docker/utils" - "io/ioutil" - "runtime" - "strings" "testing" ) var globalTestID string func newTestEngine(t *testing.T) *Engine { - // Use the caller function name as a prefix. - // This helps trace temp directories back to their test. - pc, _, _, _ := runtime.Caller(1) - callerLongName := runtime.FuncForPC(pc).Name() - parts := strings.Split(callerLongName, ".") - callerShortName := parts[len(parts)-1] - if globalTestID == "" { - globalTestID = utils.RandomString()[:4] - } - prefix := fmt.Sprintf("docker-test%s-%s-", globalTestID, callerShortName) - root, err := ioutil.TempDir("", prefix) + tmp, err := utils.TestDirectory("") if err != nil { t.Fatal(err) } - eng, err := New(root) + eng, err := New(tmp) if err != nil { t.Fatal(err) } diff --git a/engine/job.go b/engine/job.go index 365c94e06b..066a144664 100644 --- a/engine/job.go +++ b/engine/job.go @@ -1,16 +1,13 @@ package engine import ( - "bufio" "bytes" "encoding/json" "fmt" "io" - "io/ioutil" - "os" "strconv" "strings" - "sync" + "time" ) // A job is the fundamental unit of work in the docker engine. @@ -31,126 +28,75 @@ type Job struct { Name string Args []string env []string - Stdin io.Reader - Stdout io.Writer - Stderr io.Writer - handler func(*Job) string - status string + Stdout *Output + Stderr *Output + Stdin *Input + handler Handler + status Status + end time.Time onExit []func() } +type Status int + +const ( + StatusOK Status = 0 + StatusErr Status = 1 + StatusNotFound Status = 127 +) + // Run executes the job and blocks until the job completes. // If the job returns a failure status, an error is returned // which includes the status. func (job *Job) Run() error { - defer func() { - var wg sync.WaitGroup - for _, f := range job.onExit { - wg.Add(1) - go func(f func()) { - f() - wg.Done() - }(f) - } - wg.Wait() - }() - if job.Stdout != nil && job.Stdout != os.Stdout { - job.Stdout = io.MultiWriter(job.Stdout, os.Stdout) - } - if job.Stderr != nil && job.Stderr != os.Stderr { - job.Stderr = io.MultiWriter(job.Stderr, os.Stderr) + // FIXME: make this thread-safe + // FIXME: implement wait + if !job.end.IsZero() { + return fmt.Errorf("%s: job has already completed", job.Name) } + // Log beginning and end of the job job.Eng.Logf("+job %s", job.CallString()) defer func() { job.Eng.Logf("-job %s%s", job.CallString(), job.StatusString()) }() + var errorMessage string + job.Stderr.AddString(&errorMessage) if job.handler == nil { - job.status = "command not found" + job.Errorf("%s: command not found", job.Name) + job.status = 127 } else { job.status = job.handler(job) + job.end = time.Now() } - if job.status != "0" { - return fmt.Errorf("%s: %s", job.Name, job.status) + // Wait for all background tasks to complete + if err := job.Stdout.Close(); err != nil { + return err + } + if err := job.Stderr.Close(); err != nil { + return err + } + if job.status != 0 { + return fmt.Errorf("%s: %s", job.Name, errorMessage) } return nil } -func (job *Job) StdoutParseLines(dst *[]string, limit int) { - job.parseLines(job.StdoutPipe(), dst, limit) -} - -func (job *Job) StderrParseLines(dst *[]string, limit int) { - job.parseLines(job.StderrPipe(), dst, limit) -} - -func (job *Job) parseLines(src io.Reader, dst *[]string, limit int) { - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - scanner := bufio.NewScanner(src) - for scanner.Scan() { - // If the limit is reached, flush the rest of the source and return - if limit > 0 && len(*dst) >= limit { - io.Copy(ioutil.Discard, src) - return - } - line := scanner.Text() - // Append the line (with delimitor removed) - *dst = append(*dst, line) - } - }() - job.onExit = append(job.onExit, wg.Wait) -} - -func (job *Job) StdoutParseString(dst *string) { - lines := make([]string, 0, 1) - job.StdoutParseLines(&lines, 1) - job.onExit = append(job.onExit, func() { - if len(lines) >= 1 { - *dst = lines[0] - } - }) -} - -func (job *Job) StderrParseString(dst *string) { - lines := make([]string, 0, 1) - job.StderrParseLines(&lines, 1) - job.onExit = append(job.onExit, func() { *dst = lines[0] }) -} - -func (job *Job) StdoutPipe() io.ReadCloser { - r, w := io.Pipe() - job.Stdout = w - job.onExit = append(job.onExit, func() { w.Close() }) - return r -} - -func (job *Job) StderrPipe() io.ReadCloser { - r, w := io.Pipe() - job.Stderr = w - job.onExit = append(job.onExit, func() { w.Close() }) - return r -} - func (job *Job) CallString() string { return fmt.Sprintf("%s(%s)", job.Name, strings.Join(job.Args, ", ")) } func (job *Job) StatusString() string { - // FIXME: if a job returns the empty string, it will be printed - // as not having returned. - // (this only affects String which is a convenience function). - if job.status != "" { - var okerr string - if job.status == "0" { - okerr = "OK" - } else { - okerr = "ERR" - } - return fmt.Sprintf(" = %s (%s)", okerr, job.status) + // If the job hasn't completed, status string is empty + if job.end.IsZero() { + return "" } - return "" + var okerr string + if job.status == StatusOK { + okerr = "OK" + } else { + okerr = "ERR" + } + return fmt.Sprintf(" = %s (%d)", okerr, job.status) } // String returns a human-readable description of `job` @@ -338,5 +284,8 @@ func (job *Job) Printf(format string, args ...interface{}) (n int, err error) { func (job *Job) Errorf(format string, args ...interface{}) (n int, err error) { return fmt.Fprintf(job.Stderr, format, args...) - +} + +func (job *Job) Error(err error) (int, error) { + return fmt.Fprintf(job.Stderr, "%s", err) } diff --git a/engine/streams.go b/engine/streams.go new file mode 100644 index 0000000000..697407f1f4 --- /dev/null +++ b/engine/streams.go @@ -0,0 +1,166 @@ +package engine + +import ( + "bufio" + "container/ring" + "fmt" + "io" + "sync" +) + +type Output struct { + sync.Mutex + dests []io.Writer + tasks sync.WaitGroup +} + +// NewOutput returns a new Output object with no destinations attached. +// Writing to an empty Output will cause the written data to be discarded. +func NewOutput() *Output { + return &Output{} +} + +// Add attaches a new destination to the Output. Any data subsequently written +// to the output will be written to the new destination in addition to all the others. +// This method is thread-safe. +// FIXME: Add cannot fail +func (o *Output) Add(dst io.Writer) error { + o.Mutex.Lock() + defer o.Mutex.Unlock() + o.dests = append(o.dests, dst) + return nil +} + +// AddPipe creates an in-memory pipe with io.Pipe(), adds its writing end as a destination, +// and returns its reading end for consumption by the caller. +// This is a rough equivalent similar to Cmd.StdoutPipe() in the standard os/exec package. +// This method is thread-safe. +func (o *Output) AddPipe() (io.Reader, error) { + r, w := io.Pipe() + o.Add(w) + return r, nil +} + +// AddTail starts a new goroutine which will read all subsequent data written to the output, +// line by line, and append the last `n` lines to `dst`. +func (o *Output) AddTail(dst *[]string, n int) error { + src, err := o.AddPipe() + if err != nil { + return err + } + o.tasks.Add(1) + go func() { + defer o.tasks.Done() + Tail(src, n, dst) + }() + return nil +} + +// AddString starts a new goroutine which will read all subsequent data written to the output, +// line by line, and store the last line into `dst`. +func (o *Output) AddString(dst *string) error { + src, err := o.AddPipe() + if err != nil { + return err + } + o.tasks.Add(1) + go func() { + defer o.tasks.Done() + lines := make([]string, 0, 1) + Tail(src, 1, &lines) + if len(lines) == 0 { + *dst = "" + } else { + *dst = lines[0] + } + }() + return nil +} + +// Write writes the same data to all registered destinations. +// This method is thread-safe. +func (o *Output) Write(p []byte) (n int, err error) { + o.Mutex.Lock() + defer o.Mutex.Unlock() + var firstErr error + for _, dst := range o.dests { + _, err := dst.Write(p) + if err != nil && firstErr == nil { + firstErr = err + } + } + return len(p), err +} + +// Close unregisters all destinations and waits for all background +// AddTail and AddString tasks to complete. +// The Close method of each destination is called if it exists. +func (o *Output) Close() error { + o.Mutex.Lock() + defer o.Mutex.Unlock() + var firstErr error + for _, dst := range o.dests { + if closer, ok := dst.(io.WriteCloser); ok { + err := closer.Close() + if err != nil && firstErr == nil { + firstErr = err + } + } + } + o.tasks.Wait() + return firstErr +} + +type Input struct { + src io.Reader + sync.Mutex +} + +// NewInput returns a new Input object with no source attached. +// Reading to an empty Input will return io.EOF. +func NewInput() *Input { + return &Input{} +} + +// Read reads from the input in a thread-safe way. +func (i *Input) Read(p []byte) (n int, err error) { + i.Mutex.Lock() + defer i.Mutex.Unlock() + if i.src == nil { + return 0, io.EOF + } + return i.src.Read(p) +} + +// Add attaches a new source to the input. +// Add can only be called once per input. Subsequent calls will +// return an error. +func (i *Input) Add(src io.Reader) error { + i.Mutex.Lock() + defer i.Mutex.Unlock() + if i.src != nil { + return fmt.Errorf("Maximum number of sources reached: 1") + } + i.src = src + return nil +} + +// Tail reads from `src` line per line, and returns the last `n` lines as an array. +// A ring buffer is used to only store `n` lines at any time. +func Tail(src io.Reader, n int, dst *[]string) { + scanner := bufio.NewScanner(src) + r := ring.New(n) + for scanner.Scan() { + if n == 0 { + continue + } + r.Value = scanner.Text() + r = r.Next() + } + r.Do(func(v interface{}) { + if v == nil { + return + } + *dst = append(*dst, v.(string)) + }) +} diff --git a/integration/api_test.go b/integration/api_test.go index 3d5a2e42e0..896831c3d0 100644 --- a/integration/api_test.go +++ b/integration/api_test.go @@ -304,6 +304,10 @@ func TestGetContainersJSON(t *testing.T) { Cmd: []string{"echo", "test"}, }, t) + if containerID == "" { + t.Fatalf("Received empty container ID") + } + req, err := http.NewRequest("GET", "/containers/json?all=1", nil) if err != nil { t.Fatal(err) diff --git a/integration/container_test.go b/integration/container_test.go index 05eb48728c..6cd72c8608 100644 --- a/integration/container_test.go +++ b/integration/container_test.go @@ -499,7 +499,7 @@ func TestCreateVolume(t *testing.T) { t.Fatal(err) } var id string - jobCreate.StdoutParseString(&id) + jobCreate.Stdout.AddString(&id) if err := jobCreate.Run(); err != nil { t.Fatal(err) } @@ -1502,7 +1502,7 @@ func TestOnlyLoopbackExistsWhenUsingDisableNetworkOption(t *testing.T) { t.Fatal(err) } var id string - jobCreate.StdoutParseString(&id) + jobCreate.Stdout.AddString(&id) if err := jobCreate.Run(); err != nil { t.Fatal(err) } diff --git a/integration/runtime_test.go b/integration/runtime_test.go index 1ab6d0a080..7074a14ce9 100644 --- a/integration/runtime_test.go +++ b/integration/runtime_test.go @@ -390,7 +390,7 @@ func startEchoServerContainer(t *testing.T, proto string) (*docker.Runtime, *doc jobCreate.SetenvList("Cmd", []string{"sh", "-c", cmd}) jobCreate.SetenvList("PortSpecs", []string{fmt.Sprintf("%s/%s", strPort, proto)}) jobCreate.SetenvJson("ExposedPorts", ep) - jobCreate.StdoutParseString(&id) + jobCreate.Stdout.AddString(&id) if err := jobCreate.Run(); err != nil { t.Fatal(err) } diff --git a/integration/server_test.go b/integration/server_test.go index 494e23fef3..3e85effe8f 100644 --- a/integration/server_test.go +++ b/integration/server_test.go @@ -224,7 +224,7 @@ func TestRunWithTooLowMemoryLimit(t *testing.T) { job.Setenv("CpuShares", "1000") job.SetenvList("Cmd", []string{"/bin/cat"}) var id string - job.StdoutParseString(&id) + job.Stdout.AddString(&id) if err := job.Run(); err == nil { t.Errorf("Memory limit is smaller than the allowed limit. Container creation should've failed!") } diff --git a/integration/utils_test.go b/integration/utils_test.go index 1f47c45382..2feaf25396 100644 --- a/integration/utils_test.go +++ b/integration/utils_test.go @@ -46,7 +46,7 @@ func createNamedTestContainer(eng *engine.Engine, config *docker.Config, f utils if err := job.ImportEnv(config); err != nil { f.Fatal(err) } - job.StdoutParseString(&shortId) + job.Stdout.AddString(&shortId) if err := job.Run(); err != nil { f.Fatal(err) } diff --git a/server.go b/server.go index 725b964a70..ae6ccd4f40 100644 --- a/server.go +++ b/server.go @@ -39,15 +39,18 @@ func init() { // jobInitApi runs the remote api server `srv` as a daemon, // Only one api server can run at the same time - this is enforced by a pidfile. // The signals SIGINT and SIGTERM are intercepted for cleanup. -func jobInitApi(job *engine.Job) string { +func jobInitApi(job *engine.Job) engine.Status { job.Logf("Creating server") + // FIXME: ImportEnv deprecates ConfigFromJob srv, err := NewServer(job.Eng, ConfigFromJob(job)) if err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } if srv.runtime.config.Pidfile != "" { job.Logf("Creating pidfile") if err := utils.CreatePidFile(srv.runtime.config.Pidfile); err != nil { + // FIXME: do we need fatal here instead of returning a job error? log.Fatal(err) } } @@ -68,18 +71,21 @@ func jobInitApi(job *engine.Job) string { job.Eng.Hack_SetGlobalVar("httpapi.bridgeIP", srv.runtime.networkManager.bridgeNetwork.IP) } if err := job.Eng.Register("create", srv.ContainerCreate); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } if err := job.Eng.Register("start", srv.ContainerStart); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } if err := job.Eng.Register("serveapi", srv.ListenAndServe); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } - return "0" + return engine.StatusOK } -func (srv *Server) ListenAndServe(job *engine.Job) string { +func (srv *Server) ListenAndServe(job *engine.Job) engine.Status { protoAddrs := job.Args chErrors := make(chan error, len(protoAddrs)) for _, protoAddr := range protoAddrs { @@ -94,7 +100,8 @@ func (srv *Server) ListenAndServe(job *engine.Job) string { log.Println("/!\\ DON'T BIND ON ANOTHER IP ADDRESS THAN 127.0.0.1 IF YOU DON'T KNOW WHAT YOU'RE DOING /!\\") } default: - return "Invalid protocol format." + job.Errorf("Invalid protocol format.") + return engine.StatusErr } go func() { // FIXME: merge Server.ListenAndServe with ListenAndServe @@ -104,10 +111,11 @@ func (srv *Server) ListenAndServe(job *engine.Job) string { for i := 0; i < len(protoAddrs); i += 1 { err := <-chErrors if err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } } - return "0" + return engine.StatusOK } func (srv *Server) DockerVersion() APIVersion { @@ -1260,19 +1268,22 @@ func (srv *Server) ImageImport(src, repo, tag string, in io.Reader, out io.Write return nil } -func (srv *Server) ContainerCreate(job *engine.Job) string { +func (srv *Server) ContainerCreate(job *engine.Job) engine.Status { var name string if len(job.Args) == 1 { name = job.Args[0] } else if len(job.Args) > 1 { - return fmt.Sprintf("Usage: %s ", job.Name) + job.Printf("Usage: %s", job.Name) + return engine.StatusErr } var config Config if err := job.ExportEnv(&config); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } if config.Memory != 0 && config.Memory < 524288 { - return "Minimum memory limit allowed is 512k" + job.Errorf("Minimum memory limit allowed is 512k") + return engine.StatusErr } if config.Memory > 0 && !srv.runtime.capabilities.MemoryLimit { config.Memory = 0 @@ -1287,9 +1298,11 @@ func (srv *Server) ContainerCreate(job *engine.Job) string { if tag == "" { tag = DEFAULTTAG } - return fmt.Sprintf("No such image: %s (tag: %s)", config.Image, tag) + job.Errorf("No such image: %s (tag: %s)", config.Image, tag) + return engine.StatusErr } - return err.Error() + job.Error(err) + return engine.StatusErr } srv.LogEvent("create", container.ID, srv.runtime.repositories.ImageName(container.Image)) // FIXME: this is necessary because runtime.Create might return a nil container @@ -1301,7 +1314,7 @@ func (srv *Server) ContainerCreate(job *engine.Job) string { for _, warning := range buildWarnings { job.Errorf("%s\n", warning) } - return "0" + return engine.StatusOK } func (srv *Server) ContainerRestart(name string, t int) error { @@ -1619,22 +1632,25 @@ func (srv *Server) RegisterLinks(name string, hostConfig *HostConfig) error { return nil } -func (srv *Server) ContainerStart(job *engine.Job) string { +func (srv *Server) ContainerStart(job *engine.Job) engine.Status { if len(job.Args) < 1 { - return fmt.Sprintf("Usage: %s container_id", job.Name) + job.Errorf("Usage: %s container_id", job.Name) + return engine.StatusErr } name := job.Args[0] runtime := srv.runtime container := runtime.Get(name) if container == nil { - return fmt.Sprintf("No such container: %s", name) + job.Errorf("No such container: %s", name) + return engine.StatusErr } // If no environment was set, then no hostconfig was passed. if len(job.Environ()) > 0 { var hostConfig HostConfig if err := job.ExportEnv(&hostConfig); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } // Validate the HostConfig binds. Make sure that: // 1) the source of a bind mount isn't / @@ -1647,29 +1663,33 @@ func (srv *Server) ContainerStart(job *engine.Job) string { // refuse to bind mount "/" to the container if source == "/" { - return fmt.Sprintf("Invalid bind mount '%s' : source can't be '/'", bind) + job.Errorf("Invalid bind mount '%s' : source can't be '/'", bind) + return engine.StatusErr } // ensure the source exists on the host _, err := os.Stat(source) if err != nil && os.IsNotExist(err) { - return fmt.Sprintf("Invalid bind mount '%s' : source doesn't exist", bind) + job.Errorf("Invalid bind mount '%s' : source doesn't exist", bind) + return engine.StatusErr } } // Register any links from the host config before starting the container // FIXME: we could just pass the container here, no need to lookup by name again. if err := srv.RegisterLinks(name, &hostConfig); err != nil { - return err.Error() + job.Error(err) + return engine.StatusErr } container.hostConfig = &hostConfig container.ToDisk() } if err := container.Start(); err != nil { - return fmt.Sprintf("Cannot start container %s: %s", name, err) + job.Errorf("Cannot start container %s: %s", name, err) + return engine.StatusErr } srv.LogEvent("start", container.ID, runtime.repositories.ImageName(container.Image)) - return "0" + return engine.StatusOK } func (srv *Server) ContainerStop(name string, t int) error { From 3553a803e3a474207296f5542182ecbc569ca1d8 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Wed, 20 Nov 2013 07:38:59 +0000 Subject: [PATCH 18/19] Engine: better testing of streams and of basic engine primitives. Coverage=81.2% --- engine/engine_test.go | 47 ++++++++ engine/job_test.go | 80 +++++++++++++ engine/streams_test.go | 252 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 engine/job_test.go create mode 100644 engine/streams_test.go diff --git a/engine/engine_test.go b/engine/engine_test.go index f877a3e4d5..793867f50a 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -1,6 +1,9 @@ package engine import ( + "io/ioutil" + "os" + "path" "testing" ) @@ -54,3 +57,47 @@ func TestJob(t *testing.T) { t.Fatalf("handler dummy2 was not found in job2") } } + +func TestEngineRoot(t *testing.T) { + tmp, err := ioutil.TempDir("", "docker-test-TestEngineCreateDir") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmp) + dir := path.Join(tmp, "dir") + eng, err := New(dir) + if err != nil { + t.Fatal(err) + } + if st, err := os.Stat(dir); err != nil { + t.Fatal(err) + } else if !st.IsDir() { + t.Fatalf("engine.New() created something other than a directory at %s", dir) + } + if r := eng.Root(); r != dir { + t.Fatalf("Expected: %v\nReceived: %v", dir, r) + } +} + +func TestEngineString(t *testing.T) { + eng1 := newTestEngine(t) + defer os.RemoveAll(eng1.Root()) + eng2 := newTestEngine(t) + defer os.RemoveAll(eng2.Root()) + s1 := eng1.String() + s2 := eng2.String() + if eng1 == eng2 { + t.Fatalf("Different engines should have different names (%v == %v)", s1, s2) + } +} + +func TestEngineLogf(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + input := "Test log line" + if n, err := eng.Logf("%s\n", input); err != nil { + t.Fatal(err) + } else if n < len(input) { + t.Fatalf("Test: Logf() should print at least as much as the input\ninput=%d\nprinted=%d", len(input), n) + } +} diff --git a/engine/job_test.go b/engine/job_test.go new file mode 100644 index 0000000000..50d882c44b --- /dev/null +++ b/engine/job_test.go @@ -0,0 +1,80 @@ +package engine + +import ( + "os" + "testing" +) + +func TestJobStatusOK(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + eng.Register("return_ok", func(job *Job) Status { return StatusOK }) + err := eng.Job("return_ok").Run() + if err != nil { + t.Fatalf("Expected: err=%v\nReceived: err=%v", nil, err) + } +} + +func TestJobStatusErr(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + eng.Register("return_err", func(job *Job) Status { return StatusErr }) + err := eng.Job("return_err").Run() + if err == nil { + t.Fatalf("When a job returns StatusErr, Run() should return an error") + } +} + +func TestJobStatusNotFound(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + eng.Register("return_not_found", func(job *Job) Status { return StatusNotFound }) + err := eng.Job("return_not_found").Run() + if err == nil { + t.Fatalf("When a job returns StatusNotFound, Run() should return an error") + } +} + +func TestJobStdoutString(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + // FIXME: test multiple combinations of output and status + eng.Register("say_something_in_stdout", func(job *Job) Status { + job.Printf("Hello world\n") + return StatusOK + }) + + job := eng.Job("say_something_in_stdout") + var output string + if err := job.Stdout.AddString(&output); err != nil { + t.Fatal(err) + } + if err := job.Run(); err != nil { + t.Fatal(err) + } + if expectedOutput := "Hello world"; output != expectedOutput { + t.Fatalf("Stdout last line:\nExpected: %v\nReceived: %v", expectedOutput, output) + } +} + +func TestJobStderrString(t *testing.T) { + eng := newTestEngine(t) + defer os.RemoveAll(eng.Root()) + // FIXME: test multiple combinations of output and status + eng.Register("say_something_in_stderr", func(job *Job) Status { + job.Errorf("Warning, something might happen\nHere it comes!\nOh no...\nSomething happened\n") + return StatusOK + }) + + job := eng.Job("say_something_in_stderr") + var output string + if err := job.Stderr.AddString(&output); err != nil { + t.Fatal(err) + } + if err := job.Run(); err != nil { + t.Fatal(err) + } + if expectedOutput := "Something happened"; output != expectedOutput { + t.Fatalf("Stderr last line:\nExpected: %v\nReceived: %v", expectedOutput, output) + } +} diff --git a/engine/streams_test.go b/engine/streams_test.go new file mode 100644 index 0000000000..d7faf229af --- /dev/null +++ b/engine/streams_test.go @@ -0,0 +1,252 @@ +package engine + +import ( + "bufio" + "bytes" + "fmt" + "io/ioutil" + "strings" + "testing" +) + +func TestOutputAddString(t *testing.T) { + var testInputs = [][2]string{ + { + "hello, world!", + "hello, world!", + }, + + { + "One\nTwo\nThree", + "Three", + }, + + { + "", + "", + }, + + { + "A line\nThen another nl-terminated line\n", + "Then another nl-terminated line", + }, + + { + "A line followed by an empty line\n\n", + "", + }, + } + for _, testData := range testInputs { + input := testData[0] + expectedOutput := testData[1] + o := NewOutput() + var output string + if err := o.AddString(&output); err != nil { + t.Error(err) + } + if n, err := o.Write([]byte(input)); err != nil { + t.Error(err) + } else if n != len(input) { + t.Errorf("Expected %d, got %d", len(input), n) + } + o.Close() + if output != expectedOutput { + t.Errorf("Last line is not stored as return string.\nInput: '%s'\nExpected: '%s'\nGot: '%s'", input, expectedOutput, output) + } + } +} + +type sentinelWriteCloser struct { + calledWrite bool + calledClose bool +} + +func (w *sentinelWriteCloser) Write(p []byte) (int, error) { + w.calledWrite = true + return len(p), nil +} + +func (w *sentinelWriteCloser) Close() error { + w.calledClose = true + return nil +} + +func TestOutputAddClose(t *testing.T) { + o := NewOutput() + var s sentinelWriteCloser + if err := o.Add(&s); err != nil { + t.Fatal(err) + } + if err := o.Close(); err != nil { + t.Fatal(err) + } + // Write data after the output is closed. + // Write should succeed, but no destination should receive it. + if _, err := o.Write([]byte("foo bar")); err != nil { + t.Fatal(err) + } + if !s.calledClose { + t.Fatal("Output.Close() didn't close the destination") + } +} + +func TestOutputAddPipe(t *testing.T) { + var testInputs = []string{ + "hello, world!", + "One\nTwo\nThree", + "", + "A line\nThen another nl-terminated line\n", + "A line followed by an empty line\n\n", + } + for _, input := range testInputs { + expectedOutput := input + o := NewOutput() + r, err := o.AddPipe() + if err != nil { + t.Fatal(err) + } + go func(o *Output) { + if n, err := o.Write([]byte(input)); err != nil { + t.Error(err) + } else if n != len(input) { + t.Errorf("Expected %d, got %d", len(input), n) + } + if err := o.Close(); err != nil { + t.Error(err) + } + }(o) + output, err := ioutil.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if string(output) != expectedOutput { + t.Errorf("Last line is not stored as return string.\nExpected: '%s'\nGot: '%s'", expectedOutput, output) + } + } +} + +func TestTail(t *testing.T) { + var tests = make(map[string][][]string) + tests["hello, world!"] = [][]string{ + {}, + {"hello, world!"}, + {"hello, world!"}, + {"hello, world!"}, + } + tests["One\nTwo\nThree"] = [][]string{ + {}, + {"Three"}, + {"Two", "Three"}, + {"One", "Two", "Three"}, + } + for input, outputs := range tests { + for n, expectedOutput := range outputs { + var output []string + Tail(strings.NewReader(input), n, &output) + if fmt.Sprintf("%v", output) != fmt.Sprintf("%v", expectedOutput) { + t.Errorf("Tail n=%d returned wrong result.\nExpected: '%s'\nGot : '%s'", expectedOutput, output) + } + } + } +} + +func TestOutputAddTail(t *testing.T) { + var tests = make(map[string][][]string) + tests["hello, world!"] = [][]string{ + {}, + {"hello, world!"}, + {"hello, world!"}, + {"hello, world!"}, + } + tests["One\nTwo\nThree"] = [][]string{ + {}, + {"Three"}, + {"Two", "Three"}, + {"One", "Two", "Three"}, + } + for input, outputs := range tests { + for n, expectedOutput := range outputs { + o := NewOutput() + var output []string + if err := o.AddTail(&output, n); err != nil { + t.Error(err) + } + if n, err := o.Write([]byte(input)); err != nil { + t.Error(err) + } else if n != len(input) { + t.Errorf("Expected %d, got %d", len(input), n) + } + o.Close() + if fmt.Sprintf("%v", output) != fmt.Sprintf("%v", expectedOutput) { + t.Errorf("Tail(%d) returned wrong result.\nExpected: %v\nGot: %v", n, expectedOutput, output) + } + } + } +} + +func lastLine(txt string) string { + scanner := bufio.NewScanner(strings.NewReader(txt)) + var lastLine string + for scanner.Scan() { + lastLine = scanner.Text() + } + return lastLine +} + +func TestOutputAdd(t *testing.T) { + o := NewOutput() + b := &bytes.Buffer{} + o.Add(b) + input := "hello, world!" + if n, err := o.Write([]byte(input)); err != nil { + t.Fatal(err) + } else if n != len(input) { + t.Fatalf("Expected %d, got %d", len(input), n) + } + if output := b.String(); output != input { + t.Fatal("Received wrong data from Add.\nExpected: '%s'\nGot: '%s'", input, output) + } +} + +func TestInputAddEmpty(t *testing.T) { + i := NewInput() + var b bytes.Buffer + if err := i.Add(&b); err != nil { + t.Fatal(err) + } + data, err := ioutil.ReadAll(i) + if err != nil { + t.Fatal(err) + } + if len(data) > 0 { + t.Fatalf("Read from empty input shoul yield no data") + } +} + +func TestInputAddTwo(t *testing.T) { + i := NewInput() + var b1 bytes.Buffer + // First add should succeed + if err := i.Add(&b1); err != nil { + t.Fatal(err) + } + var b2 bytes.Buffer + // Second add should fail + if err := i.Add(&b2); err == nil { + t.Fatalf("Adding a second source should return an error") + } +} + +func TestInputAddNotEmpty(t *testing.T) { + i := NewInput() + b := bytes.NewBufferString("hello world\nabc") + expectedResult := b.String() + i.Add(b) + result, err := ioutil.ReadAll(i) + if err != nil { + t.Fatal(err) + } + if string(result) != expectedResult { + t.Fatalf("Expected: %v\nReceived: %v", expectedResult, result) + } +} From 35d54c665575d48954db9422702c0324f00ebc62 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Fri, 22 Nov 2013 02:28:56 +0000 Subject: [PATCH 19/19] Fix a bug in Output.Write, and improve testing coverage of error cases. --- engine/streams.go | 2 +- engine/streams_test.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/engine/streams.go b/engine/streams.go index 697407f1f4..ff7049074a 100644 --- a/engine/streams.go +++ b/engine/streams.go @@ -89,7 +89,7 @@ func (o *Output) Write(p []byte) (n int, err error) { firstErr = err } } - return len(p), err + return len(p), firstErr } // Close unregisters all destinations and waits for all background diff --git a/engine/streams_test.go b/engine/streams_test.go index d7faf229af..108c9850a5 100644 --- a/engine/streams_test.go +++ b/engine/streams_test.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "fmt" + "io" "io/ioutil" "strings" "testing" @@ -208,6 +209,27 @@ func TestOutputAdd(t *testing.T) { } } +func TestOutputWriteError(t *testing.T) { + o := NewOutput() + buf := &bytes.Buffer{} + o.Add(buf) + r, w := io.Pipe() + input := "Hello there" + expectedErr := fmt.Errorf("This is an error") + r.CloseWithError(expectedErr) + o.Add(w) + n, err := o.Write([]byte(input)) + if err != expectedErr { + t.Fatalf("Output.Write() should return the first error encountered, if any") + } + if buf.String() != input { + t.Fatalf("Output.Write() should attempt write on all destinations, even after encountering an error") + } + if n != len(input) { + t.Fatalf("Output.Write() should return the size of the input if it successfully writes to at least one destination") + } +} + func TestInputAddEmpty(t *testing.T) { i := NewInput() var b bytes.Buffer