docker daemon ContainerStop: dont wait forever if docker kill fails#41580
docker daemon ContainerStop: dont wait forever if docker kill fails#41580sparrc wants to merge 1 commit into
Conversation
e134d6d to
3bedf15
Compare
|
@cpuguy83 @tonistiigi ptal |
|
jenkins failures look like only s390x integration tests related to pinging so I dont think they're related to this change. |
b03359c to
1e31af2
Compare
|
|
There was a problem hiding this comment.
I wonder if this wait is even needed. What's the reasoning behind waiting indefinitely if Kill returns an error?
It seems this code assumes that there will be an containerpkg.WaitConditionNotRunning even if Kill fails, but that doesn't seem to be true in all cases (if at all), which is why this function waits for ever, as described in #41579
I think we should just return the error here
Thoughts?
There was a problem hiding this comment.
I think it's worth waiting.
Also, FWIW, I think the potential for deadlock here is small.
There were some bugs related to error handling from runc as well as event processing from containerd that would trigger a problem here, but those are cleaned up.
That said, totally fine to not wait for ever.
The assumption to wait forever has likely been passed down from old versions where docker directly managed the container process, where as now that has containerd in the middle.
There was a problem hiding this comment.
I'm also not sure about the usefulness of waiting, @cpuguy83 would you be opposed to at least reducing the timeout to 2 seconds like the code does in a few other places?
The downside of waiting at all is that the containerStop function can already take quite a lot longer to return than the seconds input argument, and waiting here makes that worse.
There was a problem hiding this comment.
This error is not really accurate, because SIGKILL really means that the kernel is terminating the process.
What is happening is dockerd is not receiving an exit event, for whatever reason, from containerd, maybe we should reflect that in the error message.
Also note, error messages should start lowercase for code style. Log messages with capital is fine.
There was a problem hiding this comment.
okay, how about "tried to kill container, but did not receive an exit event" ?
docker daemon: dont wait without timeout in docker kill fixes moby#41579 Signed-off-by: Cam <[email protected]>
1e31af2 to
216be72
Compare
|
changes pushed addressing some comments, others still pending feedback |
|
closing this in favor of #41586, as I found a few other issues in stop container that I thought were worth addressing with a larger refactor. |
4 participants
fixed #41579
- What I did
remove waiting without a timeout when docker kill fails within docker stop
before change:
cli:
after change:
cli:
docker logs:
- Description for the changelog
Fix hanging docker stop if SIGTERM and SIGKILL both fail
- A picture of a cute animal (not mandatory but encouraged)