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

Allow docker stats without arguments #16742

Merged
merged 1 commit into from Nov 7, 2015

Conversation

runcom
Copy link
Member

@runcom runcom commented Oct 3, 2015

This patch adds the ability to run docker stats w/o arguments and get
statistics for all running containers by default. Also add a new
--all flag to list statistics for all containers (like docker ps).
New running containers are added to the list as they show up also.
Add integration tests for this new behavior.
Docs updated accordingly. Fix missing stuff in man/commandline
reference for docker stats.

Close #10772

Signed-off-by: Antonio Murdaca amurdaca@redhat.com

Run **docker stats** with multiple containers.

$ docker stats redis1 redis2
CONTAINER CPU % MEM USAGE / LIMIT MEM % NET I/O BLOCK I/O
redis1 0.07% 796 KB / 64 MB 1.21% 788 B / 648 B 3.568 MB / 512 KB
redis2 0.07% 2.746 MB / 64 MB 4.29% 1.266 KB / 648 B 12.4 MB / 0 B

# HISTORY
October 2015, updated by Antonio Murdaca <amurdaca@redhat.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New email address? 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha yeah, but this isn't needed I'm sure

@tianon
Copy link
Member

tianon commented Oct 3, 2015

Does this update as containers come and go, or is it a static list from when the command was started? IMO that should be mentioned in the docs for this to make sure expectations are clear. 😄

@runcom
Copy link
Member Author

runcom commented Oct 3, 2015

Does this update as containers come and go, or is it a static list from when the command was started? IMO that should be mentioned in the docs for this to make sure expectations are clear.

This is a static list, but I think I can update the stats list by listening on container's create events in the cli as well and update the list with newly running containers, I'll see what I can do, I just didn't want to overload the stats code again and again :). If it'll go with just a static list I'll make sure to update the docs accordingly :)

@tianon
Copy link
Member

tianon commented Oct 3, 2015 via email

@runcom
Copy link
Member Author

runcom commented Oct 3, 2015

@tianon managed a way to auto update the list in a few line of codes (added an integration test for this also)

@runcom runcom force-pushed the 10772-docker-stats-all branch 3 times, most recently from 4ae94a2 to c2b9503 Compare October 4, 2015 21:48
@runcom
Copy link
Member Author

runcom commented Oct 12, 2015

rebased

ping @cpuguy83 @LK4D4 @icecrime

@duglin
Copy link
Contributor

duglin commented Oct 12, 2015

Looks really cool so far. I like that new containers are added dynamically, but then at the same time it seems that as they die they should be removed, no?

Just too bad it can't fit in 80 columns for those of us who like thin windows :-)

@runcom
Copy link
Member Author

runcom commented Oct 12, 2015

Looks really cool so far. I like that new containers are added dynamically, but then at the same time it seems that as they die they should be removed, no?

the auto-add/auto-remove use the same behavior of docker stats CID so stopped/exited containers are left in the list (showing zeroed stats, as it's currently happening when you docker stats a stopped container) and if you docker rm -f CID it will be removed from the docker stats list.
I like this behavior but if you prefer removing stopped/exited containers from the list I can change the patch to do so :)

@duglin
Copy link
Contributor

duglin commented Oct 12, 2015

It just seems we should be consistent. If I don't see stopped containers when I first run docker stats then I don't think I should see newly stopped ones either. Or, always show them. But I don't think newly stopped ones should be treated differently from older stopped ones - they're both in the same state.

@runcom
Copy link
Member Author

runcom commented Oct 12, 2015

It just seems we should be consistent. If I don't see stopped containers when I first run docker stats then I don't think I should see newly stopped ones either. Or, always show them. But I don't think newly stopped ones should be treated differently from older stopped ones - they're both in the same state.

so docker stats should remove stopped/exited containers while if you run docker stats --all it should display them.
Sounds good?

@duglin
Copy link
Contributor

duglin commented Oct 12, 2015

sounds good to me

@runcom runcom force-pushed the 10772-docker-stats-all branch 2 times, most recently from 9c409af to 7880927 Compare October 12, 2015 16:25
@runcom
Copy link
Member Author

runcom commented Oct 12, 2015

@duglin updated

@duglin
Copy link
Contributor

duglin commented Oct 12, 2015

@runcom it doesn't seem to be removing non-running containers. I did:

