-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve error of docker network create -d overlay
on non-Swarm node
#31912
Conversation
daemon/network.go
Outdated
@@ -262,6 +263,11 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string | |||
driver = c.Config().Daemon.DefaultDriver | |||
} | |||
|
|||
if driver == "overlay" && | |||
(daemon.clusterProvider == nil || !daemon.clusterProvider.IsManager()) { | |||
return nil, errorutils.ErrNotSwarmManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be a problem if someone uses an external k/v store for overlay networks?
7125f7e
to
54da659
Compare
@thaJeztah |
daemon/network.go
Outdated
@@ -262,6 +263,12 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string | |||
driver = c.Config().Daemon.DefaultDriver | |||
} | |||
|
|||
isSwarmModeManager := daemon.clusterProvider != nil && daemon.clusterProvider.IsManager() | |||
withClusterStore := daemon.configStore.ClusterStore != "" // classic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure looking up configStore is correct here, but netController does not seem to contain cluster store info
54da659
to
3a867d3
Compare
3a867d3
to
89086bc
Compare
Failure on windows and z should be unrelated. PTAL |
daemon/network.go
Outdated
@@ -302,6 +303,12 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string | |||
driver = c.Config().Daemon.DefaultDriver | |||
} | |||
|
|||
isSwarmModeManager := daemon.clusterProvider != nil && daemon.clusterProvider.IsManager() | |||
withClusterStore := daemon.configStore.ClusterStore != "" // classic | |||
if driver == "overlay" && !isSwarmModeManager && !withClusterStore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AkihiroSuda, I agree with this change as it will remove the very confusing previous message.
But I think we should not base it on the driver name (overlay
), because we support network plugins which can be global scope, therefore working with previous cluster store config and with new swarm.
What do you think about have libnetwork return a well known error in case the datastore is not present
https://github.com/docker/libnetwork/blob/master/store.go#L228
so that you can recognize it at L331 here and return the new error instead ?
You can define the error in https://github.com/docker/libnetwork/blob/master/error.go
89086bc
to
5a3f4eb
Compare
@aboch |
docker network create -d overlay
on non-Swarm nodedocker network create -d overlay
on non-Swarm node + vendor libnetwork@f6ce0ce
vendored moby/libnetwork#1707 in this PR |
f2ca26a
to
444f3c9
Compare
rebased PTAL @aboch cc @vdemeester ( |
docker network create -d overlay
on non-Swarm node + vendor libnetwork@f6ce0cedocker network create -d overlay
on non-Swarm node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AkihiroSuda definitely, it shouldn't be in that package. /cc @dnephin |
@vdemeester Where is the best place? |
Looks good to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
daemon/errors.go
might have worked, but it's used from the CLI. I don't have any better suggetions
@dnephin @AkihiroSuda " |
Until 1.12 we had Can we revive this pkg? (with different content) |
If this is all because we want to prevent writing the error message twice; perhaps it's not that bad to just write it twice. Also; the error message from the CLI could diverge from the error message generated by the daemon (in a way, it's already odd that the daemon/API returns instructions on how to use the CLI, while it doesn't even know what CLI is used) |
(i.e. the "Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again." part is something that the client should add probably) |
before: Error response from daemon: datastore for scope "global" is not initialized after: Error response from daemon: This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again. Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
444f3c9
to
9c77a4c
Compare
Thank you @thaJeztah , removed errorutils pkg. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦁
- What I did
Improved the error of
docker network create -d overlay
on non-Swarm node.before:
Error response from daemon: datastore for scope "global" is not initialized
after:
Error response from daemon: This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again.
This PR also vendors moby/libnetwork@f6ce0ce (for moby/libnetwork#1707)
- How I did it
Check
daemon.clusterProvider
duringcreateNetwork()
.Also, added
pkg/errorutils
for deduplicating error literals.- How to verify it
docker network create -d overlay
on non-Swarm node- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp