diff --git a/daemon/delete.go b/daemon/delete.go index 6e259fa104..5490e82c7f 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -89,16 +89,14 @@ func (daemon *Daemon) rmLink(cfg *config.Config, container *container.Container, func (daemon *Daemon) cleanupContainer(container *container.Container, config types.ContainerRmConfig) error { if container.IsRunning() { if !config.ForceRemove { - state := container.StateString() - procedure := "Stop the container before attempting removal or force remove" - if state == "paused" { - procedure = "Unpause and then " + strings.ToLower(procedure) + if state := container.StateString(); state == "paused" { + return errdefs.Conflict(fmt.Errorf("cannot remove container %q: container is %s and must be unpaused first", container.Name, state)) + } else { + return errdefs.Conflict(fmt.Errorf("cannot remove container %q: container is %s: stop the container before removing or force remove", container.Name, state)) } - err := fmt.Errorf("You cannot remove a %s container %s. %s", state, container.ID, procedure) - return errdefs.Conflict(err) } - if err := daemon.Kill(container); err != nil { - return fmt.Errorf("Could not kill running container %s, cannot remove - %v", container.ID, err) + if err := daemon.Kill(container); err != nil && !isNotRunning(err) { + return fmt.Errorf("cannot remove container %q: could not kill: %w", container.Name, err) } } diff --git a/daemon/delete_test.go b/daemon/delete_test.go index 3272792d7f..df7d434212 100644 --- a/daemon/delete_test.go +++ b/daemon/delete_test.go @@ -8,6 +8,7 @@ import ( "github.com/docker/docker/api/types" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" + "github.com/docker/docker/errdefs" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -34,23 +35,21 @@ func newContainerWithState(state *container.State) *container.Container { // TestContainerDelete tests that a useful error message and instructions is // given when attempting to remove a container (#30842) func TestContainerDelete(t *testing.T) { - tt := []struct { + tests := []struct { + doc string errMsg string - fixMsg string initContainer func() *container.Container }{ - // a paused container { - errMsg: "cannot remove a paused container", - fixMsg: "Unpause and then stop the container before attempting removal or force remove", + doc: "paused container", + errMsg: "container is paused and must be unpaused first", initContainer: func() *container.Container { return newContainerWithState(&container.State{Paused: true, Running: true}) }, }, - // a restarting container { - errMsg: "cannot remove a restarting container", - fixMsg: "Stop the container before attempting removal or force remove", + doc: "restarting container", + errMsg: "container is restarting: stop the container before removing or force remove", initContainer: func() *container.Container { c := newContainerWithState(container.NewState()) c.SetRunning(nil, nil, true) @@ -58,25 +57,27 @@ func TestContainerDelete(t *testing.T) { return c }, }, - // a running container { - errMsg: "cannot remove a running container", - fixMsg: "Stop the container before attempting removal or force remove", + doc: "running container", + errMsg: "container is running: stop the container before removing or force remove", initContainer: func() *container.Container { return newContainerWithState(&container.State{Running: true}) }, }, } - for _, te := range tt { - c := te.initContainer() - d, cleanup := newDaemonWithTmpRoot(t) - defer cleanup() - d.containers.Add(c.ID, c) + for _, tc := range tests { + tc := tc + t.Run(tc.doc, func(t *testing.T) { + c := tc.initContainer() + d, cleanup := newDaemonWithTmpRoot(t) + defer cleanup() + d.containers.Add(c.ID, c) - err := d.ContainerRm(c.ID, &types.ContainerRmConfig{ForceRemove: false}) - assert.Check(t, is.ErrorContains(err, te.errMsg)) - assert.Check(t, is.ErrorContains(err, te.fixMsg)) + err := d.ContainerRm(c.ID, &types.ContainerRmConfig{ForceRemove: false}) + assert.Check(t, is.ErrorType(err, errdefs.IsConflict)) + assert.Check(t, is.ErrorContains(err, tc.errMsg)) + }) } } diff --git a/daemon/errors.go b/daemon/errors.go index 803c070f12..f0790cce87 100644 --- a/daemon/errors.go +++ b/daemon/errors.go @@ -10,10 +10,21 @@ import ( "google.golang.org/grpc/status" ) -func errNotRunning(id string) error { - return errdefs.Conflict(errors.Errorf("Container %s is not running", id)) +func isNotRunning(err error) bool { + var nre *containerNotRunningError + return errors.As(err, &nre) } +func errNotRunning(id string) error { + return &containerNotRunningError{errors.Errorf("container %s is not running", id)} +} + +type containerNotRunningError struct { + error +} + +func (e containerNotRunningError) Conflict() {} + func containerNotFound(id string) error { return objNotFoundError{"container", id} } diff --git a/daemon/errors_test.go b/daemon/errors_test.go new file mode 100644 index 0000000000..1c79aa3361 --- /dev/null +++ b/daemon/errors_test.go @@ -0,0 +1,12 @@ +package daemon + +import ( + "testing" + + "gotest.tools/v3/assert" +) + +func TestContainerNotRunningError(t *testing.T) { + err := errNotRunning("12345") + assert.Check(t, isNotRunning(err)) +} diff --git a/hack/make/test-docker-py b/hack/make/test-docker-py index a9e415955c..564e1ec671 100644 --- a/hack/make/test-docker-py +++ b/hack/make/test-docker-py @@ -23,10 +23,12 @@ source hack/make/.integration-test-helpers # TODO re-enable test_attach_no_stream after https://github.com/docker/docker-py/issues/2513 is resolved # TODO re-enable test_create_with_device_cgroup_rules after https://github.com/docker/docker-py/issues/2939 is resolved # TODO re-enable test_prune_volumes after https://github.com/docker/docker-py/pull/3051 is resolved +# TODO re-enable test_create_with_restart_policy after https://github.com/docker/docker-py/pull/3165 is included in a release +# TODO re-enable test_api_error_parses_json after https://github.com/docker/docker-py/pull/3165 is included in a release # TODO re-enable test_connect_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169 # TODO re-enable test_create_network_ipv6_enabled after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169 # TODO re-enable test_create_with_ipv6_address after we updated to a version of docker-py with https://github.com/docker/docker-py/pull/3169 -: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules --deselect=tests/integration/api_volume_test.py::TestVolumes::test_prune_volumes --deselect=tests/integration/api_network_test.py::TestNetworks::test_connect_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_network_ipv6_enabled --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_with_ipv6_address}" +: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_restart_policy --deselect=tests/integration/api_volume_test.py::TestVolumes::test_prune_volumes --deselect=tests/integration/errors_test.py::ErrorsTest::test_api_error_parses_json --deselect=tests/integration/api_network_test.py::TestNetworks::test_connect_with_ipv6_address --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_network_ipv6_enabled --deselect=tests/integration/api_network_test.py::TestNetworks::test_create_with_ipv6_address}" ( bundle .integration-daemon-start diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index e8ac393f77..7158847c66 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -1174,21 +1174,6 @@ func (s *DockerAPISuite) TestContainerAPIDeleteRemoveLinks(c *testing.T) { assert.Equal(c, linksPostRm, "null", "call to api deleteContainer links should have removed the specified links") } -func (s *DockerAPISuite) TestContainerAPIDeleteConflict(c *testing.T) { - out := runSleepingContainer(c) - - id := strings.TrimSpace(out) - assert.NilError(c, waitRun(id)) - - apiClient, err := client.NewClientWithOpts(client.FromEnv) - assert.NilError(c, err) - defer apiClient.Close() - - err = apiClient.ContainerRemove(context.Background(), id, types.ContainerRemoveOptions{}) - expected := "cannot remove a running container" - assert.ErrorContains(c, err, expected) -} - func (s *DockerAPISuite) TestContainerAPIDeleteRemoveVolume(c *testing.T) { testRequires(c, testEnv.IsLocalDaemon) diff --git a/integration/container/remove_test.go b/integration/container/remove_test.go index 0ec463287c..1c2e06d99f 100644 --- a/integration/container/remove_test.go +++ b/integration/container/remove_test.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" "github.com/docker/docker/integration/internal/container" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -49,6 +50,7 @@ func TestRemoveContainerWithRemovedVolume(t *testing.T) { assert.NilError(t, err) _, _, err = apiClient.ContainerInspectWithRaw(ctx, cID, true) + assert.Check(t, is.ErrorType(err, errdefs.IsNotFound)) assert.Check(t, is.ErrorContains(err, "No such container")) } @@ -88,7 +90,8 @@ func TestRemoveContainerRunning(t *testing.T) { cID := container.Run(ctx, t, apiClient) err := apiClient.ContainerRemove(ctx, cID, types.ContainerRemoveOptions{}) - assert.Check(t, is.ErrorContains(err, "cannot remove a running container")) + assert.Check(t, is.ErrorType(err, errdefs.IsConflict)) + assert.Check(t, is.ErrorContains(err, "container is running")) } func TestRemoveContainerForceRemoveRunning(t *testing.T) { @@ -110,5 +113,6 @@ func TestRemoveInvalidContainer(t *testing.T) { apiClient := testEnv.APIClient() err := apiClient.ContainerRemove(ctx, "unknown", types.ContainerRemoveOptions{}) + assert.Check(t, is.ErrorType(err, errdefs.IsNotFound)) assert.Check(t, is.ErrorContains(err, "No such container")) }