docker kill should return error if container is not running. #12371

rhatdan · 2015-04-14T14:41:42Z

Assuming that docker kill is trying to actually kill the container is a mistake. If the container is not running we should report it back to the caller as a error.

Docker-DCO-1.1-Signed-off-by: Regan McCooey rmccooey27@aol.com (github: rmccooey27)
Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

runcom · 2015-04-14T14:46:04Z

I think is a dup #12085

runcom · 2015-04-14T14:47:58Z

Also #12085 (comment)

rhatdan · 2015-04-14T15:07:32Z

We currently are returning errors from kill, so I don't agree with this statement. We can steal the unit test code from the other patch.

docker kill --signal 30 MISSINGCONTAINER

Currently returns an error.

This change just adds

docker kill --signal 30 NOTRUNNINGCONTAINER

The other pull request returned error on SIGINT of a non running container, which I left as not an error,
since this is similar to what we do with docker stop, I believe, But by sending a non SIGINT SIGTERM to a container, the user would expect the signal to be handled by the container, and if the container is not running, should generate an error.

rhatdan · 2015-04-14T15:08:03Z

This fix is in response to a bugzilla

https://bugzilla.redhat.com/show_bug.cgi?id=1209537

icecrime · 2015-04-15T00:29:25Z

api/server/server.go

