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

fix when rpc reports "transport is closing" error, health check go routine will exit #32986

Merged
merged 1 commit into from May 18, 2017

Conversation

@moypray
Copy link
Contributor

commented May 3, 2017

- What I did

Fix a bug:
Description:
Kill docker-containerd continuously, and use kill -SIGUSR1 to check docker callstacks. And we will find that event handler: handleConnectionChange exit.

I wrote a script: each 10 seconds I killed containerd, and run about 15 minutes, this condition appears.

looks like PR:
#32590

- How I did it

Analysis:
When grpc connection is fall, the error maybe like this:

2017-05-03T14:18:30.163173+00:00|info|docker[-]|time="2017-05-03T14:18:30.161475751Z" level=debug msg="libcontainerd: containerd health check returned error: rpc error: code = 14 desc = grpc: the connection is unavailable"
2017-05-03T14:18:38.185628+00:00|info|docker[-]|time="2017-05-03T14:18:38.185044385Z" level=debug msg="libcontainerd: containerd health check returned error: rpc error: code = 13 desc = transport is closing"

And the second error will make the go routine handleConnectionChange returned.

I think we should use r.closeManually not is closing to determine if it should return or not.

- How to verify it

use the script, run a long time, the go routine will not appear.

- Description for the changelog

Signed-off-by: Wentao Zhang zhangwentao234@huawei.com

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

fix when rpc reports "transport is closing" error, health check go ro…
…utine will exit

Signed-off-by: Wentao Zhang <zhangwentao234@huawei.com>
@thaJeztah

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@mlaventure
Copy link
Contributor

left a comment

LGTM

With this, #32590 can be closed.

Can you double check that it also fixes the issue for you?

@thaJeztah thaJeztah added this to the 17.06.0 milestone May 15, 2017

@tonistiigi
Copy link
Member

left a comment

LGTM

@tonistiigi tonistiigi merged commit e103125 into moby:master May 18, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 33580 has succeeded
Details
janky Jenkins build Docker-PRs 42185 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 2510 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13387 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2264 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.