Refactor the statistics of network in docker stats #15786

HuKeping · 2015-08-24T09:37:14Z

For now docker stats will sum the rxbytes, txbytes, etc. of all
the interfaces.

It is OK for the output of CLI docker stats but not good for
the API response, especially when the container is in sereval
subnets.

It's better to leave these origianl data to user.

Signed-off-by: Hu Keping hukeping@huawei.com

HuKeping · 2015-08-24T09:38:41Z

For example, the output of stats after this PR will be like:

    ...
    "network": {
        "eth0": {
            "rx_bytes": 5338,
            "rx_dropped": 0,
            "rx_errors": 0,
            "rx_packets": 36,
            "tx_bytes": 648,
            "tx_dropped": 0,
            "tx_errors": 0,
            "tx_packets": 8
        },
        "eth5": {
            "rx_bytes": 4641,
            "rx_dropped": 0,
            "rx_errors": 0,
            "rx_packets": 26,
            "tx_bytes": 690,
            "tx_dropped": 0,
            "tx_errors": 0,
            "tx_packets": 9
        }
    },
    ...

calavera · 2015-08-24T10:04:04Z

I agree that we should change this. However, we cannot replace the json response without versioning the fields. We should keep returning the current json for request versions <= 1.21 and the new json for request versions > 1.22.

calavera · 2015-08-24T10:04:47Z

It will also need some tests and changes in the api docs 😸

HuKeping · 2015-08-24T13:19:30Z

Test updated.

calavera · 2015-08-25T08:03:32Z

api/types/stats.go

+	Read time.Time `json:"read"`
+
+	// Network is for fallback stats where API Version <= 1.21
+	Network NetworkStats `json:"network,omitempty"`


We rather move deprecated fields to a separated struct. Look what we do with ContainerJSON in this same file. We have a ContainerJSONBase with the common fields and we compose structs for specific versions with it and the fields required by each version of the api.

I do considered to use the similar solution as what the ContainerJSON does. But later I found it is complicated, and provides not obvious value, it will introduce many things like:

if version.LessThan("1.xx") { ... }

which I don't thinks is a good code style, and it will force the relative functions to add arguments version and maybe more things.

Besides the word Network explain itself well and so does the Networks. When someday we decide to drop Network, it is fine to just remove it and it will not need to remove those

if version <=

If you prefer the old ways, it's OK for me.

We're working on a better way to serialize objects in the api, but until then, we prefer the old way. Believe me, I dislike those conditionals as much as you do, but we need to be consistent.

Updated. I tried to not make a over deep change of the existing code. So I place the version-check the moment before this API return:

If request version < 1.21, sum up those original data and return

if request version >=1.21, the original data will be returned

But it still seems quite redundant. any better solutions please @calavera ?

thaJeztah · 2015-08-30T20:57:27Z

@HuKeping looks like this needs a rebase

Also, don't forget to update the example output in the API docs and add a mention to the API change log here; https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md

HuKeping · 2015-08-31T04:17:37Z

@calavera rebased, please take a look if this change is OK

@thaJeztah thanks and I'll update the docs after code review

ehazlett · 2015-09-04T16:25:24Z

@HuKeping would you mind rebasing again? thanks!

HuKeping · 2015-09-06T03:02:03Z

rebased @ehazlett @calavera

calavera · 2015-09-07T15:58:10Z

Thanks @HuKeping. LGTM

runcom · 2015-09-07T16:03:03Z

daemon/stats.go

@@ -58,14 +59,64 @@ func (daemon *Daemon) ContainerStats(name string, config *ContainerStatsConfig)
 				return nil
 			}

