From 29bdcaf3cfe0f4bfeed9f7f59ca8f6ad2f41dfd9 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Sun, 24 May 2015 20:05:10 -0700 Subject: [PATCH 1/7] Fix race condition on container stop I'm fairly consistently seeing an error in DockerSuite.TestContainerApiRestartNotimeoutParam: docker_api_containers_test.go:969: c.Assert(status, check.Equals, http.StatusNoContent) ... obtained int = 500 ... expected int = 204 And in the daemon logs I see: INFO[0003] Container 8cf77c20275586b36c5095613159cf73babf92ba42ed4a2954bd55dca6b08971 failed to exit within 0 seconds of SIGTERM - using the force ERRO[0003] Handler for POST /containers/{name:.*}/restart returned error: Cannot restart container 8cf77c20275586b36c5095613159cf73babf92ba42ed4a2954bd55dca6b08971: [2] Container does not exist: container destroyed ERRO[0003] HTTP Error err=Cannot restart container 8cf77c20275586b36c5095613159cf73babf92ba42ed4a2954bd55dca6b08971: [2] Container does not exist: container destroyed statusCode=500 Note the "container destroyed" error message. This is being generatd by the libcontainer code and bubbled up in container.Kill() as a result of the call to `container.killPossiblyDeadProcess(9)` on line 439. See the comment in the code, but what I think is going on is that because we don't have any timeout on the Stop() call we immediate try to force things to stop. And by the time we get into libcontainer code the process just finished stopping due to the initial signal, so this secondary sig-9 fails due to the container no longer running (ie. its 'destroyed'). Since we can't look for "container destroyed" to just ignore the error, because some other driver might have different text, I opted to just ignore the error and keep going - with the assumption that if it couldnt send a sig-9 to the process then it MUST be because its already dead and not something else. To reproduce this I just run: curl -v -X POST http://127.0.0.1:2375/v1.19/containers/8cf77c20275586b36c5095613159cf73babf92ba42ed4a2954bd55dca6b08971/restart a few times and then it fails with the HTTP 500. Would like to hear some other ideas on to handle this since I'm not thrilled with the proposed solution. Signed-off-by: Doug Davis --- daemon/container.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/daemon/container.go b/daemon/container.go index 1f65386fa9..e7fe04b7b8 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -437,7 +437,23 @@ func (container *Container) Kill() error { // 1. Send SIGKILL if err := container.killPossiblyDeadProcess(9); err != nil { - return err + // While normally we might "return err" here we're not going to + // because if we can't stop the container by this point then + // its probably because its already stopped. Meaning, between + // the time of the IsRunning() call above and now it stopped. + // Also, since the err return will be exec driver specific we can't + // look for any particular (common) error that would indicate + // that the process is already dead vs something else going wrong. + // So, instead we'll give it up to 2 more seconds to complete and if + // by that time the container is still running, then the error + // we got is probably valid and so we return it to the caller. + + if container.IsRunning() { + container.WaitStop(2 * time.Second) + if container.IsRunning() { + return err + } + } } // 2. Wait for the process to die, in last resort, try to kill the process directly From a08fb73f9387e22b222d90e638b9f512c3295bdb Mon Sep 17 00:00:00 2001 From: Jessica Frazelle Date: Tue, 26 May 2015 10:31:21 -0700 Subject: [PATCH 2/7] fix lxc tests unshare, they dont use our apparmor profile Signed-off-by: Jessica Frazelle --- integration-cli/docker_cli_run_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 1c47e53dfc..20e8c6ee4c 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -3172,7 +3172,7 @@ func (s *DockerSuite) TestTwoContainersInNetHost(c *check.C) { } func (s *DockerSuite) TestRunUnshareProc(c *check.C) { - testRequires(c, Apparmor) + testRequires(c, Apparmor, NativeExecDriver) name := "acidburn" runCmd := exec.Command(dockerBinary, "run", "--name", name, "jess/unshare", "unshare", "-p", "-m", "-f", "-r", "--mount-proc=/proc", "mount") From 04f99a6ca8232e43169b9a0706e435c551c798a3 Mon Sep 17 00:00:00 2001 From: Josh Hawn Date: Tue, 26 May 2015 14:20:55 -0700 Subject: [PATCH 3/7] Fix container unmount networkMounts UnmountVolumes used to also unmount 'specialMounts' but it no longer does after a recent refactor of volumes. This patch corrects this behavior to include unmounting of `networkMounts` which replaces `specialMounts` (now dead code). Docker-DCO-1.1-Signed-off-by: Josh Hawn (github: jlhawn) --- daemon/container_linux.go | 28 +++++++++++++++----- daemon/volumes.go | 15 ----------- integration-cli/docker_cli_cp_test.go | 37 +++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 21 deletions(-) diff --git a/daemon/container_linux.go b/daemon/container_linux.go index 2490310e9d..f16a71bdb9 100644 --- a/daemon/container_linux.go +++ b/daemon/container_linux.go @@ -956,21 +956,37 @@ func (container *Container) DisableLink(name string) { } func (container *Container) UnmountVolumes(forceSyscall bool) error { - for _, m := range container.MountPoints { - dest, err := container.GetResourcePath(m.Destination) + var volumeMounts []mountPoint + + for _, mntPoint := range container.MountPoints { + dest, err := container.GetResourcePath(mntPoint.Destination) if err != nil { return err } - if forceSyscall { - syscall.Unmount(dest, 0) + volumeMounts = append(volumeMounts, mountPoint{Destination: dest, Volume: mntPoint.Volume}) + } + + for _, mnt := range container.networkMounts() { + dest, err := container.GetResourcePath(mnt.Destination) + if err != nil { + return err } - if m.Volume != nil { - if err := m.Volume.Unmount(); err != nil { + volumeMounts = append(volumeMounts, mountPoint{Destination: dest}) + } + + for _, volumeMount := range volumeMounts { + if forceSyscall { + syscall.Unmount(volumeMount.Destination, 0) + } + + if volumeMount.Volume != nil { + if err := volumeMount.Volume.Unmount(); err != nil { return err } } } + return nil } diff --git a/daemon/volumes.go b/daemon/volumes.go index 2fdcd8311d..963bd58da3 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -8,7 +8,6 @@ import ( "path/filepath" "strings" - "github.com/docker/docker/daemon/execdriver" "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/runconfig" "github.com/docker/docker/volume" @@ -115,20 +114,6 @@ func validMountMode(mode string) bool { return validModes[mode] } -func (container *Container) specialMounts() []execdriver.Mount { - var mounts []execdriver.Mount - if container.ResolvConfPath != "" { - mounts = append(mounts, execdriver.Mount{Source: container.ResolvConfPath, Destination: "/etc/resolv.conf", Writable: !container.hostConfig.ReadonlyRootfs, Private: true}) - } - if container.HostnamePath != "" { - mounts = append(mounts, execdriver.Mount{Source: container.HostnamePath, Destination: "/etc/hostname", Writable: !container.hostConfig.ReadonlyRootfs, Private: true}) - } - if container.HostsPath != "" { - mounts = append(mounts, execdriver.Mount{Source: container.HostsPath, Destination: "/etc/hosts", Writable: !container.hostConfig.ReadonlyRootfs, Private: true}) - } - return mounts -} - func copyExistingContents(source, destination string) error { volList, err := ioutil.ReadDir(source) if err != nil { diff --git a/integration-cli/docker_cli_cp_test.go b/integration-cli/docker_cli_cp_test.go index 26e778e4f2..6d07cff6f8 100644 --- a/integration-cli/docker_cli_cp_test.go +++ b/integration-cli/docker_cli_cp_test.go @@ -596,3 +596,40 @@ func (s *DockerSuite) TestCpNameHasColon(c *check.C) { c.Fatalf("Wrong content in copied file %q, should be %q", content, "lololol\n") } } + +func (s *DockerSuite) TestCopyAndRestart(c *check.C) { + expectedMsg := "hello" + out, err := exec.Command(dockerBinary, "run", "-d", "busybox", "echo", expectedMsg).CombinedOutput() + if err != nil { + c.Fatal(string(out), err) + } + id := strings.TrimSpace(string(out)) + + if out, err = exec.Command(dockerBinary, "wait", id).CombinedOutput(); err != nil { + c.Fatalf("unable to wait for container: %s", err) + } + + status := strings.TrimSpace(string(out)) + if status != "0" { + c.Fatalf("container exited with status %s", status) + } + + tmpDir, err := ioutil.TempDir("", "test-docker-restart-after-copy-") + if err != nil { + c.Fatalf("unable to make temporary directory: %s", err) + } + defer os.RemoveAll(tmpDir) + + if _, err = exec.Command(dockerBinary, "cp", fmt.Sprintf("%s:/etc/issue", id), tmpDir).CombinedOutput(); err != nil { + c.Fatalf("unable to copy from busybox container: %s", err) + } + + if out, err = exec.Command(dockerBinary, "start", "-a", id).CombinedOutput(); err != nil { + c.Fatalf("unable to start busybox container after copy: %s - %s", err, out) + } + + msg := strings.TrimSpace(string(out)) + if msg != expectedMsg { + c.Fatalf("expected %q but got %q", expectedMsg, msg) + } +} From b3e8ab3021d2021202e14b912e7fdbfede4c7c20 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 26 May 2015 22:22:03 -0400 Subject: [PATCH 4/7] Fix unregister stats on when rm running container Signed-off-by: Brian Goff --- daemon/delete.go | 8 ++--- integration-cli/docker_api_containers_test.go | 36 +++++++++++++++++++ integration-cli/utils.go | 23 ++++++++++++ 3 files changed, 63 insertions(+), 4 deletions(-) diff --git a/daemon/delete.go b/daemon/delete.go index 830bbbe267..39d500550e 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -58,10 +58,6 @@ func (daemon *Daemon) ContainerRm(name string, config *ContainerRmConfig) error // Destroy unregisters a container from the daemon and cleanly removes its contents from the filesystem. func (daemon *Daemon) rm(container *Container, forceRemove bool) (err error) { - // stop collection of stats for the container regardless - // if stats are currently getting collected. - daemon.statsCollector.stopCollection(container) - if container.IsRunning() { if !forceRemove { return fmt.Errorf("Conflict, You cannot remove a running container. Stop the container before attempting removal or use -f") @@ -71,6 +67,10 @@ func (daemon *Daemon) rm(container *Container, forceRemove bool) (err error) { } } + // stop collection of stats for the container regardless + // if stats are currently getting collected. + daemon.statsCollector.stopCollection(container) + element := daemon.containers.Get(container.ID) if element == nil { return fmt.Errorf("Container %v not found - maybe it was already destroyed?", container.ID) diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index 363d279120..daec87c497 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -254,6 +254,42 @@ func (s *DockerSuite) TestGetContainerStats(c *check.C) { } } +func (s *DockerSuite) TestContainerStatsRmRunning(c *check.C) { + out, _ := dockerCmd(c, "run", "-d", "busybox", "top") + id := strings.TrimSpace(out) + + buf := &channelBuffer{make(chan []byte, 1)} + defer buf.Close() + chErr := make(chan error) + go func() { + _, body, err := sockRequestRaw("GET", "/containers/"+id+"/stats?stream=1", nil, "application/json") + if err != nil { + chErr <- err + } + defer body.Close() + _, err = io.Copy(buf, body) + chErr <- err + }() + defer func() { + c.Assert(<-chErr, check.IsNil) + }() + + b := make([]byte, 32) + // make sure we've got some stats + _, err := buf.ReadTimeout(b, 2*time.Second) + c.Assert(err, check.IsNil) + + // Now remove without `-f` and make sure we are still pulling stats + _, err = runCommand(exec.Command(dockerBinary, "rm", id)) + c.Assert(err, check.Not(check.IsNil), check.Commentf("rm should have failed but didn't")) + _, err = buf.ReadTimeout(b, 2*time.Second) + c.Assert(err, check.IsNil) + dockerCmd(c, "rm", "-f", id) + + _, err = buf.ReadTimeout(b, 2*time.Second) + c.Assert(err, check.Not(check.IsNil)) +} + func (s *DockerSuite) TestGetStoppedContainerStats(c *check.C) { // TODO: this test does nothing because we are c.Assert'ing in goroutine var ( diff --git a/integration-cli/utils.go b/integration-cli/utils.go index f7ad61b0ac..198054efd3 100644 --- a/integration-cli/utils.go +++ b/integration-cli/utils.go @@ -337,3 +337,26 @@ func parseCgroupPaths(procCgroupData string) map[string]string { } return cgroupPaths } + +type channelBuffer struct { + c chan []byte +} + +func (c *channelBuffer) Write(b []byte) (int, error) { + c.c <- b + return len(b), nil +} + +func (c *channelBuffer) Close() error { + close(c.c) + return nil +} + +func (c *channelBuffer) ReadTimeout(p []byte, n time.Duration) (int, error) { + select { + case b := <-c.c: + return copy(p[0:], b), nil + case <-time.After(n): + return -1, fmt.Errorf("timeout reading from channel") + } +} From 7c7aebfcfef46337aa4e20f9d024916f71650545 Mon Sep 17 00:00:00 2001 From: Qiang Huang Date: Wed, 27 May 2015 18:16:28 +0800 Subject: [PATCH 5/7] Return err if we got err on parseForm Signed-off-by: Qiang Huang --- api/server/server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/server/server.go b/api/server/server.go index 8d2dafa740..1491afe8f2 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -863,7 +863,7 @@ func (s *Server) postImagesLoad(version version.Version, w http.ResponseWriter, func (s *Server) postContainersCreate(version version.Version, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := parseForm(r); err != nil { - return nil + return err } if err := checkForJson(r); err != nil { return err @@ -1288,7 +1288,7 @@ func (s *Server) postContainersCopy(version version.Version, w http.ResponseWrit func (s *Server) postContainerExecCreate(version version.Version, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := parseForm(r); err != nil { - return nil + return err } name := vars["name"] @@ -1317,7 +1317,7 @@ func (s *Server) postContainerExecCreate(version version.Version, w http.Respons // TODO(vishh): Refactor the code to avoid having to specify stream config as part of both create and start. func (s *Server) postContainerExecStart(version version.Version, w http.ResponseWriter, r *http.Request, vars map[string]string) error { if err := parseForm(r); err != nil { - return nil + return err } var ( execName = vars["name"] From 94d604357ff9cf7a5fcc4d98ffe58f804adf0ae6 Mon Sep 17 00:00:00 2001 From: Qiang Huang Date: Wed, 27 May 2015 18:25:57 +0800 Subject: [PATCH 6/7] Remove redundant set header Which is already done in writeJSON. Signed-off-by: Qiang Huang --- api/server/server.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/api/server/server.go b/api/server/server.go index 8d2dafa740..208d5a49e9 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -244,8 +244,6 @@ func (s *Server) postAuth(version version.Version, w http.ResponseWriter, r *htt } func (s *Server) getVersion(version version.Version, w http.ResponseWriter, r *http.Request, vars map[string]string) error { - w.Header().Set("Content-Type", "application/json") - v := &types.Version{ Version: dockerversion.VERSION, ApiVersion: api.APIVERSION, @@ -359,8 +357,6 @@ func (s *Server) getImagesJSON(version version.Version, w http.ResponseWriter, r } func (s *Server) getInfo(version version.Version, w http.ResponseWriter, r *http.Request, vars map[string]string) error { - w.Header().Set("Content-Type", "application/json") - info, err := s.daemon.SystemInfo() if err != nil { return err From a34cb321865c16ac42a12fe32fa3c8c4a5ffd263 Mon Sep 17 00:00:00 2001 From: Hu Keping Date: Wed, 27 May 2015 22:27:36 +0800 Subject: [PATCH 7/7] Remove unused code As far as I see, it's a dead code. Signed-off-by: Hu Keping --- integration-cli/utils.go | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/integration-cli/utils.go b/integration-cli/utils.go index f7ad61b0ac..edaa24c5e2 100644 --- a/integration-cli/utils.go +++ b/integration-cli/utils.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "net/http" "net/http/httptest" "os" "os/exec" @@ -275,26 +274,6 @@ type FileServer struct { *httptest.Server } -func fileServer(files map[string]string) (*FileServer, error) { - var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { - if filePath, found := files[r.URL.Path]; found { - http.ServeFile(w, r, filePath) - } else { - http.Error(w, http.StatusText(404), 404) - } - } - - for _, file := range files { - if _, err := os.Stat(file); err != nil { - return nil, err - } - } - server := httptest.NewServer(handler) - return &FileServer{ - Server: server, - }, nil -} - // randomUnixTmpDirPath provides a temporary unix path with rand string appended. // does not create or checks if it exists. func randomUnixTmpDirPath(s string) string {