From 78100706303efdf9a3deee0239a7dbb3a93837ec Mon Sep 17 00:00:00 2001 From: Adrien Folie Date: Sun, 6 Jul 2014 22:43:24 +0200 Subject: [PATCH 1/4] Stop/Kill options for containers removal Docker-DCO-1.1-Signed-off-by: Adrien Folie (github: folieadrien) --- api/client/commands.go | 13 ++++++++++--- api/server/server.go | 12 +++++++++++- server/server.go | 11 ++++++++--- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/api/client/commands.go b/api/client/commands.go index 7e1e81e503..decc643474 100644 --- a/api/client/commands.go +++ b/api/client/commands.go @@ -983,7 +983,8 @@ func (cli *DockerCli) CmdRm(args ...string) error { cmd := cli.Subcmd("rm", "[OPTIONS] CONTAINER [CONTAINER...]", "Remove one or more containers") v := cmd.Bool([]string{"v", "-volumes"}, false, "Remove the volumes associated with the container") link := cmd.Bool([]string{"l", "#link", "-link"}, false, "Remove the specified link and not the underlying container") - force := cmd.Bool([]string{"f", "-force"}, false, "Force removal of running container") + stop := cmd.Bool([]string{"#f", "s", "#-force", "-stop"}, false, "Stop and remove a running container") + kill := cmd.Bool([]string{"k", "-kill"}, false, "Kill and remove a running container") if err := cmd.Parse(args); err != nil { return nil @@ -992,6 +993,9 @@ func (cli *DockerCli) CmdRm(args ...string) error { cmd.Usage() return nil } + if *stop && *kill { + return fmt.Errorf("Conflicting options: -s/--stop and -k/--kill") + } val := url.Values{} if *v { val.Set("v", "1") @@ -999,8 +1003,11 @@ func (cli *DockerCli) CmdRm(args ...string) error { if *link { val.Set("link", "1") } - if *force { - val.Set("force", "1") + if *stop { + val.Set("stop", "1") + } + if *kill { + val.Set("kill", "1") } var encounteredError error diff --git a/api/server/server.go b/api/server/server.go index 6d6ed3e773..56b767c405 100644 --- a/api/server/server.go +++ b/api/server/server.go @@ -678,9 +678,19 @@ func deleteContainers(eng *engine.Engine, version version.Version, w http.Respon return fmt.Errorf("Missing parameter") } job := eng.Job("container_delete", vars["name"]) + + if version.GreaterThanOrEqualTo("1.14") { + job.Setenv("stop", r.Form.Get("stop")) + job.Setenv("kill", r.Form.Get("kill")) + + if job.GetenvBool("stop") && job.GetenvBool("kill") { + return fmt.Errorf("Bad parameters: can't use stop and kill simultaneously") + } + } else { + job.Setenv("stop", r.Form.Get("force")) + } job.Setenv("removeVolume", r.Form.Get("v")) job.Setenv("removeLink", r.Form.Get("link")) - job.Setenv("forceRemove", r.Form.Get("force")) if err := job.Run(); err != nil { return err } diff --git a/server/server.go b/server/server.go index 40995da5c4..114a3bf46d 100644 --- a/server/server.go +++ b/server/server.go @@ -1787,7 +1787,8 @@ func (srv *Server) ContainerDestroy(job *engine.Job) engine.Status { name := job.Args[0] removeVolume := job.GetenvBool("removeVolume") removeLink := job.GetenvBool("removeLink") - forceRemove := job.GetenvBool("forceRemove") + stop := job.GetenvBool("stop") + kill := job.GetenvBool("kill") container := srv.daemon.Get(name) @@ -1821,12 +1822,16 @@ func (srv *Server) ContainerDestroy(job *engine.Job) engine.Status { if container != nil { if container.State.IsRunning() { - if forceRemove { + if stop { if err := container.Stop(5); err != nil { return job.Errorf("Could not stop running container, cannot remove - %v", err) } + } else if kill { + if err := container.Kill(); err != nil { + return job.Errorf("Could not kill running container, cannot remove - %v", err) + } } else { - return job.Errorf("Impossible to remove a running container, please stop it first or use -f") + return job.Errorf("You cannot remove a running container. Stop the container before attempting removal or use -s or -k") } } if err := srv.daemon.Destroy(container); err != nil { From d689a5e1e2633b5fd0db4ccccfb4969e03ae256b Mon Sep 17 00:00:00 2001 From: Adrien Folie Date: Sun, 6 Jul 2014 22:44:56 +0200 Subject: [PATCH 2/4] Add tests for rm with stop and kill options Docker-DCO-1.1-Signed-off-by: Adrien Folie (github: folieadrien) --- api/server/server_unit_test.go | 47 ++++++++++++++++++++++ integration-cli/docker_cli_rm_test.go | 58 +++++++++++++++++++++------ integration/api_test.go | 29 -------------- 3 files changed, 93 insertions(+), 41 deletions(-) diff --git a/api/server/server_unit_test.go b/api/server/server_unit_test.go index c51ea6fc0d..6814ca8a47 100644 --- a/api/server/server_unit_test.go +++ b/api/server/server_unit_test.go @@ -451,6 +451,53 @@ func TestGetImagesByName(t *testing.T) { } } +func TestDeleteContainers(t *testing.T) { + eng := engine.New() + name := "foo" + var called bool + eng.Register("container_delete", func(job *engine.Job) engine.Status { + called = true + if len(job.Args) == 0 { + t.Fatalf("Job arguments is empty") + } + if job.Args[0] != name { + t.Fatalf("name != '%s': %#v", name, job.Args[0]) + } + return engine.StatusOK + }) + r := serveRequest("DELETE", "/containers/"+name, nil, eng, t) + if !called { + t.Fatalf("handler was not called") + } + if r.Code != http.StatusNoContent { + t.Fatalf("Got status %d, expected %d", r.Code, http.StatusNoContent) + } +} + +func TestDeleteContainersWithStopAndKill(t *testing.T) { + if api.APIVERSION.LessThan("1.14") { + return + } + eng := engine.New() + var called bool + eng.Register("container_delete", func(job *engine.Job) engine.Status { + called = true + return engine.StatusOK + }) + r := serveRequest("DELETE", "/containers/foo?stop=1&kill=1", nil, eng, t) + if r.Code != http.StatusBadRequest { + t.Fatalf("Got status %d, expected %d", r.Code, http.StatusBadRequest) + } + if called { + t.Fatalf("container_delete jobs was called, but it shouldn't") + } + res := strings.TrimSpace(r.Body.String()) + expected := "Bad parameters: can't use stop and kill simultaneously" + if !strings.Contains(res, expected) { + t.Fatalf("Output %s, expected %s in it", res, expected) + } +} + func serveRequest(method, target string, body io.Reader, eng *engine.Engine, t *testing.T) *httptest.ResponseRecorder { return serveRequestUsingVersion(method, target, api.APIVERSION, body, eng, t) } diff --git a/integration-cli/docker_cli_rm_test.go b/integration-cli/docker_cli_rm_test.go index 34b5df0338..b61f1c9a93 100644 --- a/integration-cli/docker_cli_rm_test.go +++ b/integration-cli/docker_cli_rm_test.go @@ -42,25 +42,59 @@ func TestRemoveContainerWithVolume(t *testing.T) { logDone("rm - volume") } -func TestRemoveContainerRunning(t *testing.T) { - cmd := exec.Command(dockerBinary, "run", "-dt", "--name", "foo", "busybox", "top") - if _, err := runCommand(cmd); err != nil { - t.Fatal(err) - } +func TestRemoveRunningContainer(t *testing.T) { + createRunningContainer(t, "foo") // Test cannot remove running container - cmd = exec.Command(dockerBinary, "rm", "foo") + cmd := exec.Command(dockerBinary, "rm", "foo") if _, err := runCommand(cmd); err == nil { t.Fatalf("Expected error, can't rm a running container") } - // Remove with -f - cmd = exec.Command(dockerBinary, "rm", "-f", "foo") - if _, err := runCommand(cmd); err != nil { - t.Fatal(err) - } - deleteAllContainers() logDone("rm - running container") } + +func TestStopAndRemoveRunningContainer(t *testing.T) { + createRunningContainer(t, "foo") + + // Stop then remove with -s + cmd := exec.Command(dockerBinary, "rm", "-s", "foo") + if _, err := runCommand(cmd); err != nil { + t.Fatal(err) + } + + deleteAllContainers() + + logDone("rm - running container with --stop=true") +} + +func TestKillAndRemoveRunningContainer(t *testing.T) { + createRunningContainer(t, "foo") + + // Kill then remove with -k + cmd := exec.Command(dockerBinary, "rm", "-k", "foo") + if _, err := runCommand(cmd); err != nil { + t.Fatal(err) + } + + deleteAllContainers() + + logDone("rm - running container with --kill=true") +} + +func TestRemoveContainerWithStopAndKill(t *testing.T) { + cmd := exec.Command(dockerBinary, "rm", "-sk", "foo") + if _, err := runCommand(cmd); err == nil { + t.Fatalf("Expected error: can't use stop and kill simulteanously") + } + logDone("rm - with --stop=true and --kill=true") +} + +func createRunningContainer(t *testing.T, name string) { + cmd := exec.Command(dockerBinary, "run", "-dt", "--name", name, "busybox", "top") + if _, err := runCommand(cmd); err != nil { + t.Fatal(err) + } +} diff --git a/integration/api_test.go b/integration/api_test.go index 0da96ccd1e..b8414b6179 100644 --- a/integration/api_test.go +++ b/integration/api_test.go @@ -768,35 +768,6 @@ func TestPostContainersAttachStderr(t *testing.T) { containerWait(eng, containerID, t) } -// FIXME: Test deleting running container -// FIXME: Test deleting container with volume -// FIXME: Test deleting volume in use by other container -func TestDeleteContainers(t *testing.T) { - eng := NewTestEngine(t) - defer mkDaemonFromEngine(eng, t).Nuke() - - containerID := createTestContainer(eng, - &runconfig.Config{ - Image: unitTestImageID, - Cmd: []string{"touch", "/test"}, - }, - t, - ) - req, err := http.NewRequest("DELETE", "/containers/"+containerID, nil) - if err != nil { - t.Fatal(err) - } - r := httptest.NewRecorder() - if err := server.ServeRequest(eng, api.APIVERSION, r, req); err != nil { - t.Fatal(err) - } - assertHttpNotError(r, t) - if r.Code != http.StatusNoContent { - t.Fatalf("%d NO CONTENT expected, received %d\n", http.StatusNoContent, r.Code) - } - containerAssertNotExists(eng, containerID, t) -} - func TestOptionsRoute(t *testing.T) { eng := NewTestEngine(t) defer mkDaemonFromEngine(eng, t).Nuke() From 5ba11e6890e51fd01f674160473af3ac432622c5 Mon Sep 17 00:00:00 2001 From: Adrien Folie Date: Tue, 8 Jul 2014 20:31:20 +0200 Subject: [PATCH 3/4] update CLI & api docs Docker-DCO-1.1-Signed-off-by: Adrien Folie (github: folieadrien) --- docs/man/docker-rm.1.md | 10 +++++++--- docs/sources/reference/api/docker_remote_api.md | 8 ++++++++ docs/sources/reference/commandline/cli.md | 17 ++++++++++++++++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/docs/man/docker-rm.1.md b/docs/man/docker-rm.1.md index 1b45376976..f100ddb87c 100644 --- a/docs/man/docker-rm.1.md +++ b/docs/man/docker-rm.1.md @@ -6,7 +6,8 @@ docker-rm - Remove one or more containers # SYNOPSIS **docker rm** -[**-f**|**--force**[=*false*]] +[**-s**|**--stop**[=*false*]] +[**-k**|**--kill**[=*false*]] [**-l**|**--link**[=*false*]] [**-v**|**--volumes**[=*false*]] CONTAINER [CONTAINER...] @@ -19,8 +20,11 @@ remove a running container unless you use the \fB-f\fR option. To see all containers on a host use the **docker ps -a** command. # OPTIONS -**-f**, **--force**=*true*|*false* - Force removal of running container. The default is *false*. +**-s**, **--stop**=*true*|*false* + Stop then remove a running container. The default is *false*. + +**-k**, **--kill**=*true*|*false* + Kill then remove a running container. The default is *false*. **-l**, **--link**=*true*|*false* Remove the specified link and not the underlying container. The default is *false*. diff --git a/docs/sources/reference/api/docker_remote_api.md b/docs/sources/reference/api/docker_remote_api.md index 92d3dfcb4c..dd0e358e51 100644 --- a/docs/sources/reference/api/docker_remote_api.md +++ b/docs/sources/reference/api/docker_remote_api.md @@ -42,6 +42,14 @@ You can still call an old version of the API using ### What's new +`DELETE /containers/(id)` + +New! You can now use the `stop` parameter to stop running containers +before removal (replace `force`). + +New! You can now use the `kill` parameter to kill running containers +before removal. + `GET /containers/(name)/json` **New!** diff --git a/docs/sources/reference/commandline/cli.md b/docs/sources/reference/commandline/cli.md index 7ae1b30c0a..e9d21cac47 100644 --- a/docs/sources/reference/commandline/cli.md +++ b/docs/sources/reference/commandline/cli.md @@ -838,7 +838,8 @@ registry or to a self-hosted one. Remove one or more containers - -f, --force=false Force removal of running container + -s, --stop=false Stop and remove a running container + -k, --kill=false Kill and remove a running container -l, --link=false Remove the specified link and not the underlying container -v, --volumes=false Remove the volumes associated with the container @@ -863,6 +864,20 @@ This will remove the underlying link between `/webapp` and the `/redis` containers removing all network communication. + $ sudo docker rm --stop redis + redis + +The main process inside the container referenced under the link `/redis` will receive +SIGTERM, and after a grace period, SIGKILL, then the container will be removed. + + $ sudo docker rm --kill redis + redis + +The main process inside the container referenced under the link `/redis` will receive +SIGKILL, then the container will be removed. + +NOTE: If you try to use `stop` and `kill` simultaneously, Docker will return an error. + $ sudo docker rm $(docker ps -a -q) This command will delete all stopped containers. The command From fe5ab5b058e33d2d8577f1803448078e8eb92345 Mon Sep 17 00:00:00 2001 From: Victor Vieux Date: Fri, 11 Jul 2014 23:27:23 +0000 Subject: [PATCH 4/4] proper rebase Docker-DCO-1.1-Signed-off-by: Victor Vieux (github: vieux) --- docs/sources/reference/api/docker_remote_api.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/sources/reference/api/docker_remote_api.md b/docs/sources/reference/api/docker_remote_api.md index dd0e358e51..04b750f4c5 100644 --- a/docs/sources/reference/api/docker_remote_api.md +++ b/docs/sources/reference/api/docker_remote_api.md @@ -34,6 +34,15 @@ You can still call an old version of the API using ### What's new +`DELETE /containers/(id)` + +**New!** +You can now use the `stop` parameter to stop running containers before removal +(replace `force`). + +**New!** +You can now use the `kill` parameter to kill running containers before removal. + ## v1.13 ### Full Documentation @@ -42,14 +51,6 @@ You can still call an old version of the API using ### What's new -`DELETE /containers/(id)` - -New! You can now use the `stop` parameter to stop running containers -before removal (replace `force`). - -New! You can now use the `kill` parameter to kill running containers -before removal. - `GET /containers/(name)/json` **New!**