1. docker
  2. Commit 01efb9a5ab37fc183a7953e70cc9dd36cda5e77c
Browse code

daemon/cluster/executor/container: fix error-handling

While working on this file, I noticed the `isContainerCreateNameConflict`,
`isUnknownContainer`, and `isStoppedContainer` utilities, which are used
to perform error-type detection through string-matching.

These utilities were added in 534a90a99367af6f6bba1ddcc7eb07506e41f774,
as part of the initial implementation of the Swarm executor in Docker.
At that time, the Docker API client did not return typed errors, and
various part of the code depended on string matching, which is brittle,
and it looks like `isContainerCreateNameConflict` at least is already
broken since c9d0a77657ccc3be144ed4de12d43c8cfcca8590, which changed
the error-message.

Starting with ebcb7d6b406fe50ea9a237c73004d75884184c33, we use typed
errors through the errdefs package, so we can replace these utilities:

The `isUnknownContainer` utility is replace by `errdefs.IsNotFound`,
which is returned if the object is not found. Interestingly, this utility
was checking for containers only (`No such container`), but was also
used for an `removeNetworks` call. Tracking back history of that use to
verify if it was _intentionally_ checking for a "container not found"
error;

- This check added in the initial implementation 534a90a99367af6f6bba1ddcc7eb07506e41f774
- Moved from `controller.Remove` to `container.Shutdown` to make sure the
sandbox was removed in 680d0ba4abfb00c8ac959638c72003dba0826b46
- And finally touched again in 70fa7b6a3fd9aaada582ae02c50710f218b54d1a,
which was a follow-up to the previous one, and fixed the conditions
to prevent returning early before the network was removed.

None of those patches mention that these errors are related to containers,
and checking the codepath that's executed, we can only expect a
`libmetwork.ErrNoSuchNetwork` to be returned, so this looks to have been
a bug.

The `isStoppedContainer` utility is replaced by `errdefs.IsNotModified`,
which is the error (status) returned in situations where the container
is already stopped; https://github.com/moby/moby/blob/caf502a0bc44c19b4e1dde50428de07ff756a8d3/daemon/stop.go#L30-L35
This is the only

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Sebastiaan van Stijn authored on 2024/06/18 04:14:47
Showing 2 changed files
daemon/cluster/executor/container/adapter.go
History View file @ 01efb9a
... ... 
@@ -557,16 +557,3 @@ func (c *containerAdapter) logs(ctx context.Context, options api.LogSubscription
557 557 
 	}
558 558 
 	return msgs, nil
559 559 
 }
