From 52f0474851298e7ba70b1a7ea16e3421d5926c98 Mon Sep 17 00:00:00 2001 From: John Howard Date: Wed, 17 Aug 2016 15:46:28 -0700 Subject: [PATCH] Windows: docker top implementation Signed-off-by: John Howard --- daemon/top_windows.go | 43 ++++++++++++----- integration-cli/benchmark_test.go | 2 +- integration-cli/docker_api_containers_test.go | 4 +- integration-cli/docker_cli_ps_test.go | 2 +- .../docker_cli_service_update_test.go | 2 +- integration-cli/docker_cli_top_test.go | 46 +++++++++++++++---- .../docker_deprecated_api_v124_test.go | 2 +- integration-cli/docker_utils.go | 2 +- integration-cli/test_vars.go | 11 +++++ integration-cli/test_vars_unix.go | 2 - integration-cli/test_vars_windows.go | 3 -- libcontainerd/client_windows.go | 25 +++++----- libcontainerd/types_windows.go | 12 ++--- 13 files changed, 105 insertions(+), 51 deletions(-) create mode 100644 integration-cli/test_vars.go diff --git a/daemon/top_windows.go b/daemon/top_windows.go index 123eb29862..3dd8ead468 100644 --- a/daemon/top_windows.go +++ b/daemon/top_windows.go @@ -2,31 +2,52 @@ package daemon import ( "errors" - "strconv" + "fmt" + "time" "github.com/docker/docker/api/types" + "github.com/docker/go-units" ) -// ContainerTop is a minimal implementation on Windows currently. -// TODO Windows: This needs more work, but needs platform API support. -// All we can currently return (particularly in the case of Hyper-V containers) -// is a PID and the command. -func (daemon *Daemon) ContainerTop(containerID string, psArgs string) (*types.ContainerProcessList, error) { - - // It's really not an equivalent to linux 'ps' on Windows +// ContainerTop handles `docker top` client requests. +// Future considerations: +// -- Windows users are far more familiar with CPU% total. +// Further, users on Windows rarely see user/kernel CPU stats split. +// The kernel returns everything in terms of 100ns. To obtain +// CPU%, we could do something like docker stats does which takes two +// samples, subtract the difference and do the maths. Unfortunately this +// would slow the stat call down and require two kernel calls. So instead, +// we do something similar to linux and display the CPU as combined HH:MM:SS.mmm. +// -- Perhaps we could add an argument to display "raw" stats +// -- "Memory" is an extremely overloaded term in Windows. Hence we do what +// task manager does and use the private working set as the memory counter. +// We could return more info for those who really understand how memory +// management works in Windows if we introduced a "raw" stats (above). +func (daemon *Daemon) ContainerTop(name string, psArgs string) (*types.ContainerProcessList, error) { + // It's not at all an equivalent to linux 'ps' on Windows if psArgs != "" { return nil, errors.New("Windows does not support arguments to top") } - s, err := daemon.containerd.Summary(containerID) + container, err := daemon.GetContainer(name) if err != nil { return nil, err } + s, err := daemon.containerd.Summary(container.ID) + if err != nil { + return nil, err + } procList := &types.ContainerProcessList{} + procList.Titles = []string{"Name", "PID", "CPU", "Private Working Set"} - for _, v := range s { - procList.Titles = append(procList.Titles, strconv.Itoa(int(v.Pid))+" "+v.Command) + for _, j := range s { + d := time.Duration((j.KernelTime100ns + j.UserTime100ns) * 100) // Combined time in nanoseconds + procList.Processes = append(procList.Processes, []string{ + j.ImageName, + fmt.Sprint(j.ProcessId), + fmt.Sprintf("%02d:%02d:%02d.%03d", int(d.Hours()), int(d.Minutes())%60, int(d.Seconds())%60, int(d.Nanoseconds()/1000000)%1000), + units.HumanSize(float64(j.MemoryWorkingSetPrivateBytes))}) } return procList, nil } diff --git a/integration-cli/benchmark_test.go b/integration-cli/benchmark_test.go index 647d014d30..b87e131b7e 100644 --- a/integration-cli/benchmark_test.go +++ b/integration-cli/benchmark_test.go @@ -29,7 +29,7 @@ func (s *DockerSuite) BenchmarkConcurrentContainerActions(c *check.C) { defer innerGroup.Done() for i := 0; i < numIterations; i++ { args := []string{"run", "-d", defaultSleepImage} - args = append(args, defaultSleepCommand...) + args = append(args, sleepCommandForDaemonPlatform()...) out, _, err := dockerCmdWithError(args...) if err != nil { chErr <- fmt.Errorf(out) diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index 567413e1bd..53e3ec90bd 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -881,7 +881,7 @@ func (s *DockerSuite) TestContainerApiStart(c *check.C) { name := "testing-start" config := map[string]interface{}{ "Image": "busybox", - "Cmd": append([]string{"/bin/sh", "-c"}, defaultSleepCommand...), + "Cmd": append([]string{"/bin/sh", "-c"}, sleepCommandForDaemonPlatform()...), "OpenStdin": true, } @@ -1117,7 +1117,7 @@ func (s *DockerSuite) TestContainerApiChunkedEncoding(c *check.C) { config := map[string]interface{}{ "Image": "busybox", - "Cmd": append([]string{"/bin/sh", "-c"}, defaultSleepCommand...), + "Cmd": append([]string{"/bin/sh", "-c"}, sleepCommandForDaemonPlatform()...), "OpenStdin": true, } b, err := json.Marshal(config) diff --git a/integration-cli/docker_cli_ps_test.go b/integration-cli/docker_cli_ps_test.go index 044e710459..225c6f5424 100644 --- a/integration-cli/docker_cli_ps_test.go +++ b/integration-cli/docker_cli_ps_test.go @@ -640,7 +640,7 @@ func (s *DockerSuite) TestPsImageIDAfterUpdate(c *check.C) { originalImageID, err := getIDByName(originalImageName) c.Assert(err, checker.IsNil) - runCmd = exec.Command(dockerBinary, append([]string{"run", "-d", originalImageName}, defaultSleepCommand...)...) + runCmd = exec.Command(dockerBinary, append([]string{"run", "-d", originalImageName}, sleepCommandForDaemonPlatform()...)...) out, _, err = runCommandWithOutput(runCmd) c.Assert(err, checker.IsNil) containerID := strings.TrimSpace(out) diff --git a/integration-cli/docker_cli_service_update_test.go b/integration-cli/docker_cli_service_update_test.go index e99a176bcf..dd71cb0e7d 100644 --- a/integration-cli/docker_cli_service_update_test.go +++ b/integration-cli/docker_cli_service_update_test.go @@ -14,7 +14,7 @@ func (s *DockerSwarmSuite) TestServiceUpdatePort(c *check.C) { d := s.AddDaemon(c, true, true) serviceName := "TestServiceUpdatePort" - serviceArgs := append([]string{"service", "create", "--name", serviceName, "-p", "8080:8081", defaultSleepImage}, defaultSleepCommand...) + serviceArgs := append([]string{"service", "create", "--name", serviceName, "-p", "8080:8081", defaultSleepImage}, sleepCommandForDaemonPlatform()...) // Create a service with a port mapping of 8080:8081. out, err := d.Cmd(serviceArgs...) diff --git a/integration-cli/docker_cli_top_test.go b/integration-cli/docker_cli_top_test.go index e0865b9212..caae29024a 100644 --- a/integration-cli/docker_cli_top_test.go +++ b/integration-cli/docker_cli_top_test.go @@ -4,32 +4,62 @@ import ( "strings" "github.com/docker/docker/pkg/integration/checker" + icmd "github.com/docker/docker/pkg/integration/cmd" "github.com/go-check/check" ) func (s *DockerSuite) TestTopMultipleArgs(c *check.C) { - testRequires(c, DaemonIsLinux) - out, _ := dockerCmd(c, "run", "-i", "-d", "busybox", "top") + out, _ := runSleepingContainer(c, "-d") cleanedContainerID := strings.TrimSpace(out) - out, _ = dockerCmd(c, "top", cleanedContainerID, "-o", "pid") - c.Assert(out, checker.Contains, "PID", check.Commentf("did not see PID after top -o pid: %s", out)) + var expected icmd.Expected + switch daemonPlatform { + case "windows": + expected = icmd.Expected{ExitCode: 1, Err: "Windows does not support arguments to top"} + default: + expected = icmd.Expected{Out: "PID"} + } + result := dockerCmdWithResult("top", cleanedContainerID, "-o", "pid") + c.Assert(result, icmd.Matches, expected) } func (s *DockerSuite) TestTopNonPrivileged(c *check.C) { - testRequires(c, DaemonIsLinux) - out, _ := dockerCmd(c, "run", "-i", "-d", "busybox", "top") + out, _ := runSleepingContainer(c, "-d") cleanedContainerID := strings.TrimSpace(out) out1, _ := dockerCmd(c, "top", cleanedContainerID) out2, _ := dockerCmd(c, "top", cleanedContainerID) dockerCmd(c, "kill", cleanedContainerID) - c.Assert(out1, checker.Contains, "top", check.Commentf("top should've listed `top` in the process list, but failed the first time")) - c.Assert(out2, checker.Contains, "top", check.Commentf("top should've listed `top` in the process list, but failed the second time")) + // Windows will list the name of the launched executable which in this case is busybox.exe, without the parameters. + // Linux will display the command executed in the container + var lookingFor string + if daemonPlatform == "windows" { + lookingFor = "busybox.exe" + } else { + lookingFor = "top" + } + + c.Assert(out1, checker.Contains, lookingFor, check.Commentf("top should've listed `%s` in the process list, but failed the first time", lookingFor)) + c.Assert(out2, checker.Contains, lookingFor, check.Commentf("top should've listed `%s` in the process list, but failed the second time", lookingFor)) +} + +// TestTopWindowsCoreProcesses validates that there are lines for the critical +// processes which are found in a Windows container. Note Windows is architecturally +// very different to Linux in this regard. +func (s *DockerSuite) TestTopWindowsCoreProcesses(c *check.C) { + testRequires(c, DaemonIsWindows) + out, _ := runSleepingContainer(c, "-d") + cleanedContainerID := strings.TrimSpace(out) + out1, _ := dockerCmd(c, "top", cleanedContainerID) + lookingFor := []string{"smss.exe", "csrss.exe", "wininit.exe", "services.exe", "lsass.exe", "CExecSvc.exe"} + for i, s := range lookingFor { + c.Assert(out1, checker.Contains, s, check.Commentf("top should've listed `%s` in the process list, but failed. Test case %d", s, i)) + } } func (s *DockerSuite) TestTopPrivileged(c *check.C) { + // Windows does not support --privileged testRequires(c, DaemonIsLinux, NotUserNamespace) out, _ := dockerCmd(c, "run", "--privileged", "-i", "-d", "busybox", "top") cleanedContainerID := strings.TrimSpace(out) diff --git a/integration-cli/docker_deprecated_api_v124_test.go b/integration-cli/docker_deprecated_api_v124_test.go index 8e2823477c..c2690d794d 100644 --- a/integration-cli/docker_deprecated_api_v124_test.go +++ b/integration-cli/docker_deprecated_api_v124_test.go @@ -162,7 +162,7 @@ func (s *DockerSuite) TestDeprecatedPostContainersStartWithoutLinksInHostConfig( // An alternate test could be written to validate the negative testing aspect of this testRequires(c, DaemonIsLinux) name := "test-host-config-links" - dockerCmd(c, append([]string{"create", "--name", name, "busybox"}, defaultSleepCommand...)...) + dockerCmd(c, append([]string{"create", "--name", name, "busybox"}, sleepCommandForDaemonPlatform()...)...) hc := inspectFieldJSON(c, name, "HostConfig") config := `{"HostConfig":` + hc + `}` diff --git a/integration-cli/docker_utils.go b/integration-cli/docker_utils.go index e56b9f2949..fb3c93776d 100644 --- a/integration-cli/docker_utils.go +++ b/integration-cli/docker_utils.go @@ -1435,7 +1435,7 @@ func runSleepingContainerInImage(c *check.C, image string, extraArgs ...string) args := []string{"run", "-d"} args = append(args, extraArgs...) args = append(args, image) - args = append(args, defaultSleepCommand...) + args = append(args, sleepCommandForDaemonPlatform()...) return dockerCmd(c, args...) } diff --git a/integration-cli/test_vars.go b/integration-cli/test_vars.go new file mode 100644 index 0000000000..97bcddd5f4 --- /dev/null +++ b/integration-cli/test_vars.go @@ -0,0 +1,11 @@ +package main + +// sleepCommandForDaemonPlatform is a helper function that determines what +// the command is for a sleeping container based on the daemon platform. +// The Windows busybox image does not have a `top` command. +func sleepCommandForDaemonPlatform() []string { + if daemonPlatform == "windows" { + return []string{"sleep", "240"} + } + return []string{"top"} +} diff --git a/integration-cli/test_vars_unix.go b/integration-cli/test_vars_unix.go index 853889abe7..f9ecc01123 100644 --- a/integration-cli/test_vars_unix.go +++ b/integration-cli/test_vars_unix.go @@ -12,5 +12,3 @@ const ( // runs indefinitely while still being interruptible by a signal. defaultSleepImage = "busybox" ) - -var defaultSleepCommand = []string{"top"} diff --git a/integration-cli/test_vars_windows.go b/integration-cli/test_vars_windows.go index ff51f89bde..bfc9a5a915 100644 --- a/integration-cli/test_vars_windows.go +++ b/integration-cli/test_vars_windows.go @@ -13,6 +13,3 @@ const ( // on `sleep` with a high duration. defaultSleepImage = "busybox" ) - -// TODO Windows: In TP5, decrease this sleep time, as performance will be better -var defaultSleepCommand = []string{"sleep", "240"} diff --git a/libcontainerd/client_windows.go b/libcontainerd/client_windows.go index 95e508478b..b5be8ee469 100644 --- a/libcontainerd/client_windows.go +++ b/libcontainerd/client_windows.go @@ -410,26 +410,23 @@ func (clnt *client) GetPidsForContainer(containerID string) ([]int, error) { // visible on the container host. However, libcontainerd does have // that information. func (clnt *client) Summary(containerID string) ([]Summary, error) { - var s []Summary + + // Get the libcontainerd container object clnt.lock(containerID) defer clnt.unlock(containerID) - cont, err := clnt.getContainer(containerID) + container, err := clnt.getContainer(containerID) if err != nil { return nil, err } - - // Add the first process - s = append(s, Summary{ - Pid: cont.containerCommon.systemPid, - Command: cont.ociSpec.Process.Args[0]}) - // And add all the exec'd processes - for _, p := range cont.processes { - s = append(s, Summary{ - Pid: p.processCommon.systemPid, - Command: p.commandLine}) + p, err := container.hcsContainer.ProcessList() + if err != nil { + return nil, err } - return s, nil - + pl := make([]Summary, len(p)) + for i := range p { + pl[i] = Summary(p[i]) + } + return pl, nil } // UpdateResources updates resources for a running container. diff --git a/libcontainerd/types_windows.go b/libcontainerd/types_windows.go index 65264a9811..abe10e4f5f 100644 --- a/libcontainerd/types_windows.go +++ b/libcontainerd/types_windows.go @@ -1,6 +1,9 @@ package libcontainerd -import "github.com/docker/docker/libcontainerd/windowsoci" +import ( + "github.com/Microsoft/hcsshim" + "github.com/docker/docker/libcontainerd/windowsoci" +) // Spec is the base configuration for the container. type Spec windowsoci.WindowsSpec @@ -11,11 +14,8 @@ type Process windowsoci.Process // User specifies user information for the containers main process. type User windowsoci.User -// Summary contains a container summary from containerd -type Summary struct { - Pid uint32 - Command string -} +// Summary contains a ProcessList item from HCS to support `top` +type Summary hcsshim.ProcessListItem // StateInfo contains description about the new state container has entered. type StateInfo struct {