From ca9544522ebc3e622b332a4cd9a3d160931814d6 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 16 Jun 2014 15:30:42 +0200 Subject: [PATCH 1/7] Report child error better (and later) We use a unix domain socketpair instead of a pipe for the sync pipe, which allows us to use two-way shutdown. After sending the context we shut down the write side which lets the child know it finished reading. We then block on a read in the parent for the child closing the file (ensuring we close our version of it too) to sync for when the child is finished initializing. If the read is non-empty we assume this is an error report and fail with an error. Otherwise we continue as before. This also means we're now calling back the start callback later, meaning at that point its more likely to have succeeded, as well as having consumed all the container resources (like volume mounts, making it safe to e.g. unmount them when the start callback is called). Docker-DCO-1.1-Signed-off-by: Alexander Larsson (github: alexlarsson) --- namespaces/exec.go | 8 +++++++- namespaces/init.go | 10 +++++++--- namespaces/sync_pipe.go | 31 ++++++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/namespaces/exec.go b/namespaces/exec.go index c4b724a78..e32da4b86 100644 --- a/namespaces/exec.go +++ b/namespaces/exec.go @@ -32,6 +32,7 @@ func Exec(container *libcontainer.Config, term Terminal, rootfs, dataPath string if err != nil { return -1, err } + defer syncPipe.Close() if container.Tty { master, console, err = system.CreateMasterAndConsole() @@ -52,6 +53,9 @@ func Exec(container *libcontainer.Config, term Terminal, rootfs, dataPath string return -1, err } + // Now we passed the pipe to the child, close our side + syncPipe.CloseChild() + started, err := system.GetProcessStartTime(command.Process.Pid) if err != nil { return -1, err @@ -90,7 +94,9 @@ func Exec(container *libcontainer.Config, term Terminal, rootfs, dataPath string defer libcontainer.DeleteState(dataPath) // Sync with child - syncPipe.Close() + if err := syncPipe.BlockOnChild(); err != nil { + return -1, err + } if startCallback != nil { startCallback() diff --git a/namespaces/init.go b/namespaces/init.go index 6c4745142..4a6ce6e3a 100644 --- a/namespaces/init.go +++ b/namespaces/init.go @@ -27,7 +27,13 @@ import ( // Move this to libcontainer package. // Init is the init process that first runs inside a new namespace to setup mounts, users, networking, // and other options required for the new container. -func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, syncPipe *SyncPipe, args []string) error { +func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, syncPipe *SyncPipe, args []string) (err error) { + defer func() { + if err != nil { + syncPipe.ReportError(err) + } + }() + rootfs, err := utils.ResolveRootfs(uncleanRootfs) if err != nil { return err @@ -42,10 +48,8 @@ func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, syn // We always read this as it is a way to sync with the parent as well networkState, err := syncPipe.ReadFromParent() if err != nil { - syncPipe.Close() return err } - syncPipe.Close() if consolePath != "" { if err := console.OpenAndDup(consolePath); err != nil { diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index 57851c5c6..ae6fefc30 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -5,6 +5,7 @@ import ( "fmt" "io/ioutil" "os" + "syscall" "github.com/docker/libcontainer/network" ) @@ -18,10 +19,14 @@ type SyncPipe struct { func NewSyncPipe() (s *SyncPipe, err error) { s = &SyncPipe{} - s.child, s.parent, err = os.Pipe() + + fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0) if err != nil { return nil, err } + s.child = os.NewFile(uintptr(fds[0]), "child syncpipe") + s.parent = os.NewFile(uintptr(fds[1]), "parent syncpipe") + return s, nil } @@ -51,6 +56,18 @@ func (s *SyncPipe) SendToChild(networkState *network.NetworkState) error { return err } s.parent.Write(data) + syscall.Shutdown(int(s.parent.Fd()), syscall.SHUT_WR) + return nil +} + +func (s *SyncPipe) BlockOnChild() error { + data, err := ioutil.ReadAll(s.parent) + if err != nil { + return nil + } + if len(data) > 0 { + return fmt.Errorf("Child error: %s", string(data)) + } return nil } @@ -69,6 +86,11 @@ func (s *SyncPipe) ReadFromParent() (*network.NetworkState, error) { } +func (s *SyncPipe) ReportError(err error) { + s.child.Write([]byte(err.Error())) + s.CloseChild() +} + func (s *SyncPipe) Close() error { if s.parent != nil { s.parent.Close() @@ -78,3 +100,10 @@ func (s *SyncPipe) Close() error { } return nil } + +func (s *SyncPipe) CloseChild() { + if s.child != nil { + s.child.Close() + s.child = nil + } +} From eb9a561b18279931c320e2bb330779c38591c59c Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 11:38:53 -0700 Subject: [PATCH 2/7] Address review comments Ensure that the command is killed if we receive an error from the child Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/exec.go | 4 +++- namespaces/init.go | 2 +- namespaces/sync_pipe.go | 6 +++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/namespaces/exec.go b/namespaces/exec.go index e32da4b86..0aa2bb9c7 100644 --- a/namespaces/exec.go +++ b/namespaces/exec.go @@ -94,7 +94,9 @@ func Exec(container *libcontainer.Config, term Terminal, rootfs, dataPath string defer libcontainer.DeleteState(dataPath) // Sync with child - if err := syncPipe.BlockOnChild(); err != nil { + if err := syncPipe.ReadFromChild(); err != nil { + command.Process.Kill() + command.Wait() return -1, err } diff --git a/namespaces/init.go b/namespaces/init.go index 4a6ce6e3a..53d2611b8 100644 --- a/namespaces/init.go +++ b/namespaces/init.go @@ -30,7 +30,7 @@ import ( func Init(container *libcontainer.Config, uncleanRootfs, consolePath string, syncPipe *SyncPipe, args []string) (err error) { defer func() { if err != nil { - syncPipe.ReportError(err) + syncPipe.ReportChildError(err) } }() diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index ae6fefc30..2160243e4 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -60,10 +60,10 @@ func (s *SyncPipe) SendToChild(networkState *network.NetworkState) error { return nil } -func (s *SyncPipe) BlockOnChild() error { +func (s *SyncPipe) ReadFromChild() error { data, err := ioutil.ReadAll(s.parent) if err != nil { - return nil + return err } if len(data) > 0 { return fmt.Errorf("Child error: %s", string(data)) @@ -86,7 +86,7 @@ func (s *SyncPipe) ReadFromParent() (*network.NetworkState, error) { } -func (s *SyncPipe) ReportError(err error) { +func (s *SyncPipe) ReportChildError(err error) { s.child.Write([]byte(err.Error())) s.CloseChild() } From e7916505a37182ba14bda2f4dc4db1185ea2b4ce Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 11:50:22 -0700 Subject: [PATCH 3/7] Add unit test for sync pipe Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/sync_pipe.go | 10 ++++-- namespaces/sync_pipe_test.go | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 namespaces/sync_pipe_test.go diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index 2160243e4..8a7c62215 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -24,6 +24,7 @@ func NewSyncPipe() (s *SyncPipe, err error) { if err != nil { return nil, err } + s.child = os.NewFile(uintptr(fds[0]), "child syncpipe") s.parent = os.NewFile(uintptr(fds[1]), "parent syncpipe") @@ -32,6 +33,7 @@ func NewSyncPipe() (s *SyncPipe, err error) { func NewSyncPipeFromFd(parendFd, childFd uintptr) (*SyncPipe, error) { s := &SyncPipe{} + if parendFd > 0 { s.parent = os.NewFile(parendFd, "parendPipe") } else if childFd > 0 { @@ -39,6 +41,7 @@ func NewSyncPipeFromFd(parendFd, childFd uintptr) (*SyncPipe, error) { } else { return nil, fmt.Errorf("no valid sync pipe fd specified") } + return s, nil } @@ -65,9 +68,11 @@ func (s *SyncPipe) ReadFromChild() error { if err != nil { return err } + if len(data) > 0 { - return fmt.Errorf("Child error: %s", string(data)) + return fmt.Errorf("%s", data) } + return nil } @@ -83,7 +88,6 @@ func (s *SyncPipe) ReadFromParent() (*network.NetworkState, error) { } } return networkState, nil - } func (s *SyncPipe) ReportChildError(err error) { @@ -95,9 +99,11 @@ func (s *SyncPipe) Close() error { if s.parent != nil { s.parent.Close() } + if s.child != nil { s.child.Close() } + return nil } diff --git a/namespaces/sync_pipe_test.go b/namespaces/sync_pipe_test.go new file mode 100644 index 000000000..79d7f5daa --- /dev/null +++ b/namespaces/sync_pipe_test.go @@ -0,0 +1,63 @@ +package namespaces + +import ( + "fmt" + "testing" +) + +func TestSendErrorFromChild(t *testing.T) { + pipe, err := NewSyncPipe() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := pipe.Close(); err != nil { + t.Fatal(err) + } + }() + + expected := "something bad happened" + + pipe.ReportChildError(fmt.Errorf(expected)) + + childError := pipe.ReadFromChild() + if childError == nil { + t.Fatal("expected an error to be returned but did not receive anything") + } + + if childError.Error() != expected { + t.Fatalf("expected %q but received error message %q", expected, childError.Error()) + } +} + +func TestSendPayloadToChild(t *testing.T) { + pipe, err := NewSyncPipe() + if err != nil { + t.Fatal(err) + } + + defer func() { + if err := pipe.Close(); err != nil { + t.Fatal(err) + } + }() + + expected := "libcontainer" + + if err := pipe.SendToChild(map[string]string{"name": expected}); err != nil { + t.Fatal(err) + } + + payload, err := pipe.ReadFromParent() + if err != nil { + t.Fatal(err) + } + + if len(payload) != 1 { + t.Fatalf("expected to only have one value in the payload but received %d", len(payload)) + } + + if name := payload["name"]; name != expected { + t.Fatalf("expected name %q but received %q", expected, name) + } +} From 73ff1addf935f64e3049da47e3a652f78ad17b3e Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 12:03:51 -0700 Subject: [PATCH 4/7] Fix cross compile with syscall Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/sync_pipe.go | 14 -------------- namespaces/sync_pipe_linux.go | 20 ++++++++++++++++++++ 2 files changed, 20 insertions(+), 14 deletions(-) create mode 100644 namespaces/sync_pipe_linux.go diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index 8a7c62215..dd947c8eb 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -17,20 +17,6 @@ type SyncPipe struct { parent, child *os.File } -func NewSyncPipe() (s *SyncPipe, err error) { - s = &SyncPipe{} - - fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0) - if err != nil { - return nil, err - } - - s.child = os.NewFile(uintptr(fds[0]), "child syncpipe") - s.parent = os.NewFile(uintptr(fds[1]), "parent syncpipe") - - return s, nil -} - func NewSyncPipeFromFd(parendFd, childFd uintptr) (*SyncPipe, error) { s := &SyncPipe{} diff --git a/namespaces/sync_pipe_linux.go b/namespaces/sync_pipe_linux.go new file mode 100644 index 000000000..ad61e75d2 --- /dev/null +++ b/namespaces/sync_pipe_linux.go @@ -0,0 +1,20 @@ +package namespaces + +import ( + "os" + "syscall" +) + +func NewSyncPipe() (s *SyncPipe, err error) { + s = &SyncPipe{} + + fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0) + if err != nil { + return nil, err + } + + s.child = os.NewFile(uintptr(fds[0]), "child syncpipe") + s.parent = os.NewFile(uintptr(fds[1]), "parent syncpipe") + + return s, nil +} From e098c02ef7b34a9c783b8e49a94da9769cfdcd5b Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 14:06:26 -0700 Subject: [PATCH 5/7] Update tests for network state Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/sync_pipe_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/namespaces/sync_pipe_test.go b/namespaces/sync_pipe_test.go index 79d7f5daa..69bd0abbf 100644 --- a/namespaces/sync_pipe_test.go +++ b/namespaces/sync_pipe_test.go @@ -3,6 +3,8 @@ package namespaces import ( "fmt" "testing" + + "github.com/docker/libcontainer/network" ) func TestSendErrorFromChild(t *testing.T) { @@ -44,7 +46,7 @@ func TestSendPayloadToChild(t *testing.T) { expected := "libcontainer" - if err := pipe.SendToChild(map[string]string{"name": expected}); err != nil { + if err := pipe.SendToChild(&network.NetworkState{VethHost: expected}); err != nil { t.Fatal(err) } @@ -53,11 +55,7 @@ func TestSendPayloadToChild(t *testing.T) { t.Fatal(err) } - if len(payload) != 1 { - t.Fatalf("expected to only have one value in the payload but received %d", len(payload)) - } - - if name := payload["name"]; name != expected { - t.Fatalf("expected name %q but received %q", expected, name) + if payload.VethHost != expected { + t.Fatalf("expected veth host %q but received %q", expected, payload.VethHost) } } From bd7d1eb7b68468167df73c4cca017bcc1789ed1a Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 14:08:49 -0700 Subject: [PATCH 6/7] Fix parent type in sync pipe Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/sync_pipe.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index dd947c8eb..f2e349412 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -17,11 +17,11 @@ type SyncPipe struct { parent, child *os.File } -func NewSyncPipeFromFd(parendFd, childFd uintptr) (*SyncPipe, error) { +func NewSyncPipeFromFd(parentFd, childFd uintptr) (*SyncPipe, error) { s := &SyncPipe{} - if parendFd > 0 { - s.parent = os.NewFile(parendFd, "parendPipe") + if parentFd > 0 { + s.parent = os.NewFile(parentFd, "parentPipe") } else if childFd > 0 { s.child = os.NewFile(childFd, "childPipe") } else { From a980a961c108e9f74738984c671034a50d48294b Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 26 Jun 2014 14:18:39 -0700 Subject: [PATCH 7/7] Return error on shutdown call Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- namespaces/sync_pipe.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/namespaces/sync_pipe.go b/namespaces/sync_pipe.go index f2e349412..dcb5d9749 100644 --- a/namespaces/sync_pipe.go +++ b/namespaces/sync_pipe.go @@ -44,9 +44,10 @@ func (s *SyncPipe) SendToChild(networkState *network.NetworkState) error { if err != nil { return err } + s.parent.Write(data) - syscall.Shutdown(int(s.parent.Fd()), syscall.SHUT_WR) - return nil + + return syscall.Shutdown(int(s.parent.Fd()), syscall.SHUT_WR) } func (s *SyncPipe) ReadFromChild() error {