560 
-
561 
-// todo: typed/wrapped errors
562 
-func isContainerCreateNameConflict(err error) bool {
563 
-	return strings.Contains(err.Error(), "Conflict. The name")
564 
-}
565 
-
566 
-func isUnknownContainer(err error) bool {
567 
-	return strings.Contains(err.Error(), "No such container:")
568 
-}
569 
-
570 
-func isStoppedContainer(err error) bool {
571 
-	return strings.Contains(err.Error(), "is already stopped")
572 
-}
daemon/cluster/executor/container/controller.go
History View file @ 01efb9a
... ... 
@@ -11,6 +11,7 @@ import (
11 11 
 	"github.com/docker/docker/api/types"
12 12 
 	"github.com/docker/docker/api/types/events"
13 13 
 	executorpkg "github.com/docker/docker/daemon/cluster/executor"
14 
+	"github.com/docker/docker/errdefs"
14 15 
 	"github.com/docker/docker/libnetwork"
15 16 
 	"github.com/docker/go-connections/nat"
16 17 
 	gogotypes "github.com/gogo/protobuf/types"
... ... 
@@ -65,7 +66,7 @@ func (r *controller) Task() (*api.Task, error) {
65 65 
 func (r *controller) ContainerStatus(ctx context.Context) (*api.ContainerStatus, error) {
66 66 
 	ctnr, err := r.adapter.inspect(ctx)
67 67 
 	if err != nil {
68 
-		if isUnknownContainer(err) {
68 
+		if errdefs.IsNotFound(err) {
69 69 
 			return nil, nil
70 70 
 		}
71 71 
 		return nil, err
... ... 
@@ -76,7 +77,7 @@ func (r *controller) ContainerStatus(ctx context.Context) (*api.ContainerStatus,
76 76 
 func (r *controller) PortStatus(ctx context.Context) (*api.PortStatus, error) {
77 77 
 	ctnr, err := r.adapter.inspect(ctx)
78 78 
 	if err != nil {
79 
-		if isUnknownContainer(err) {
79 
+		if errdefs.IsNotFound(err) {
80 80 
 			return nil, nil
81 81 
 		}
82 82
... ... 
@@ -178,7 +179,7 @@ func (r *controller) Prepare(ctx context.Context) error {
178 178 
 		}
179 179 
 	}
180 180 
 	if err := r.adapter.create(ctx); err != nil {
181 
-		if isContainerCreateNameConflict(err) {
181 
+		if errdefs.IsConflict(err) {
182 182 
 			if _, err := r.adapter.inspect(ctx); err != nil {
183 183 
 				return err
184 184 
 			}
... ... 
@@ -379,7 +380,7 @@ func (r *controller) Shutdown(ctx context.Context) error {
379 379 
 	}
380 380
381 381 
 	if err := r.adapter.shutdown(ctx); err != nil {
382 
-		if !(isUnknownContainer(err) || isStoppedContainer(err)) {
382 
+		if !(errdefs.IsNotFound(err) || errdefs.IsNotModified(err)) {
383 383 
 			return err
384 384 
 		}
385 385 
 	}
... ... 
@@ -387,7 +388,7 @@ func (r *controller) Shutdown(ctx context.Context) error {
387 387 
 	// Try removing networks referenced in this task in case this
388 388 
 	// task is the last one referencing it
389 389 
 	if err := r.adapter.removeNetworks(ctx); err != nil {
390 
-		if !isUnknownContainer(err) {
390 
+		if !errdefs.IsNotFound(err) {
391 391 
 			return err
392 392 
 		}
393 393 
 	}
... ... 
@@ -406,7 +407,7 @@ func (r *controller) Terminate(ctx context.Context) error {
406 406 
 	}
407 407
408 408 
 	if err := r.adapter.terminate(ctx); err != nil {
409 
-		if isUnknownContainer(err) {
409 
+		if errdefs.IsNotFound(err) {
410 410 
 			return nil
411 411 
 		}
412 412
... ... 
@@ -428,7 +429,7 @@ func (r *controller) Remove(ctx context.Context) error {
428 428
429 429 
 	// It may be necessary to shut down the task before removing it.
430 430 
 	if err := r.Shutdown(ctx); err != nil {
431 
-		if isUnknownContainer(err) {
431 
+		if errdefs.IsNotFound(err) {
432 432 
 			return nil
433 433 
 		}
434 434 
 		// This may fail if the task was already shut down.
... ... 
@@ -436,7 +437,7 @@ func (r *controller) Remove(ctx context.Context) error {
436 436 
 	}
437 437
438 438 
 	if err := r.adapter.remove(ctx); err != nil {
439 
-		if isUnknownContainer(err) {
439 
+		if errdefs.IsNotFound(err) {
440 440 
 			return nil
441 441 
 		}
442 442
... ... 
@@ -459,7 +460,7 @@ func (r *controller) waitReady(pctx context.Context) error {
459 459
460 460 
 	ctnr, err := r.adapter.inspect(ctx)
461 461 
 	if err != nil {
462 
-		if !isUnknownContainer(err) {
462 
+		if !errdefs.IsNotFound(err) {
463 463 
 			return errors.Wrap(err, "inspect container failed")
464 464 
 		}
465 465 
 	} else {