Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #10824. Adjusted docker stats memory output #32777

Closed

Conversation

@Sergeant007
Copy link

commented Apr 22, 2017

Signed-off-by: Sergey Tryuber Sergeant007@users.noreply.github.com

- What I did

"docker stats" CLI command now outputs memory usage without page cache. Fixes #10824.

- How I did it

Followed to the solution suggested by @crosbymichael in #10824 - "just subtract the cache from the usage value before displaying it to the user".

- How to verify it

#10824 contains the debug steps to prove that "docker stats" used to include page cache in its output. This PR corrects the behavior.

Experiment (Docker version 17.06.0-dev, build 1eec7b5). The steps are done inside development container, you would need to open several terminals to the development container.

Step 0. build the binaries and then start docker daemon listening on tcp

// start with tcp since curl in development container cannot work with file sockets
$dockerd -D -H tcp://0.0.0.0:2375

Step 1. start a container with bash

docker -H localhost:2375 run --name mem_stats_test --interactive --tty ubuntu:14.04 /bin/bash

Step 2. open a new terminal window and ensure that memory consumption in stats is low

$ docker -H localhost:2375 stats mem_stats_test --no-stream
CONTAINER           CPU %               MEM USAGE / LIMIT     MEM %               NET I/O             BLOCK I/O           PIDS
mem_stats_test      0.00%               1.699MiB / 15.58GiB   0.01%               648B / 0B           0B / 0B             1
$
$curl http://localhost:2375/containers/mem_stats_test/stats?stream=false | python -m json.tool
...
    "memory_stats": {
        "limit": 16724922368,
        "max_usage": 2629632,
        "stats": {
            "active_anon": 507904,
            "active_file": 0,
            "cache": 20480,                  <-- CACHE
            "dirty": 0,
            "hierarchical_memory_limit": 9223372036854771712,
            "inactive_anon": 4096,
            "inactive_file": 0,
            "mapped_file": 0,
            "pgfault": 1377,
            "pgmajfault": 0,
            "pgpgin": 754,
            "pgpgout": 629,
            "rss": 491520,
            "rss_huge": 0,
            "total_active_anon": 507904,
            "total_active_file": 0,
            "total_cache": 20480,
            "total_dirty": 0,
            "total_inactive_anon": 4096,
            "total_inactive_file": 0,
            "total_mapped_file": 0,
            "total_pgfault": 1377,
            "total_pgmajfault": 0,
            "total_pgpgin": 754,
            "total_pgpgout": 629,
            "total_rss": 491520,
            "total_rss_huge": 0,
            "total_unevictable": 0,
            "total_writeback": 0,
            "unevictable": 0,
            "writeback": 0
        },
        "usage": 1802240                     <-- OVERALL USAGE
    },
...

Step 3. Let is work for several hours to ensure that memory doesn't grow significanly (or believe me ans skip this step)

Step 4. Do some heavy file operations that grow page cache (in docker container console from Step 1)

# # Once following operation is finished, page cache will grow, but no additional "real" memory consumption is added
# dd if=/dev/urandom of=/tmp/1GB.bin bs=64M count=16 iflag=fullblock
16+0 records in
16+0 records out
1073741824 bytes (1.1 GB) copied, 10.9137 s, 98.4 MB/s

Step 5. Check docker stats again to see that there is no page cache included in the output

$ # reported memoty DID NOT grew up (significantly)!!!
$ docker -H localhost:2375 stats mem_stats_test --no-stream
CONTAINER           CPU %               MEM USAGE / LIMIT     MEM %               NET I/O             BLOCK I/O           PIDS
mem_stats_test      0.00%               4.039MiB / 15.58GiB   0.03%               648B / 0B           0B / 0B             1
$curl http://localhost:2375/containers/mem_stats_test/stats?stream=false | python -m json.tool
...
"memory_stats": {
        "limit": 16724922368,
        "max_usage": 1145520128,
        "stats": {
            "active_anon": 520192,
            "active_file": 0,
            "cache": 1073766400,                          <-- OUR PAGE CACHE
            "dirty": 0,
            "hierarchical_memory_limit": 9223372036854771712,
            "inactive_anon": 0,
            "inactive_file": 1073741824,
            "mapped_file": 0,
            "pgfault": 2050,
            "pgmajfault": 0,
            "pgpgin": 263492,
            "pgpgout": 1221,
            "rss": 495616,
            "rss_huge": 0,
            "total_active_anon": 520192,
            "total_active_file": 0,
            "total_cache": 1073766400,
            "total_dirty": 0,
            "total_inactive_anon": 0,
            "total_inactive_file": 1073741824,
            "total_mapped_file": 0,
            "total_pgfault": 2050,
            "total_pgmajfault": 0,
            "total_pgpgin": 263492,
            "total_pgpgout": 1221,
            "total_rss": 495616,
            "total_rss_huge": 0,
            "total_unevictable": 0,
            "total_writeback": 0,
            "unevictable": 0,
            "writeback": 0
        },
        "usage": 1078001664                                  <-- THIS INCLUDES PAGE CACHE !!!!
...

Step 6. Conclusion
With this PR docker stats outputs reasonable amount of used memory (no page cache included)

- Description for the changelog

"docker stats" command outputs memory usage with no page cache memory included.

- A picture of a cute animal (not mandatory but encouraged)

My cat Tom.
tom

Copy link
Member

left a comment

unit-tests are missing.

@GordonTheTurtle

This comment has been minimized.

Copy link

commented Apr 27, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "10824-dockerstats-misleading-mem" git@github.com:Sergeant007/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354520728
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@Sergeant007 Sergeant007 force-pushed the Sergeant007:10824-dockerstats-misleading-mem branch from f1e18e6 to 31fe27a Apr 27, 2017
@Sergeant007 Sergeant007 force-pushed the Sergeant007:10824-dockerstats-misleading-mem branch from 31fe27a to 4c56d4a Apr 27, 2017
@GordonTheTurtle GordonTheTurtle removed the dco/no label Apr 27, 2017
@Sergeant007

This comment has been minimized.

Copy link
Author

commented Apr 27, 2017

@ripcurld0, @vdemeester
Added unit tests. Please, review.

Copy link
Member

left a comment

left some suggestions :)

