Use counter for tracking container states by cpuguy83 · Pull Request #29554 · moby/moby

cpuguy83 · 2016-12-19T20:46:51Z

Container state counts are used for reporting in the /info endpoint.
Currently when /info is called, each container is iterated over and
the containers 'StateString()' is called. This is not very efficient
with lots of containers, and is also racey since StateString() is not
using a mutex and the mutex is not otherwise locked.

We could just lock the container mutex, but this is proven to be
problematic since there are frequent deadlock scenarios and we should
always have the /info endpoint available since this endpoint is used
to get general information about the docker host.

Closes #28912

dnephin

Generally design SGTM, but I do have a question about it

dnephin · 2016-12-19T21:11:50Z

I think you could do something like:

defer s.UpdateCounter()()

func (s *State) UpdateCounter() func() { oldState := s.StateString() return func() { s.counter.SetCount(oldState, s.StateString()) } }

dnephin · 2016-12-19T21:13:16Z

Isn't this still racy if the container changes state between oldState := s.StateString() and the defer running?

It would reduce the count of the old state twice?

Ah, you're right the SetCount needs to be done while the lock is held.

stevvooe · 2017-01-10T03:16:35Z

@cpuguy83 Is there any way we could leverage the prometheus metrics registry to aggregate this data?

AkihiroSuda · 2017-01-19T07:52:41Z

The lock-free proposal (#30225) can close this PR?

cpuguy83 · 2017-01-19T16:23:53Z

@AkihiroSuda I don't think so. Even with a lock-free container, it's still horrible to iterate every single container for docker info

stevvooe · 2017-01-19T22:57:05Z

@AkihiroSuda I agree with @cpuguy83 here. Aggregating the entire dataset will be much less reliable than pre-aggregating with counters.

@cpuguy83 Any luck getting go-metrics to do this? Let me know if you need pointers there.

LK4D4 · 2017-01-30T21:00:30Z

@cpuguy83 CI fails :/

cpuguy83 · 2017-02-10T14:34:04Z

@stevvooe I have wired in go-metrics here, although I did have to keep a separate counter for reporting to info as there doesn't really seem to be a way to query these metrics in a desirable way.

This still has some issue somewhere that so far I've only seen pop in tests where the counts get way off that I need to figure out.

stevvooe · 2017-02-14T00:00:02Z

as there doesn't really seem to be a way to query these metrics in a desirable way.

@cpuguy83 That is a wise decision!

@crosbymichael @juliusv Any ideas here?

juliusv · 2017-02-14T09:36:59Z

Any ideas here?

There's indeed no way to get the value of a single metric with Prometheus's Go client library, except for calling Write on it to dump it to its protobuf representation.

If you're going to maintain the count yourself anyways, you may want to think about not tracking the state twice, but using throw-away ConstMetrics every time you are scraped that just proxy your internal state to Prometheus. This is the common pattern for exporters which just translate existing values to Prometheus metrics. See for example https://github.com/prometheus/memcached_exporter/blob/master/main.go#L334.

vdemeester · 2017-03-01T14:15:07Z

ping @cpuguy83, what is the status here ? 👼

cpuguy83 · 2017-03-08T21:56:27Z

Working on it.

cpuguy83 · 2017-04-30T01:06:38Z

This is all green folks.

cpuguy83 · 2017-05-01T13:34:02Z

Something to consider b/c once metrics are out of experimental we probably ought not change this...

docker info looks at running, paused, and stopped. Where if it's not running or paused it's considered stopped (vs created, exited, etc).
Should we break these values down better for metrics?

dnephin

LGTM

What fixed the counter problems? Was it changing the counters to be per-container containers?

cpuguy83 · 2017-05-01T14:31:36Z

What fixed the counter problems?

Just started stashing the actual container state when it gets updated rather than calling inc+dec on split states.
Then we build the summary when info or the metrics endpoint is called.

stevvooe · 2017-05-01T20:01:59Z

LGTM after providing example /metrics output (in a gist or something).

@crosbymichael PTAL

cpuguy83 · 2017-05-01T20:26:18Z

Example of the metrics:

https://gist.github.com/cpuguy83/a04005ad22f03a46c5a0c95fe1d53d26

stevvooe · 2017-05-01T20:55:35Z

@cpuguy83 Wondering if we should make an effort to drop the engine_daemon_ prefixes... these seem redundant.

crosbymichael · 2017-05-01T20:58:17Z

LGTM

Its the best we can do without the ability to query prom metrics in process. , @cpuguy83 not saying your implementation is bad, just working with what we have :) It would be a lot less code if we could query the metric in process and get the values.

cpuguy83 · 2017-05-01T20:59:58Z

It would be a lot less code if we could query the metric in process and get the values

Can you explain what you mean by this?

crosbymichael · 2017-05-01T21:01:17Z

If we didn't have to query for the info endpoint, you just have a single metric with labels, and you inc/dec the id+state for each of the states, its like 10 lines of code.

cpuguy83 · 2017-05-01T21:04:26Z

@stevvooe Yeah, we could probably simplify the metrics namespace.
Maybe just docker_engine_* or just docker_*

stevvooe · 2017-05-01T21:08:11Z