@@ -203,7 +203,7 @@ func postContainersKill(eng *engine.Engine, version version.Version, w http.Resp
 	name := vars["name"]

 	// If we have a signal, look at it. Otherwise, do nothing
-	if sigStr := vars["signal"]; sigStr != "" {
+	if sigStr := r.Form.Get("signal"); sigStr != "" {


Sorry, I don't understand how this change relates to the PR? It seems it should never have been fetched from vars, but then how does kill even works today, and why is there no test to catch this?

I think this is a real bug becuase signal comes from the GET params and not any URL segment

Correct, when testing the fix, a patch came in that blew up our fix. So I included it in the pull request.

jessfraz · 2015-04-22T23:01:08Z

LGTM

duglin · 2015-04-22T23:40:56Z

can we get some testcases for this?

rhatdan · 2015-04-23T12:05:23Z

Added a test.

duglin · 2015-04-23T12:17:03Z

@rhatdan I don't see the test, did you forget to "git add" it?

rhatdan · 2015-04-23T14:29:29Z

@duglin oops

cpuguy83 · 2015-04-23T15:00:49Z

integration-cli/docker_cli_kill_test.go

@@ -26,6 +26,11 @@ func (s *DockerSuite) TestKillContainer(c *check.C) {
 		c.Fatalf("failed to kill container: %s, %v", out, err)
 	}

+	/* Container should be stopped, second kill should return an error */
+	if _, _, err = runCommandWithOutput(killCmd); err == nil {


This will always error because you can't run the same cmd twice.

it might be better to create a new test where you:

create a container

stop it

try to kill it and then check for your new error message.
I think its important to make sure that we run, and verify, that the code you added is hit.

rhatdan · 2015-04-23T15:14:59Z

Ok added a new function.

duglin · 2015-04-23T15:22:37Z

integration-cli/docker_cli_kill_test.go

+
+	killCmd := exec.Command(dockerBinary, "kill", cleanedContainerID)
+	/* Container should be stopped, second kill should return an error */
+	if _, _, err = runCommandWithOutput(killCmd); err == nil {


Can you check to make sure your text is displayed? Otherwise we can't be sure we're hitting your code - it could have failed for some other reason by mistake.

I don't think you addressed this comment yet

cpuguy83 · 2015-04-23T15:40:44Z

Error: kill succeeded on a stopped container

rhatdan · 2015-04-23T18:26:57Z

Ok this is broken, because we are only returning error on kill signal not on kill -9. I have fixed the test.
But it does bring up a question, should we be returning an error if a user tries to kill -9 a dead container.

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

duglin · 2015-05-07T13:35:17Z

@rhatdan any chance of you addressing #12371 (comment) ?

rhatdan · 2015-05-11T19:48:51Z

@duglin I think the current pull request fails on all kill signals.

cpuguy83 · 2015-05-22T00:47:02Z

ping @rhatdan any update here?

rhatdan · 2015-05-27T01:25:10Z

@cpuguy83 I think it is all set? I have it returning errors for both kinds of kills.

cpuguy83 · 2015-05-27T12:59:40Z

integration-cli/docker_cli_kill_test.go

+		c.Fatalf("kill succeeded on a stopped container")
+	}
+
+	deleteContainer(cleanedContainerID)


No need for this delete here.

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

duglin · 2015-05-27T15:10:42Z

integration-cli/docker_cli_kill_test.go

+
+	killCmd := exec.Command(dockerBinary, "kill", "-s", "30", cleanedContainerID)
+	_, _, err = runCommandWithOutput(killCmd)
+	c.Assert(err, check.Not(check.IsNil), check.Commentf("Kill succeeded on a stopped container"))


I don't think this addresses the previous comment, you're not checking the output to make sure you get the new error message you're generating.

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

rhatdan · 2015-05-28T00:56:40Z

The failure looks unrelated to the pull request.

thaJeztah · 2015-05-30T20:00:06Z

@rhatdan triggered a rebuild

ping @cpuguy83 @duglin PTAL

rhatdan · 2015-06-10T15:25:24Z

Any movement on this one? Seems like a fairly simple patch to merge.

cpuguy83 · 2015-06-10T15:28:22Z

LGTM

duglin · 2015-06-10T15:32:54Z

LGTM

docker kill should return error if container is not running.

rhatdan · 2015-06-10T15:46:08Z

Thanks.

runcom · 2015-06-10T17:47:12Z

I just want to note that here #12085 (comment) we talked about breaking Kill api and I'm just wondering if we should version this change

thaJeztah · 2015-06-10T18:05:38Z

Good point @runcom.

@LK4D4 wdyt (since you raised that concern)

runcom · 2015-06-10T18:21:04Z

And also with this change not only container kill changes its behavior but also delete (and stop iirc)

GordonTheTurtle added status/0-triage dco/yes and removed dco/no labels Apr 14, 2015

icecrime added status/2-code-review and removed status/0-triage labels Apr 15, 2015

icecrime reviewed Apr 15, 2015
View reviewed changes

icecrime removed the dco/yes label Apr 23, 2015

rhatdan force-pushed the kill branch from 0f592ec to 4baada3 Compare April 23, 2015 12:05

GordonTheTurtle removed the dco/no label Apr 23, 2015

cpuguy83 reviewed Apr 23, 2015
View reviewed changes

rhatdan force-pushed the kill branch from 32cef40 to ff3a461 Compare April 23, 2015 15:12

GordonTheTurtle removed the dco/no label Apr 23, 2015

rhatdan force-pushed the kill branch from ff3a461 to ce0b631 Compare April 23, 2015 15:13

GordonTheTurtle removed the dco/no label Apr 23, 2015

duglin reviewed Apr 23, 2015
View reviewed changes

rhatdan force-pushed the kill branch from ce0b631 to 3ba51ed Compare April 23, 2015 18:25

GordonTheTurtle removed the dco/no label Apr 23, 2015

Merge branch 'master' of github.com:docker/docker into kill

d0a4f31

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

rhatdan force-pushed the kill branch from 3c6ca9b to d0a4f31 Compare April 29, 2015 19:07

rhatdan force-pushed the kill branch from d0a4f31 to c0891fd Compare May 11, 2015 19:47

rhatdan force-pushed the kill branch from 845bdac to 5adba2d Compare May 27, 2015 01:24

cpuguy83 reviewed May 27, 2015
View reviewed changes

rhatdan force-pushed the kill branch from 5adba2d to c6ab03f Compare May 27, 2015 14:50

Merge branch 'master' of github.com:docker/docker into kill

66121d1

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

rhatdan force-pushed the kill branch from c6ab03f to 66121d1 Compare May 27, 2015 15:00

duglin reviewed May 27, 2015
View reviewed changes

Merge branch 'master' of github.com:docker/docker into kill

35a7f0c

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

duglin added a commit that referenced this pull request Jun 10, 2015

Merge pull request #12371 from rhatdan/kill

46af724

docker kill should return error if container is not running.

duglin merged commit 46af724 into moby:master Jun 10, 2015

aanand mentioned this pull request Jul 28, 2015

Error when attempting to kill a non-running container breaks backwards compatibility #15080

Closed

thaJeztah mentioned this pull request Jan 21, 2024

Keep backwards compatibility in kill api. #15098

Merged

docker kill should return error if container is not running. #12371

docker kill should return error if container is not running. #12371

Uh oh!

Conversation

rhatdan commented Apr 14, 2015

Uh oh!

runcom commented Apr 14, 2015

Uh oh!

runcom commented Apr 14, 2015

Uh oh!

rhatdan commented Apr 14, 2015

Uh oh!

rhatdan commented Apr 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessfraz commented Apr 22, 2015

Uh oh!

duglin commented Apr 22, 2015

Uh oh!

rhatdan commented Apr 23, 2015

Uh oh!

duglin commented Apr 23, 2015

Uh oh!

rhatdan commented Apr 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Apr 23, 2015

Uh oh!

rhatdan commented Apr 23, 2015

Uh oh!

duglin commented May 7, 2015

Uh oh!

rhatdan commented May 11, 2015

Uh oh!

cpuguy83 commented May 22, 2015

Uh oh!

rhatdan commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented May 28, 2015

Uh oh!

thaJeztah commented May 30, 2015

Uh oh!

rhatdan commented Jun 10, 2015

Uh oh!

cpuguy83 commented Jun 10, 2015

Uh oh!

duglin commented Jun 10, 2015

Uh oh!

rhatdan commented Jun 10, 2015

Uh oh!

runcom commented Jun 10, 2015

Uh oh!

thaJeztah commented Jun 10, 2015

Uh oh!

runcom commented Jun 10, 2015

Uh oh!

Uh oh!