Return an empty stats if "container not found" #34004

yummypeng · 2017-07-07T08:06:43Z

If we get "container not found" error from containerd, it's possibly
because that this container has already been stopped. It will be ok to
ignore this error and just return an empty stats.

Steps to reproduce:

# docker run -tid --name tmp --restart=always ubuntu true && docker stats --no-stream
48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c


(hangs...)



^C
#

It's a duplicate of #27772 which has already been closed by #28026, but #28026 is not a complete fix because if docker stats --no-stream comes faster after docker run, the container is still running when the code hits https://github.com/moby/moby/blob/master/daemon/stats.go#L35. Then I can see a lot of error logs from daemon:

time="2017-07-07T14:54:41.210577988+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:42.210686973+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:43.210721305+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:44.210637074+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:45.210742374+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:46.210928003+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:47.210899239+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:48.210914277+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:49.210748399+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:50.210818936+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"
time="2017-07-07T14:54:51.210596037+08:00" level=error msg="collecting stats for 48d3344f404a2b4a28538af857dedab5bca7f3939667f51509c61dd9f150bf5c: rpc error: code = Unknown desc = containerd: container not found"

This pr will ignore this error and just return an empty stats. So that client will not hang forever.

after this pr:

# docker run -tid --name tmp --restart=always ubuntu true && docker stats --no-stream
0c93066033730361fb296af0e3b07e4b264169535353aecd9121159b81dfa101
CONTAINER           CPU %               MEM USAGE / LIMIT   MEM %               NET I/O             BLOCK I/O           PIDS

Signed-off-by: Yuanhong Peng [email protected]

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

yummypeng · 2017-07-07T08:28:42Z

Oops, I close this pr by mistake and CI stopped @thaJeztah

thaJeztah · 2017-07-07T22:12:03Z

Looks like CI started again, and is green now 👍

thaJeztah

Left some suggestions :)

thaJeztah · 2017-07-07T22:13:17Z

daemon/stats/collector.go