-			s := getStat(v)
+			statsJSON := getStatJSON(v)
+			if version.LessThan("1.21") {


can we have booleans in ContainerStatsConfig saying which format to use instead of leaking API version here in daemon?

Put API version in ContainerStatsConfig is a good idea, but I prefer to use version instead of boolean, WDYT

jessfraz · 2015-09-08T21:11:58Z

this will need docs updates right

thaJeztah · 2015-09-08T21:45:50Z

Yup, it does; #15786 (comment)

HuKeping · 2015-09-09T01:51:45Z

Docs added @thaJeztah @jfrazelle , PTAL

thaJeztah · 2015-09-09T10:28:08Z

Thanks, @HuKeping. Docs changes LGTM (but we're not in docs-review yet).
Question; this is only affects the API; the output of docker stats on the CLI is still the same, correct?

HuKeping · 2015-09-09T10:45:47Z

Question; this is only affects the API; the output of docker stats on the CLI is still the same, correct?

Yes.

HuKeping · 2015-09-11T10:15:43Z

rebased for conflict

jessfraz · 2015-09-11T20:17:30Z

needs rebase sorry

HuKeping · 2015-09-14T12:32:53Z

rebased @jfrazelle

For now docker stats will sum the rxbytes, txbytes, etc. of all the interfaces. It is OK for the output of CLI `docker stats` but not good for the API response, especially when the container is in sereval subnets. It's better to leave these origianl data to user. Signed-off-by: Hu Keping <hukeping@huawei.com>

HuKeping · 2015-09-16T01:39:34Z

ping @cpuguy83

cpuguy83 · 2015-09-16T14:32:29Z

LGTM, moving to docs review.

thaJeztah · 2015-09-16T17:19:19Z

docs re-LGTM

ping @SvenDowideit @moxiegirl

moxiegirl · 2015-09-16T18:26:48Z

@HuKeping thank you! LGTM

thaJeztah · 2015-09-16T18:29:08Z

All green, merging \o/

Refactor the statistics of network in docker stats

twhiteman · 2015-10-19T17:37:32Z

@HuKeping the API docs example shows a network JSON entry, it should be networks now right?

https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.21.md

{
     "read" : "2015-01-08T22:57:31.547920715Z",
     "network": {      <- should be "networks"
             "eth0": {
                 "rx_bytes": 5338,
                 "rx_dropped": 0,
                 ...

HuKeping · 2015-10-20T00:22:59Z

Yes it is @twhiteman , sorry for that I'll fix it.

This fixes a bug introduced in moby#15786: * if a pre-v1.20 client requested docker stats, the daemon would return both an API-compatible JSON blob *and* an API-incompatible JSON blob: see https://gist.github.com/donhcd/338a5b3681cd6a071629 Signed-off-by: Donald Huang <don.hcd@gmail.com>

This fixes a bug introduced in moby#15786: * if a pre-v1.20 client requested docker stats, the daemon would return both an API-compatible JSON blob *and* an API-incompatible JSON blob: see https://gist.github.com/donhcd/338a5b3681cd6a071629 Signed-off-by: Donald Huang <don.hcd@gmail.com> (cherry picked from commit d2c04f8) The commit title wrongfully mentioned API v1.22, when it meant to mention v1.21.

GordonTheTurtle added the status/0-triage label Aug 24, 2015

calavera added status/1-design-review and removed status/0-triage labels Aug 24, 2015

calavera added status/2-code-review and removed status/1-design-review labels Aug 24, 2015

calavera reviewed Aug 25, 2015
View reviewed changes

HuKeping mentioned this pull request Sep 6, 2015

Fix connection block when using docker stats API #14488

Merged

runcom reviewed Sep 7, 2015
View reviewed changes

thaJeztah added impact/api impact/changelog labels Sep 9, 2015

calavera added this to the 1.9.0 milestone Sep 9, 2015

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Sep 16, 2015

moxiegirl added status/4-merge and removed status/3-docs-review labels Sep 16, 2015

thaJeztah added a commit that referenced this pull request Sep 16, 2015

Merge pull request #15786 from HuKeping/stats-network

259a0fb

Refactor the statistics of network in docker stats

thaJeztah merged commit 259a0fb into moby:master Sep 16, 2015

HuKeping deleted the stats-network branch September 17, 2015 01:36

HuKeping mentioned this pull request Oct 20, 2015

Docs: update docs for API stats #17192

Merged

This was referenced Oct 28, 2015

API Changes in Docker 1.9 fsouza/go-dockerclient#407

Closed

API Changes in Docker 1.9 nginx-proxy/docker-gen#129

Open

donhcd mentioned this pull request Oct 30, 2015

fix pre-1.21 docker stats #17549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the statistics of network in docker stats #15786

Refactor the statistics of network in docker stats #15786

HuKeping commented Aug 24, 2015

HuKeping commented Aug 24, 2015

calavera commented Aug 24, 2015

calavera commented Aug 24, 2015

HuKeping commented Aug 24, 2015

calavera Aug 25, 2015

HuKeping Aug 25, 2015

calavera Aug 25, 2015

HuKeping Aug 27, 2015

thaJeztah commented Aug 30, 2015

HuKeping commented Aug 31, 2015

ehazlett commented Sep 4, 2015

HuKeping commented Sep 6, 2015

calavera commented Sep 7, 2015

runcom Sep 7, 2015

HuKeping Sep 8, 2015

jessfraz commented Sep 8, 2015

thaJeztah commented Sep 8, 2015

HuKeping commented Sep 9, 2015

thaJeztah commented Sep 9, 2015

HuKeping commented Sep 9, 2015

HuKeping commented Sep 11, 2015

jessfraz commented Sep 11, 2015

HuKeping commented Sep 14, 2015

HuKeping commented Sep 16, 2015

cpuguy83 commented Sep 16, 2015

thaJeztah commented Sep 16, 2015

moxiegirl commented Sep 16, 2015

thaJeztah commented Sep 16, 2015

twhiteman commented Oct 19, 2015

HuKeping commented Oct 20, 2015

Refactor the statistics of network in docker stats #15786

Refactor the statistics of network in docker stats #15786

Conversation

HuKeping commented Aug 24, 2015

HuKeping commented Aug 24, 2015

calavera commented Aug 24, 2015

calavera commented Aug 24, 2015

HuKeping commented Aug 24, 2015

calavera Aug 25, 2015

Choose a reason for hiding this comment

HuKeping Aug 25, 2015

Choose a reason for hiding this comment

calavera Aug 25, 2015

Choose a reason for hiding this comment

HuKeping Aug 27, 2015

Choose a reason for hiding this comment

thaJeztah commented Aug 30, 2015

HuKeping commented Aug 31, 2015

ehazlett commented Sep 4, 2015

HuKeping commented Sep 6, 2015

calavera commented Sep 7, 2015

runcom Sep 7, 2015

Choose a reason for hiding this comment

HuKeping Sep 8, 2015

Choose a reason for hiding this comment

jessfraz commented Sep 8, 2015

thaJeztah commented Sep 8, 2015

HuKeping commented Sep 9, 2015

thaJeztah commented Sep 9, 2015

HuKeping commented Sep 9, 2015

HuKeping commented Sep 11, 2015

jessfraz commented Sep 11, 2015

HuKeping commented Sep 14, 2015

HuKeping commented Sep 16, 2015

cpuguy83 commented Sep 16, 2015

thaJeztah commented Sep 16, 2015

moxiegirl commented Sep 16, 2015

thaJeztah commented Sep 16, 2015

twhiteman commented Oct 19, 2015

HuKeping commented Oct 20, 2015