docker run -d ubuntu sleep 600
docker run ubuntu echo hi

and I see two appears in the stats, but then the 2nd one never goes away.

Also, you may want to sort the output based on something that's stable, like the IDs or time started. Even with just those two I saw the order reversing quite a lot which makes it hard to look at when you're watching one particular container to see what its doing.

@runcom
Copy link
Member Author

runcom commented Oct 12, 2015

Fixed removing on exited containers, thanks @duglin, I'll take care of the order

@tianon
Copy link
Member

tianon commented Oct 12, 2015 via email


Display a live stream of one or more containers' resource usage statistics

--all=false Show all containers (default shows just running)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you forgot the -a flag here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks @thaJeztah!

@thaJeztah
Copy link
Member

One comment, otherwise docs LGTM

we also need to update completions for this (new flag and container is no longer required for the subcommand) @runcom do you want to take care of those as well, or ping the maintainers of the completion scripts?

@vdemeester
Copy link
Member

😻

@thaJeztah
Copy link
Member

ping @albers @sdurrheimer just talked with @runcom on IRC and if you could do the completions, that'd be awesome!

@thaJeztah
Copy link
Member

ping @SvenDowideit @moxiegirl @jamtur01 thanks!

c.Fatalf("Expected stats output to contain both %s and %s, got %s", id1, id2, out)
}
if strings.Contains(out, id3) {
c.Fatalf("Expected %s not to be listed in stats, got %s", id3, out)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is super awkward.

Expected %s not to be listed in stats, got %s

Did not expect %s in stats. Got %s in stats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

@moxiegirl
Copy link
Contributor

@thaJeztah Is this supposed to be in Docs review?

@albers
Copy link
Member

albers commented Nov 6, 2015

@thaJeztah yeah, I'll do the bash completion part. Thanks for bringing me in.

@thaJeztah
Copy link
Member

@moxiegirl yes it is, so feel free to review :-)

@@ -10,13 +10,22 @@ parent = "smn_cli"

# stats
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runcom Thanks for update. The examples should come after the description. Also, we want to make sure to illustrate the fact they can pass id as well as name. Update please command ref and man..

---> https://gist.github.com/moxiegirl/8744402a9adf5aa288a1

stats

Usage: docker stats [OPTIONS] [CONTAINER...]

Display a live stream of one or more containers' resource usage statistics

  -a, --all=false    Show all containers (default shows just running)
  --help=false       Print usage
  --no-stream=false  Disable streaming stats and only pull the first result

The docker stats command returns a live data stream for running containers. To limit data to one or more specific containers, specify a list of container names or ids separated by a space. You can specify a stopped container but stopped containers do not return any data.

If you want more detailed information about a container's resource usage, use the /containers/(id)/stats API endpoint.

Examples

Running docker stats on all running containers

$ docker stats
CONTAINER           CPU %               MEM USAGE / LIMIT     MEM %               NET I/O             BLOCK I/O
redis1              0.07%               796 KB / 64 MB        1.21%               788 B / 648 B       3.568 MB / 512 KB
redis2              0.07%               2.746 MB / 64 MB      4.29%               1.266 KB / 648 B    12.4 MB / 0 B
nginx1              0.03%               4.583 MB / 64 MB      6.30%               2.854 KB / 648 B    27.7 MB / 0 B

Running docker stats on multiple containers by name and id.

$ docker stats fervent_panini 5acfcb1b4fd1
CONTAINER           CPU %               MEM USAGE/LIMIT     MEM %               NET I/O
5acfcb1b4fd1        0.00%               115.2 MB/1.045 GB   11.03%              1.422 kB/648 B
fervent_panini      0.02%               11.08 MB/1.045 GB   1.06%               648 B/648 B

This patch adds the ability to run `docker stats` w/o arguments and get
statistics for all running containers by default. Also add a new
`--all` flag to list statistics for all containers (like `docker ps`).
New running containers are added to the list as they show up also.
Add integration tests for this new behavior.
Docs updated accordingly. Fix missing stuff in man/commandline
reference for `docker stats`.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Nov 7, 2015

@moxiegirl thanks, PTAL

@moxiegirl
Copy link
Contributor

@runcom Thank you! LGTM ping @thaJeztah for the merge.

@thaJeztah
Copy link
Member

Thanks! LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants