docker kill: fix bug where failed kills didnt fallback to unix kill by sparrc · Pull Request #41588 · moby/moby

sparrc · 2020-10-24T21:02:25Z

fixes docker kill does not execute "direct kill" if initial sigkill fails #41587
removes potential infinite Wait and goroutine leak at end of kill
function
refactored container state waiting out of container_operations_unix.go
and into kill.go for consistency

before change

docker kill would just return error if the first sigkill function failed:

% docker kill kt
Error response from daemon: Cannot kill container: kt: foo error

after change

docker falls back to unix kill and container exits:

% docker kill kt
kt

docker logs:

Oct 24 20:54:08 ip-10-0-0-157.us-west-2.compute.internal dockerd[13955]: time="2020-10-24T20:54:08.239338811Z" level=error msg="Container failed to exit within 10 seconds of kill - trying direct SIGKILL" container=f1df7d8bafba1d39124ff7c1edf32c9e1527e318f4df4332ffbcba537c5fc798 error="context deadline exceeded"
Oct 24 20:54:08 ip-10-0-0-157.us-west-2.compute.internal dockerd[13955]: time="2020-10-24T20:54:08.300068287Z" level=info msg="ignoring event" module=libcontainerd namespace=moby topic=/tasks/delete type="*events.TaskDelete"

- Description for the changelog

fix bug where docker kill would not properly fallback to unix sigkill

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

sparrc · 2020-10-27T01:27:10Z

appears to be only the s390x integration tests failing again

sparrc · 2021-02-16T23:39:20Z

hello, we are continuing to see this issue in ECS, any chance we could get some eyes on this fix?

zuzzas · 2021-02-19T09:04:22Z

Can an issue describe here manifest if a PID does not exist? We've hit the exact same problem, but there was not a single process in the crontainer even present on the system. I can see guarding ifs, but seems like there is a problem with the logic.

Server: Docker Engine - Community
 Engine:
  Version:          18.09.7
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.10.8
  Git commit:       2d0083d
  Built:            Thu Jun 27 17:23:02 2019
  OS/Arch:          linux/amd64
  Experimental:     false

samuelkarp

I think the code mostly looks good, I'm just not quite clear on one of the changes that was made.

samuelkarp · 2021-02-25T01:09:07Z

~~It looks like this is moving from one magic number to another, but...why 10 seconds here?~~

Is the goal to move the 10s wait from killProcessDirectly to here? Can you tell me a bit more about why you wanted to make that change?

yes you are correct about the goal. The reason that was done was just for consistency. If you look across the functions in kill.go there are 3 other calls to container.Wait.

In container_operations_unix.go there were no other calls to container.Wait except here in killProcessDirectly.

It seemed better to keep the waiting functions contained to kill.go just for consistency rather than have a call to container.Wait at two different levels on the call stack.

mmerkes · 2021-03-17T20:41:00Z

@sparrc @samuelkarp Along with this change, would love to see this get merged.

1. fixes moby#41587 2. removes potential infinite Wait and goroutine leak at end of kill function fixes moby#41587 Signed-off-by: Cam <[email protected]>

sparrc · 2021-04-14T22:46:17Z

thanks so much @tianon and @cpuguy83 for reviewing and merging #41586 !

Could I ask that you please take a look at this PR too while it's fresh? It is highly related and also a situation that we regularly see customers hitting at ECS.

sparrc · 2021-04-20T18:09:44Z

Hello, just wanted to ping to see if this PR could be reviewed, thank you!

cpuguy83

This looks like it makes the existing intent of the code more robust so that at least looks good.

However it seems like if we are getting to the point where we have to call killProcessDirectly, we should instead be calling task.Delete in containerd instead which will (should) ensure that certain sticky situations are cleaned up (like a blocked runc-init) and work more correctly for other runtimes.

@kolyshkin, thoughts?

cpuguy83 · 2021-04-20T18:34:26Z