// MemoryStats.Limit will never be 0 unless the container is not running and we haven't
// got any data from cgroup
if limit != 0 {
return float64(usedNoCache) / float64(limit) * 100.0

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 27, 2017

Member

why the float64(usedNoCache)? (usedNoCache already is a float64)

memLimit = float64(v.MemoryStats.Limit)
memPerc = memPercent
memPercent = calculateMemPercentUnixNoCache(v.MemoryStats.Limit, mem)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 27, 2017

Member

you can pass memLimit here instead of v.MemoryStats.Limit, and change the signature of calculateMemPercentUnixNoCache to;

func calculateMemPercentUnixNoCache(limit float64, usedNoCache float64) float64
@@ -227,3 +222,17 @@ func calculateNetwork(network map[string]types.NetworkStats) (float64, float64)
}
return rx, tx
}

//there is no reason do display memory that includes page cache

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Apr 27, 2017

Member

Perhaps use GoDoc convention here;

// calculateMemUsageUnixNoCache calculates the amount of memory excluding page cache

or something like that 😄

Signed-off-by: Sergey Tryuber <Sergeant007@users.noreply.github.com>
@Sergeant007 Sergeant007 force-pushed the Sergeant007:10824-dockerstats-misleading-mem branch from 4c56d4a to 706fc05 Apr 28, 2017
@Sergeant007

This comment has been minimized.

Copy link
Author

commented Apr 28, 2017

Hi @thaJeztah
Thanks for good suggestions, I've corrected all of them. Please review.
I wasn't able to find any "godoc documentation convention" - only blog posts. So I did my best there. Please, advise if necessary.

Copy link
Member

left a comment

LGTM, thanks!

@boaz0
boaz0 approved these changes Apr 30, 2017
Copy link
Member

left a comment

👍

@Sergeant007

This comment has been minimized.

Copy link
Author

commented May 10, 2017

Hi @stevvooe,
I see that you was assigned to merge the pull request, but now there are merge conflict. Will you be able to resolve them by yourself?

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Please resolve by yourself

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Also, now this needs to be moved to https://github.com/docker/cli

@Sergeant007

This comment has been minimized.

Copy link
Author

commented May 13, 2017

Hi @AkihiroSuda

Please resolve by yourself
Also, now this needs to be moved to https://github.com/docker/cli

This does not "needs to be moved", this is throwing away current and making a brand new pull request and large chunk of "paper work", just because you, guys, are not friendly to 3rd party contributions (or slow, or whatever else - please, find and fire).

Unfortunately I'll have time to do the adjustment only in next several weeks.

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented May 13, 2017

CL is now Docker, Inc.'s product and not under the scope of Moby project. (#32694)

I carried this PR to docker/cli#80

you, guys, are not friendly to 3rd party contributions (or slow, or whatever else - please, find and fire).

Even I'm one of 3rd party contributors, not an employee of Docker, Inc. 😕

@thaJeztah

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@Sergeant007 please try to keep your comments respectful? We realise there's extra work needed during the transition to a separate Moby project and Docker product, which we're right in the middle of. We do try to review and merge PR's as quickly as possible, but rebases and merge-conflicts cannot be avoided in a fast moving repository, but are happy to help where needed.

thaJeztah added a commit to docker/cli that referenced this pull request May 17, 2017
[Carry moby/moby#32777] Adjusted docker stats memory output
@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented May 18, 2017

merged docker/cli#80, closing

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request May 19, 2017
[Carry moby/moby#32777] Adjusted docker stats memory output
Upstream-commit: 85c2330dfa4d3845d4cd5f4502709335bce8ee0e
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.