@cpuguy83 Right now, it is engine_daemon_, which is a bit redundant. I am unsure about putting docker_ there, as well, since this is moby.

cpuguy83 · 2017-05-02T15:40:26Z

@stevvooe Do you have a suggestion? Just remove all-together?

stevvooe · 2017-05-02T18:16:48Z

@stevvooe Do you have a suggestion? Just remove all-together?

I think we can remove the prefix, but I'd like to hear @crosbymichael's opinion. We put work into "namespacing" metrics but prometheus already has a lot of context, like "job", "target" and info structs that can be used to create that context.

rogaha · 2017-05-03T07:41:01Z

LGTM

cpuguy83 · 2017-05-04T17:04:41Z

@stevvooe Are you ok if we merge this and talk about namespace separately?

stevvooe · 2017-05-05T20:09:04Z

LGTM

Let's open a bug to review the namespacing for metrics.

cpuguy83 · 2017-05-05T20:15:20Z

Opened #33056 to discuss.

Merging this one since it has all the required LGTM's and is all green.

GordonTheTurtle added the status/0-triage label Dec 19, 2016

cpuguy83 mentioned this pull request Dec 19, 2016

container/state.go: fix race in StateString() #28912

Closed

cpuguy83 force-pushed the keep_running_count_of_states branch from 395cc44 to 17d9b1a Compare December 19, 2016 21:04

dnephin reviewed Dec 19, 2016

View reviewed changes

cpuguy83 force-pushed the keep_running_count_of_states branch 2 times, most recently from aff5310 to 31c805d Compare December 19, 2016 21:49

LK4D4 added status/2-code-review and removed status/0-triage labels Dec 19, 2016

AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 20, 2016

cpuguy83 force-pushed the keep_running_count_of_states branch 2 times, most recently from 6ed6402 to 4678a2d Compare December 22, 2016 21:11

cpuguy83 force-pushed the keep_running_count_of_states branch 2 times, most recently from 3877dee to e25bc83 Compare February 10, 2017 03:12

GordonTheTurtle assigned vdemeester Feb 10, 2017

stevvooe closed this Feb 14, 2017

stevvooe reopened this Feb 14, 2017

cpuguy83 force-pushed the keep_running_count_of_states branch from e25bc83 to 9b1a79e Compare March 8, 2017 21:46

cpuguy83 force-pushed the keep_running_count_of_states branch 3 times, most recently from 60b6e01 to fd78c07 Compare March 9, 2017 03:22

cpuguy83 removed status/failing-ci Indicates that the PR in its current state fails the test suite rebuild/powerpc labels Apr 30, 2017

dnephin added the rebuild/powerpc label May 1, 2017

GordonTheTurtle removed the rebuild/powerpc label May 1, 2017

dnephin approved these changes May 1, 2017

View reviewed changes

alfred-landrum mentioned this pull request May 2, 2017

Add container state count metrics #32976

Closed

cpuguy83 added this to the 17.06.0 milestone May 4, 2017

cpuguy83 mentioned this pull request May 5, 2017

Metrics namespacing #33056

Closed

cpuguy83 merged commit 669f4ba into moby:master May 5, 2017

cpuguy83 deleted the keep_running_count_of_states branch May 5, 2017 20:15

kolyshkin mentioned this pull request May 1, 2020

backport: No container locks on docker ps projectatomic/docker#375

Merged

thaJeztah mentioned this pull request Feb 15, 2022

daemon: use RWMutex for stateCounter #43245

Merged

thaJeztah mentioned this pull request Mar 25, 2022

[20.10 backport] daemon: use RWMutex for stateCounter #43426

Merged

Uh oh!

Conversation

cpuguy83 commented Dec 19, 2016

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Jan 10, 2017

Uh oh!

AkihiroSuda commented Jan 19, 2017

Uh oh!

cpuguy83 commented Jan 19, 2017

Uh oh!

stevvooe commented Jan 19, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

cpuguy83 commented Feb 10, 2017

Uh oh!

stevvooe commented Feb 14, 2017

Uh oh!

juliusv commented Feb 14, 2017

Uh oh!

vdemeester commented Mar 1, 2017

Uh oh!

cpuguy83 commented Mar 8, 2017

Uh oh!

cpuguy83 commented Apr 30, 2017

Uh oh!

cpuguy83 commented May 1, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 1, 2017

Uh oh!

stevvooe commented May 1, 2017

Uh oh!

cpuguy83 commented May 1, 2017

Uh oh!

stevvooe commented May 1, 2017

Uh oh!

crosbymichael commented May 1, 2017

Uh oh!

cpuguy83 commented May 1, 2017

Uh oh!

crosbymichael commented May 1, 2017

Uh oh!

cpuguy83 commented May 1, 2017

Uh oh!

stevvooe commented May 1, 2017

Uh oh!

cpuguy83 commented May 2, 2017

Uh oh!

stevvooe commented May 2, 2017

Uh oh!

rogaha commented May 3, 2017

Uh oh!

cpuguy83 commented May 4, 2017

Uh oh!

stevvooe commented May 5, 2017

Uh oh!

cpuguy83 commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!