From c3be82eb0a35906a7ad73f2575f5ec937b4d0e5e Mon Sep 17 00:00:00 2001 From: Flavio Crisciani Date: Tue, 28 Nov 2017 17:06:26 -0800 Subject: [PATCH] Restore error type in FindNetwork The error type libnetwork.ErrNoSuchNetwork is used in the controller to retry the network creation as a managed network though the manager. The change of the type was breaking the logic causing the network to not being created anymore so that no new container on that network was able to be launched Added unit test Signed-off-by: Flavio Crisciani (cherry picked from commit 51cea0a53c2fd36832277402e9faac81bfb4abd4) Signed-off-by: Sebastiaan van Stijn --- .../cluster/executor/container/controller.go | 2 +- components/engine/daemon/daemon_test.go | 13 ++++++++++++ components/engine/daemon/errors.go | 21 +++++++++++++++---- components/engine/daemon/network.go | 4 +++- .../docker_cli_netmode_test.go | 2 +- 5 files changed, 35 insertions(+), 7 deletions(-) diff --git a/components/engine/daemon/cluster/executor/container/controller.go b/components/engine/daemon/cluster/executor/container/controller.go index 3ba4302d55..e5d453779a 100644 --- a/components/engine/daemon/cluster/executor/container/controller.go +++ b/components/engine/daemon/cluster/executor/container/controller.go @@ -183,7 +183,7 @@ func (r *controller) Start(ctx context.Context) error { for { if err := r.adapter.start(ctx); err != nil { - if _, ok := err.(libnetwork.ErrNoSuchNetwork); ok { + if _, ok := errors.Cause(err).(libnetwork.ErrNoSuchNetwork); ok { // Retry network creation again if we // failed because some of the networks // were not found. diff --git a/components/engine/daemon/daemon_test.go b/components/engine/daemon/daemon_test.go index 13d1059c1c..719ae4e989 100644 --- a/components/engine/daemon/daemon_test.go +++ b/components/engine/daemon/daemon_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "testing" + "github.com/docker/docker/api/errdefs" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/container" _ "github.com/docker/docker/pkg/discovery/memory" @@ -18,6 +19,9 @@ import ( "github.com/docker/docker/volume/local" "github.com/docker/docker/volume/store" "github.com/docker/go-connections/nat" + "github.com/docker/libnetwork" + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" ) // @@ -304,3 +308,12 @@ func TestMerge(t *testing.T) { } } } + +func TestFindNetworkErrorType(t *testing.T) { + d := Daemon{} + _, err := d.FindNetwork("fakeNet") + _, ok := errors.Cause(err).(libnetwork.ErrNoSuchNetwork) + if !errdefs.IsNotFound(err) || !ok { + assert.Fail(t, "The FindNetwork method MUST always return an error that implements the NotFound interface and is ErrNoSuchNetwork") + } +} diff --git a/components/engine/daemon/errors.go b/components/engine/daemon/errors.go index cd8de4dc7d..1f4f50490f 100644 --- a/components/engine/daemon/errors.go +++ b/components/engine/daemon/errors.go @@ -21,10 +21,6 @@ func volumeNotFound(id string) error { return objNotFoundError{"volume", id} } -func networkNotFound(id string) error { - return objNotFoundError{"network", id} -} - type objNotFoundError struct { object string id string @@ -214,3 +210,20 @@ func translateContainerdStartErr(cmd string, setExitCode func(int), err error) e // TODO: it would be nice to get some better errors from containerd so we can return better errors here return retErr } + +// TODO: cpuguy83 take care of it once the new library is ready +type errNotFound struct{ error } + +func (errNotFound) NotFound() {} + +func (e errNotFound) Cause() error { + return e.error +} + +// notFound is a helper to create an error of the class with the same name from any error type +func notFound(err error) error { + if err == nil { + return nil + } + return errNotFound{err} +} diff --git a/components/engine/daemon/network.go b/components/engine/daemon/network.go index 62fe951cbb..52b6a321e5 100644 --- a/components/engine/daemon/network.go +++ b/components/engine/daemon/network.go @@ -56,7 +56,9 @@ func (daemon *Daemon) GetNetworkByID(partialID string) (libnetwork.Network, erro list := daemon.GetNetworksByID(partialID) if len(list) == 0 { - return nil, errors.WithStack(networkNotFound(partialID)) + // Be very careful to change the error type here, the libnetwork.ErrNoSuchNetwork error is used by the controller + // to retry the creation of the network as managed through the swarm manager + return nil, errors.WithStack(notFound(libnetwork.ErrNoSuchNetwork(partialID))) } if len(list) > 1 { return nil, errors.WithStack(invalidIdentifier(partialID)) diff --git a/components/engine/integration-cli/docker_cli_netmode_test.go b/components/engine/integration-cli/docker_cli_netmode_test.go index abf1ff2cf7..2b134d4de1 100644 --- a/components/engine/integration-cli/docker_cli_netmode_test.go +++ b/components/engine/integration-cli/docker_cli_netmode_test.go @@ -49,7 +49,7 @@ func (s *DockerSuite) TestNetHostname(c *check.C) { c.Assert(out, checker.Contains, "Invalid network mode: invalid container format container:") out, _ = dockerCmdWithFail(c, "run", "--net=weird", "busybox", "ps") - c.Assert(strings.ToLower(out), checker.Contains, "no such network") + c.Assert(strings.ToLower(out), checker.Contains, "not found") } func (s *DockerSuite) TestConflictContainerNetworkAndLinks(c *check.C) {