+			return err
+		}
+		if isZombie {
+			return errdefs.System(errors.Errorf("container %s PID %d is zombie and can not be killed. Use the --init option when creating containers to run an init inside the container that forwards signals and reaps processes", stringid.TruncateID(container.ID), pid))


side note: I don't think this error is accurate since the container PID would itself only be a zombie if the shim exited early for whatever reason.

sparrc · 2021-04-20T20:02:43Z

@cpuguy83 would it be possible to consider that question in a follow-up PR? I think it's worth breaking that out into two changes. Reason being that this PR mostly retains the same behavior as before while removing the potential for the docker kill function to hang forever.

Changing from a unix kill -9 to task.Delete seems like a larger conceptual change to me that maybe shouldn't be bundled in with this refactor?

cpuguy83 · 2021-04-20T20:45:53Z

@sparrc IMO, if we are going to introduce a behavior change here (which has been mostly static for awhile), I'd prefer to do it once.

sparrc · 2021-04-22T23:01:57Z

This looks like it makes the existing intent of the code more robust so that at least looks good.

However it seems like if we are getting to the point where we have to call killProcessDirectly, we should instead be calling task.Delete in containerd instead which will (should) ensure that certain sticky situations are cleaned up (like a blocked runc-init) and work more correctly for other runtimes.

@kolyshkin, thoughts?

OK, just to clarify what you mean by task.Delete, is your suggestion that in kill.go instead of falling back to killProcessDirectly (ie here on master), we instead call out to daemon.containerd.DeleteTask() ? Or would containerd.Delete() be better? (Im not sure exactly the distinction between these).

If that's the case then I guess we could just delete killProcessDirectly entirely, as it wouldn't be used anywhere anymore.

kzys · 2021-04-22T23:57:12Z

containerd's task deletion is doing some checks and asks clients to stop the task beforehand.

If your container is still running;

% sudo ctr image pull docker.io/library/debian:latest
...                             
unpacking linux/amd64 sha256:ba4a437377a0c450ac9bb634c3754a17b1f814ce6fa3157c0dc9eef431b29d1f...
done
% sudo ctr run --rm --tty docker.io/library/debian:latest debian
root@ip-172-31-25-68:/#

Deletion wouldn't work.

% sudo ctr t rm debian
ERRO[0000] unable to delete debian                       error="task must be stopped before deletion: running: failed precondition"
ctr: task must be stopped before deletion: running: failed precondition
%

Falling back to containerd-level task deletion may introduce more error cases, like what if containerd is unhealthy. Using task.Kill() is possible but containerd's health is still a concern.

Since this is a fallback case, sending SIGKILL seems much robust to me.

sparrc · 2021-04-28T17:30:07Z

@cpuguy83 @kolyshkin thoughts?

sparrc · 2021-05-11T16:34:36Z

@cpuguy83 @kolyshkin thoughts on changing this pid kill -9 to containerd task delete? I asked @kzys to take a look since he is a coworker of mine who is a containerd maintainer, in case you were wondering the context of his comment above.

cpuguy83

LGTM

cpuguy83 · 2021-05-11T18:33:24Z

+	}
+
+	// In case there were some exceptions(e.g., state of zombie and D)
+	if system.IsProcessAlive(pid) {


This seems redundant/not needed with the zombie check.

sparrc · 2021-05-13T17:25:01Z

@tianon could you take a look at this? it is similar in concept to #41586 but applies to docker kill instead of docker stop

sparrc · 2021-05-20T00:20:30Z

very keen to get this merged if anyone from docker has time to take a look, happy to walk through the changes if it would help, you can find me in the Gophers and Docker Community slack channels.

samuelkarp

LGTM

tianon

LGTM, although I would love (as I've noted in #41587) if we could find a way to reproduce so we can more definitively answer @cpuguy83's questions about whether the fallback block is necessary. 😅

thaJeztah · 2021-05-21T08:53:41Z

@tonistiigi interesting error on s390x; https://ci-next.docker.com/public/blue/rest/organizations/jenkins/pipelines/moby/branches/PR-41588/runs/6/nodes/181/log/?start=0

[2021-05-20T18:45:51.977Z] + docker build --force-rm --build-arg APT_MIRROR -t docker:593c9f3edde77558d171fd029ef9773426204ef3 .
[2021-05-20T18:45:51.977Z] #1 [internal] load .dockerignore
[2021-05-20T18:45:51.977Z] #1 ERROR: proto: CacheRecords: wiretype end group for non-group
[2021-05-20T18:45:51.977Z] 
[2021-05-20T18:45:51.977Z] #2 [internal] load build definition from Dockerfile
[2021-05-20T18:45:51.977Z] #2 transferring dockerfile: 17.07kB done
[2021-05-20T18:45:51.977Z] #2 DONE 0.0s
[2021-05-20T18:45:51.977Z] ------
[2021-05-20T18:45:51.977Z]  > [internal] load .dockerignore:
[2021-05-20T18:45:51.977Z] ------
[2021-05-20T18:45:51.977Z] failed to solve with frontend dockerfile.v0: failed to build LLB: proto: CacheRecords: wiretype end group for non-group
script returned exit code 1

sparrc · 2021-05-21T18:36:21Z

@thaJeztah could that be related to this change or do you think it's a flakey failure?

thaJeztah · 2021-05-21T18:38:59Z

Definitely not related, but hadn't seen that failure before (we were discussing some breaking changes in gRPC recently though, and this felt possibly related)

Let me check CI and restart

thaJeztah

LGTM

thaJeztah · 2021-05-25T12:43:49Z

+func killProcessDirectly(container *container.Container) error {
+	pid := container.GetPID()
+	// Ensure that we don't kill ourselves
+	if pid == 0 {


Not for this PR, but we should consider making this

Suggested change

if pid == 0 {

if pid <= 0 {

I recall a situation where -1 was returned (by containerd?) because it was initialised with -1, and killing -1 does "fun things" 😅

angelcar reviewed Oct 26, 2020

View reviewed changes

Comment thread daemon/kill.go Outdated

angelcar approved these changes Oct 26, 2020

View reviewed changes

sparrc force-pushed the kill-refactor branch from ab7c38a to 550ae46 Compare October 27, 2020 01:36

sparrc mentioned this pull request Oct 28, 2020

Fix hung docker stop if stop signal and daemon.Kill both fail #41586

Merged

sparrc force-pushed the kill-refactor branch from 550ae46 to 198d342 Compare February 20, 2021 00:42

sparrc mentioned this pull request Feb 23, 2021

ECS Agent was not able to stop the container aws/amazon-ecs-agent#2808

Closed

samuelkarp reviewed Feb 25, 2021

View reviewed changes

docker kill: fix bug where failed kills didnt fallback to unix kill

e57a365

1. fixes moby#41587 2. removes potential infinite Wait and goroutine leak at end of kill function fixes moby#41587 Signed-off-by: Cam <[email protected]>

sparrc force-pushed the kill-refactor branch from 198d342 to e57a365 Compare April 14, 2021 22:43

cpuguy83 reviewed Apr 20, 2021

View reviewed changes

cpuguy83 approved these changes May 12, 2021

View reviewed changes

angelcar mentioned this pull request May 20, 2021

Give up trying to stop container if max timeouts are reached aws/amazon-ecs-agent#2874

Merged

samuelkarp approved these changes May 20, 2021

View reviewed changes

fenxiong mentioned this pull request May 20, 2021

V1.52.2 stage aws/amazon-ecs-agent#2875

Merged

tianon mentioned this pull request May 21, 2021

docker kill does not execute "direct kill" if initial sigkill fails #41587

Closed

tianon approved these changes May 21, 2021

View reviewed changes

thaJeztah approved these changes May 25, 2021

View reviewed changes

thaJeztah merged commit 97303df into moby:master May 25, 2021

thaJeztah added impact/changelog status/2-code-review labels May 25, 2021

thaJeztah added this to the 21.xx milestone May 25, 2021

thaJeztah added the kind/bugfix PR's that fix bugs label May 25, 2021

sparrc mentioned this pull request Jun 8, 2021

Fix log statement 'failed to exit' timeout accuracy #42488

Merged

thaJeztah mentioned this pull request Oct 21, 2021

[20.10] Backport fixes for kill/stop handling #42956

Merged

thaJeztah added the process/cherry-picked label Oct 21, 2021

vaibhavhd mentioned this pull request May 17, 2022

[202012] Update dockerd and containerd versions sonic-net/sonic-buildimage#10856

Draft

6 tasks

Uh oh!

Conversation

sparrc commented Oct 24, 2020 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sparrc commented Oct 27, 2020

Uh oh!

sparrc commented Feb 16, 2021

Uh oh!

zuzzas commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

sparrc Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmerkes commented Mar 17, 2021

Uh oh!

sparrc commented Apr 14, 2021

Uh oh!

sparrc commented Apr 20, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

sparrc commented Apr 20, 2021

Uh oh!

cpuguy83 commented Apr 20, 2021

Uh oh!

sparrc commented Apr 22, 2021

Uh oh!

kzys commented Apr 22, 2021

Uh oh!

sparrc commented Apr 28, 2021

Uh oh!

sparrc commented May 11, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 May 11, 2021

Choose a reason for hiding this comment

Uh oh!

sparrc commented May 13, 2021

Uh oh!

sparrc commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 21, 2021

Uh oh!

sparrc commented May 21, 2021

Uh oh!

thaJeztah commented May 21, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 25, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

sparrc commented Oct 24, 2020 •

edited by thaJeztah

Loading

zuzzas commented Feb 19, 2021 •

edited

Loading

sparrc Mar 8, 2021 •

edited

Loading

sparrc commented May 20, 2021 •

edited

Loading