+				// if we get "container not found" error from containerd, it's possibly
+				// because that this container has already been stopped. it will be ok to
+				// ignore this error and just return an empty stats.
+				if _, ok := err.(notRunningErr); !ok && !strings.Contains(err.Error(), "container not found") {


Could we make the container not found error a typed error (like we do for notRunningErr)?

Not introduced in this PR, but while looking at this code:
The double negative is a bit confusing here as well; would this be more readable (assuming a typed error for container not found was implemented;?

for _, pair := range pairs { stats, err := s.supervisor.GetContainerStats(pair.container) switch err.(type) { case nil: // FIXME: move to containerd on Linux (not Windows) stats.CPUStats.SystemUsage = systemUsage stats.CPUStats.OnlineCPUs = onlineCPUs pair.publisher.Publish(*stats) case notRunningErr, notFoundErr: // publish empty stats containing only name and ID if not running or not found pair.publisher.Publish(types.StatsJSON{ Name: pair.container.Name, ID: pair.container.ID, }) default: logrus.Errorf("collecting stats for %s: %v", pair.container.ID, err) } }

@thaJeztah Thanks for your suggestion ❤️ I have updated the codes.

thaJeztah

left one more comment

also /cc @cpuguy83 for review

thaJeztah · 2017-07-10T02:52:30Z

daemon/daemon_unix.go

@@ -1165,6 +1165,9 @@ func (daemon *Daemon) stats(c *container.Container) (*types.StatsJSON, error) {
 	}
 	stats, err := daemon.containerd.Stats(c.ID)
 	if err != nil {
+		if strings.Contains(err.Error(), "container not found") {


I guess this also needs to be done in the _windows version;

moby/daemon/daemon_windows.go

Lines 525 to 529 in 9d87e6e

// Obtain the stats from HCS via libcontainerd

stats, err := daemon.containerd.Stats(c.ID)

if err != nil {

return nil, err

}

Updated 😉 @thaJeztah

thaJeztah · 2017-07-10T04:14:48Z

daemon/stats/collector.go

+
+type notFoundErr interface {
+	error
+}


Hm, looking at this again; this interface is not specific enough, as both daemon. errNotRunning, and daemon. errNotFound satisfy this interface; see https://play.golang.org/p/HtkBu1_QuM. If you make it more specific, it works though; https://play.golang.org/p/S7DzAuRITj

@thaJeztah Tons of thanks for finding this ❤️

@yummypeng you're welcome! I almost missed it as well ☺️

If we get "container not found" error from containerd, it's possibly because that this container has already been stopped. It will be ok to ignore this error and just return an empty stats. Signed-off-by: Yuanhong Peng <[email protected]>

yummypeng · 2017-07-10T08:35:45Z

janky failed but it seems not related to my PR:

07:41:22 ----------------------------------------------------------------------
07:41:22 FAIL: docker_cli_swarm_test.go:1146: DockerSwarmSuite.TestSwarmLockUnlockCluster
07:41:22 
07:41:22 [d4ec576a08906] waiting for daemon to start
07:41:22 [d4ec576a08906] daemon started
07:41:22 
07:41:22 [dacb4fc0da4ef] waiting for daemon to start
07:41:22 [dacb4fc0da4ef] daemon started
07:41:22 
07:41:22 [d0e91bd982f1a] waiting for daemon to start
07:41:22 [d0e91bd982f1a] daemon started
07:41:22 
07:41:22 [dacb4fc0da4ef] exiting daemon
07:41:22 [dacb4fc0da4ef] waiting for daemon to start
07:41:22 [dacb4fc0da4ef] daemon started
07:41:22 
07:41:22 [dacb4fc0da4ef] exiting daemon
07:41:22 [d4ec576a08906] exiting daemon
07:41:22 [d4ec576a08906] waiting for daemon to start
07:41:22 [d4ec576a08906] daemon started
07:41:22 
07:41:22 [d0e91bd982f1a] exiting daemon
07:41:22 [d0e91bd982f1a] waiting for daemon to start
07:41:22 [d0e91bd982f1a] daemon started
07:41:22 
07:41:22 [dacb4fc0da4ef] waiting for daemon to start
07:41:22 [dacb4fc0da4ef] daemon started
07:41:22 
07:41:22 docker_cli_swarm_test.go:1192:
07:41:22     // d2 is now set to lock
07:41:22     checkSwarmUnlockedToLocked(c, d2)
07:41:22 docker_utils_test.go:471:
07:41:22     c.Assert(v, checker, args...)
07:41:22 ... obtained bool = false
07:41:22 ... expected bool = true
07:41:22 
07:41:22 [d4ec576a08906] exiting daemon
07:41:22 [dacb4fc0da4ef] exiting daemon
07:41:22 [d0e91bd982f1a] exiting daemon
07:41:35 
07:41:35 ----------------------------------------------------------------------

yummypeng · 2017-07-11T02:34:28Z

ping @thaJeztah @cpuguy83

thaJeztah

LGTM

vieux · 2017-07-11T10:05:30Z

LGTM ping @tiborvass

vieux · 2017-07-11T10:05:42Z

ping @kolyshkin

tiborvass · 2017-07-12T03:10:10Z

LGTM

If we get "container not found" error from containerd, it's possibly because that this container has already been stopped. It will be ok to ignore this error and just return an empty stats. fix DTS2017062812251 related upstream PR: moby#34004 Signed-off-by: Yuanhong Peng <[email protected]>

Return an empty stats if "container not found" If we get "container not found" error from containerd, it's possibly because that this container has already been stopped. It will be ok to ignore this error and just return an empty stats. fix DTS2017062812251 related upstream PR: moby#34004 Signed-off-by: Yuanhong Peng <[email protected]> See merge request docker/docker!666

GordonTheTurtle added the status/0-triage label Jul 7, 2017

yummypeng closed this Jul 7, 2017

yummypeng reopened this Jul 7, 2017

yummypeng force-pushed the fix-docker-stats-hang branch from 4c77b79 to c33a3f4 Compare July 7, 2017 08:27

thaJeztah added status/2-code-review and removed status/0-triage labels Jul 7, 2017

thaJeztah requested changes Jul 7, 2017

View reviewed changes

yummypeng force-pushed the fix-docker-stats-hang branch from c33a3f4 to 911c002 Compare July 10, 2017 02:07

thaJeztah requested changes Jul 10, 2017

View reviewed changes

yummypeng force-pushed the fix-docker-stats-hang branch from 911c002 to dc8b314 Compare July 10, 2017 03:04

thaJeztah requested changes Jul 10, 2017

View reviewed changes

yummypeng force-pushed the fix-docker-stats-hang branch from dc8b314 to c567391 Compare July 10, 2017 06:18

Return an empty stats if "container not found"

4a6cbf9

If we get "container not found" error from containerd, it's possibly because that this container has already been stopped. It will be ok to ignore this error and just return an empty stats. Signed-off-by: Yuanhong Peng <[email protected]>

yummypeng force-pushed the fix-docker-stats-hang branch from c567391 to 4a6cbf9 Compare July 10, 2017 08:31

thaJeztah approved these changes Jul 11, 2017

View reviewed changes

thaJeztah mentioned this pull request Jul 11, 2017

Flaky test: DockerSwarmSuite.TestSwarmLockUnlockCluster #34051

Open

tiborvass merged commit c8a2596 into moby:master Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return an empty stats if "container not found" #34004

Return an empty stats if "container not found" #34004

yummypeng commented Jul 7, 2017 •

edited

Loading

yummypeng commented Jul 7, 2017

thaJeztah commented Jul 7, 2017

thaJeztah left a comment

thaJeztah Jul 7, 2017

thaJeztah Jul 7, 2017

yummypeng Jul 10, 2017

thaJeztah left a comment

thaJeztah Jul 10, 2017

yummypeng Jul 10, 2017

thaJeztah Jul 10, 2017

yummypeng Jul 10, 2017

thaJeztah Jul 11, 2017

yummypeng commented Jul 10, 2017

yummypeng commented Jul 11, 2017

thaJeztah left a comment

vieux commented Jul 11, 2017

vieux commented Jul 11, 2017

tiborvass commented Jul 12, 2017

	// Obtain the stats from HCS via libcontainerd
	stats, err := daemon.containerd.Stats(c.ID)
	if err != nil {
	return nil, err
	}

Return an empty stats if "container not found" #34004

Return an empty stats if "container not found" #34004

Conversation

yummypeng commented Jul 7, 2017 • edited Loading

yummypeng commented Jul 7, 2017

thaJeztah commented Jul 7, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yummypeng commented Jul 10, 2017

yummypeng commented Jul 11, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

vieux commented Jul 11, 2017

vieux commented Jul 11, 2017

tiborvass commented Jul 12, 2017

yummypeng commented Jul 7, 2017 •

edited

Loading