Merge pull request #46213 from thaJeztah/daemon_remove_errors

daemon: cleanupContainer: don't fail if container is already stopped

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)
 		}
 	}
 

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))
+		})
 	}
 }
 

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}
 }

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))
+}

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
 

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)
 

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"))
 }