From 60c2eac8bfa88fa4dfda7dea56c5d3e1e491e1b9 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 May 2018 13:12:29 -0700 Subject: [PATCH 1/4] daemon unit tests: skip some if non-root This prevents the following test case failures "go test" is run as non-root in the daemon/ directory: > --- FAIL: TestContainerInitDNS (0.02s) > daemon_test.go:209: chown /tmp/docker-container-test-054812199/volumes: operation not permitted > > --- FAIL: TestDaemonReloadNetworkDiagnosticPort (0.00s) > reload_test.go:525: mkdir /var/lib/docker/network/files/: permission denied > --- FAIL: TestRootMountCleanup (0.00s) > daemon_linux_test.go:240: assertion failed: error is not nil: operation not permitted Signed-off-by: Kir Kolyshkin Upstream-commit: 16670ed4842b1ee4853ba39b6ebf2b771d28db9a Component: engine --- components/engine/daemon/daemon_linux_test.go | 4 ++++ components/engine/daemon/daemon_test.go | 4 ++++ components/engine/daemon/reload_test.go | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/components/engine/daemon/daemon_linux_test.go b/components/engine/daemon/daemon_linux_test.go index 3bbbc814c6..1635b08535 100644 --- a/components/engine/daemon/daemon_linux_test.go +++ b/components/engine/daemon/daemon_linux_test.go @@ -229,6 +229,10 @@ func checkMounted(t *testing.T, p string, expect bool) { } func TestRootMountCleanup(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } + t.Parallel() testRoot, err := ioutil.TempDir("", t.Name()) diff --git a/components/engine/daemon/daemon_test.go b/components/engine/daemon/daemon_test.go index 2fe4276d7a..f0a67274a5 100644 --- a/components/engine/daemon/daemon_test.go +++ b/components/engine/daemon/daemon_test.go @@ -153,6 +153,10 @@ func TestValidContainerNames(t *testing.T) { } func TestContainerInitDNS(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") // for chown + } + tmp, err := ioutil.TempDir("", "docker-container-test-") if err != nil { t.Fatal(err) diff --git a/components/engine/daemon/reload_test.go b/components/engine/daemon/reload_test.go index 9174bfba54..97690c0312 100644 --- a/components/engine/daemon/reload_test.go +++ b/components/engine/daemon/reload_test.go @@ -1,6 +1,7 @@ package daemon // import "github.com/docker/docker/daemon" import ( + "os" "reflect" "sort" "testing" @@ -499,6 +500,9 @@ func TestDaemonDiscoveryReloadOnlyClusterAdvertise(t *testing.T) { } func TestDaemonReloadNetworkDiagnosticPort(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("root required") + } daemon := &Daemon{ imageService: images.NewImageService(images.ImageServiceConfig{}), } From 843828ae852e5cbc7c5486c8c2846408c300359f Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 May 2018 16:23:07 -0700 Subject: [PATCH 2/4] daemon/parsePsOutput: minor optimisation It does not make sense to keep looking for PID once we found it, so let's give it a break. The side effect of this patch is, if there's more than one column titled "PID", the last (rightmost) column was used before, and now the first (leftmost) column is used. Should make no practical difference whatsoever. Signed-off-by: Kir Kolyshkin Upstream-commit: 654a7625fc9f0b7b04da0e0e4d151af04a65cc7f Component: engine --- components/engine/daemon/top_unix.go | 1 + 1 file changed, 1 insertion(+) diff --git a/components/engine/daemon/top_unix.go b/components/engine/daemon/top_unix.go index a54fa37ffa..28cfb370d8 100644 --- a/components/engine/daemon/top_unix.go +++ b/components/engine/daemon/top_unix.go @@ -70,6 +70,7 @@ func parsePSOutput(output []byte, procs []uint32) (*container.ContainerTopOKBody for i, name := range procList.Titles { if name == "PID" { pidIndex = i + break } } if pidIndex == -1 { From b7dcd71938cbb7cfd9d8fe904b5826f7bb4eb7ed Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 May 2018 16:26:30 -0700 Subject: [PATCH 3/4] ContainerTop: speed up Current ContainerTop (a.k.a. docker top) implementation uses "ps" to get the info about *all* running processes, then parses it, then filters the results to only contain PIDs used by the container. Collecting data only to throw most of it away is inefficient, especially on a system running many containers (or processes). For example, "docker top" on a container with a single process can take up to 0.5 seconds to execute (on a mostly idle system) which is noticeably slow. Since the containers PIDs are known beforehand, let's use ps's "-q" option to provide it with a list of PIDs we want info about. The problem with this approach is, some ps options can't be used with "-q" (the only one I'm aware of is "f" ("forest view") but there might be more). As the list of such options is not known, in case ps fails, it is executed again without "q" (retaining the old behavior). Next, the data produced by "ps" is filtered in the same way as before. The difference here is, in case "-q" worked, the list is much shorter. I ran some benchmarks on my laptop, with about 8000 "sleep" processes running to amplify the savings. The improvement in "docker top" execution times is 5x to 10x (roughly 0.05s vs 0.5s). The improvement in ContainerTop() execution time is up to 100x (roughly 3ms vs 300ms). I haven't measured the memory or the CPU time savings, guess those are not that critical. NOTE that busybox ps does not implement -q so the fallback is always used, but AFAIK it is not usable anyway and Docker expects a normal ps to be on the system (say the list of fields produced by "busybox ps -ef" differs from normal "ps -ef" etc.). Signed-off-by: Kir Kolyshkin Upstream-commit: a076badb8b33f1ecdc5d46f0a3701f10c0579f73 Component: engine --- components/engine/daemon/top_unix.go | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/top_unix.go b/components/engine/daemon/top_unix.go index 28cfb370d8..7854d2376f 100644 --- a/components/engine/daemon/top_unix.go +++ b/components/engine/daemon/top_unix.go @@ -113,6 +113,20 @@ func parsePSOutput(output []byte, procs []uint32) (*container.ContainerTopOKBody return procList, nil } +// psPidsArg converts a slice of PIDs to a string consisting +// of comma-separated list of PIDs prepended by "-q". +// For example, psPidsArg([]uint32{1,2,3}) returns "-q1,2,3". +func psPidsArg(pids []uint32) string { + b := []byte{'-', 'q'} + for i, p := range pids { + b = strconv.AppendUint(b, uint64(p), 10) + if i < len(pids)-1 { + b = append(b, ',') + } + } + return string(b) +} + // ContainerTop lists the processes running inside of the given // container by calling ps with the given args, or with the flags // "-ef" if no args are given. An error is returned if the container @@ -145,9 +159,16 @@ func (daemon *Daemon) ContainerTop(name string, psArgs string) (*container.Conta return nil, err } - output, err := exec.Command("ps", strings.Split(psArgs, " ")...).Output() + args := strings.Split(psArgs, " ") + pids := psPidsArg(procs) + output, err := exec.Command("ps", append(args, pids)...).Output() if err != nil { - return nil, fmt.Errorf("Error running ps: %v", err) + // some ps options (such as f) can't be used together with q, + // so retry without it + output, err = exec.Command("ps", args...).Output() + if err != nil { + return nil, fmt.Errorf("Error running ps: %v", err) + } } procList, err := parsePSOutput(output, procs) if err != nil { From 034a2859498d38b1432d12dc239f47088922ef1d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 May 2018 17:09:41 -0700 Subject: [PATCH 4/4] ContainerTop: improve error message If "ps" fails, in many cases it prints a meaningful error message which a user can benefit from. Let's use it. While at it, let's use errdefs.System to classify the error, as well as errors.Wrap. Before: > $ docker top $CT > Error response from daemon: Error running ps: exit status 1 After: > $ docker top $CT auxm > Error response from daemon: ps: error: thread display conflicts with forest display or > $ docker top $CT saur > Error response from daemon: ps: error: conflicting format options or, if there's no meaningful error on stderr, same as before: > $ docker top $CT 1234 > Error response from daemon: ps: exit status 1 Signed-off-by: Kir Kolyshkin Upstream-commit: a41328d5704b8d1adbcd099fb4bb0697060df806 Component: engine --- components/engine/daemon/top_unix.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/components/engine/daemon/top_unix.go b/components/engine/daemon/top_unix.go index 7854d2376f..99ca56f0f4 100644 --- a/components/engine/daemon/top_unix.go +++ b/components/engine/daemon/top_unix.go @@ -3,6 +3,7 @@ package daemon // import "github.com/docker/docker/daemon" import ( + "bytes" "context" "fmt" "os/exec" @@ -11,6 +12,8 @@ import ( "strings" "github.com/docker/docker/api/types/container" + "github.com/docker/docker/errdefs" + "github.com/pkg/errors" ) func validatePSArgs(psArgs string) error { @@ -167,7 +170,14 @@ func (daemon *Daemon) ContainerTop(name string, psArgs string) (*container.Conta // so retry without it output, err = exec.Command("ps", args...).Output() if err != nil { - return nil, fmt.Errorf("Error running ps: %v", err) + if ee, ok := err.(*exec.ExitError); ok { + // first line of stderr shows why ps failed + line := bytes.SplitN(ee.Stderr, []byte{'\n'}, 2) + if len(line) > 0 && len(line[0]) > 0 { + err = errors.New(string(line[0])) + } + } + return nil, errdefs.System(errors.Wrap(err, "ps")) } } procList, err := parsePSOutput(output, procs)