Skip to content
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

Docker Network UX & remote API changes #16645

Merged
merged 3 commits into from Oct 7, 2015
Merged

Docker Network UX & remote API changes #16645

merged 3 commits into from Oct 7, 2015

Conversation

mavenugo
Copy link
Contributor

This PR is a follow up for various discussion around docker network experimental UX and API.
Includes :

  • Moved the UX and API handling from docker/libnetwork to docker/docker
  • Removed the experimental services concept & replaced with a simplified network UX and API.
  • Replaced experimental --publish-service with --net=<user-defined-network>
  • Moving all the relevant Network UX and Remote APIs out of experimental
  • Removed experimental --default-network daemon flag
  • Neccessary backend changes to accomodate multiple networks connect UX per container
  • Integration Tests
  • Initial Docs update

Note that this PR updates the UX and API aspects of the CNM functionality.
there are a few related back-end issues still open (moby/libnetwork#567, moby/libnetwork#570, moby/libnetwork#568) and will be addressed as part of libnetwork vendor-in in subsequent PRs.

UX (https://github.com/mavenugo/docker/blob/ux/docs/userguide/dockernetworks.md)
API (https://github.com/mavenugo/docker/blob/ux/docs/reference/api/docker_remote_api_v1.21.md#25-networks).

@cpuguy83
Copy link
Member

Some notes from playing with this:

  1. Disconnecting a container from a network does not seem to remove /etc/hosts entries from the specified container for that network, though it seems the other containers on the network are updated to remove the container's entry
  2. Docs say that you can only connect a running container to a network, but running or not seems to be ok (which I think is good). Initially from the docs I thought it would be impossible to pre-setup a all of a container's network needs (ie, when it needs to be on more than one network) prior to starting it... although I did notice that if you have a stopped container, connect it to a network, then start it, you get a warning: Could not connect container fdbfcb3fb78ba0541accf1e77e8df9168c09f0fe0e9cac550d52fff50ba0276e :a sandbox has already joined the endpoint, even though it does work.
  3. It is currently possible to remove a network which is in use, given that the container(s) using that network are also on another network.

@mavenugo
Copy link
Contributor Author

@cpuguy83 thanks for the feedback. Will look into these.
on #3, I have a test-case (https://github.com/docker/docker/pull/16645/files#diff-0189e098e6ba3aeffd9ee321ee6aca8aR3458) testing for that specific use-case. Will check if multiple networks scenario has any impact on it.

@mavenugo
Copy link
Contributor Author

@cpuguy83

#1 is a backend issue in libnetwork (moby/libnetwork#572).
#2 and #3 should be resolved now with the updated check for active containers. Also added an integration-test for that.

As discussed in IRC, we are trying to be consistent with the current network resource usage when the container is down and hence, its not appropriate to hold onto network resources when it is not active. But having said that, there has been requests for ip-address stability across container restarts (moby/libnetwork#569). When we resolve that, we could potentially remove this restriction and allow users to connect and disconnect from networks even when the container is down.

@mavenugo
Copy link
Contributor Author

The windows CI failure is not related to this PR and I see multiple PRs failing on the same issue.

@tombee
Copy link
Contributor

tombee commented Sep 29, 2015

Hey @mavenugo, awesome work on this -- this is exciting stuff. The simplified UX is nice! 👍

This works great, the steps I followed to get this working are in this gist: https://gist.github.com/tombee/fa22d54923829b8be046

@mavenugo
Copy link
Contributor Author

@tombee thanks for the feedback and trying it out using the multi-host networking feature.

@ibuildthecloud
Copy link
Contributor

Sorry, just deleted some comment, I realized I was testing the wrong branch... too many PRs :)

@ibuildthecloud
Copy link
Contributor

At this point I don't see the value in docker network inspect. With the absence of the -f flag like docker inspect -f '{{...}', it just returns the same info in docker network ls

@ibuildthecloud
Copy link
Contributor

docker network ls -l and docker network ls -n=1 don't seem to work

@ibuildthecloud
Copy link
Contributor

I stand corrected. I see that docker network inspect will return the container info. I think that containers should be containers: {} if there are no containers in the network. Not just drop the field. So omitempty shouldn't be set.

Also, why don't stopped container show up in network inspect?

@ibuildthecloud
Copy link
Contributor

I think its critical to return the scope of the network in /networks and in the inspect. It will be important to understand if the network is local or global, especially for orchestration systems.

@ibuildthecloud
Copy link
Contributor

In the container inspect why would we prefer to put the network name in NetworkSettings.Networks as opposed to the ID. It would seem to me that with local and global networks it would be possible to get a name conflict. For example, host A has local net called foo and then host B creates global net called foo.

I also prefer id's because I often log docker inspect output. Over time you can create and delete things with the same name. It's nice to have a unique reference like the ID.

@mavenugo
Copy link
Contributor Author

@ibuildthecloud thanks for trying out the changes.

docker network ls -l and docker network ls -n=1 don't seem to work

Will fix it

So omitempty shouldn't be set.

Agreed. will fix it

why don't stopped container show up in network inspect?

Thats because, with the current backend implementation, When a container is stopped, we cleanup the endpoints and free up sandbox, endpoint and cleanup all the network resources allocated for that. These are operational state. If the container is started again, we will make use of the NetworkSettings (Configuration state) to recreate the resources appropriately.
Since the network resources are freed up when the container is stopped, inspect just doesnt show up any information for that container. Also, if all the containers in a network is stopped, user can even remove the network.
Having said that, if we support moby/libnetwork#569, then this will change and the network resources will be retained even if the container is stopped.

I think its critical to return the scope of the network in /networks

okay. That requires a back-end API change. am trying to keep this PR purely UX centric (as you can see there is no libnetwork vendor-in in this PR) to ease the reviewer burden. We can update it in another PR after the backend is updated.

why would we prefer to put the network name in NetworkSettings.Networks as opposed to the ID

The main purpose is that, we dont persist the default networks (bridge, none and host). Hence upon restart these default networks get a new ID, but the name remains the same. That is the only reason.
The question on why we dont persist the default networks is a long conversation. But, I prefer storing the ID as well. If you look at the code carefully, I made sure the []networks can take in both name or id. Because, when the backend changes (to accomodate default network persistence), we can flip the switch as easily to start storing id. Infact we can do it for 1.9 after the libnetwork backend supports default network persistence.

@cpuguy83
Copy link
Member

Design 👍 I like this a lot.

Only thing that was a little weird is the ordering of docker network connect <network> <container>, I naturally want to switch <container> and <network>... just throwing it out there in case anyone else did this as well... but as is, is ok.

@mavenugo
Copy link
Contributor Author

thanks @cpuguy83

the ordering of docker network connect , I naturally want to switch and

Yes, we did think of that. but, decided to settle with due to a matter of convention where folks would come to expect the param to be the key for all the docker network command. Hence placing it first seems more appropriate.

@ibuildthecloud
Copy link
Contributor

@mavenugo I think IP stability is important. Specifically I'm dealing with hadoop right now and if you change the IP bad things happen. We are currently using our managed networking which maintains IPs across reboot, but I think that should be a core feature.

For the NetworkSettings.Networks, is it possible there is a compromise such that IDs go in there except host, bridge, none put the name.

I really need to know if the networks are global vs local, can that be added to the API?

Last thing. I noticed that docker volume defaults to docker volume ls where as docker network prints the help. I think between @cpuguy83 and @mavenugo we should have a consistent approach.

@cpuguy83
Copy link
Member

@ibuildthecloud IP stability is completely separate from this change, especially since there is no stability today.

@icecrime
Copy link
Contributor

icecrime commented Oct 7, 2015

image

@mavenugo
Copy link
Contributor Author

mavenugo commented Oct 7, 2015

lol... thanks @icecrime @cpuguy83
🎆 🎆 🎉

@manoj0077
Copy link

@mavenugo coming to point raised by @vieux when the container is disconnected from network and removed ...loop back interface is not getting deleted...../var/run/docker/netns is getting populated in this scenario....

mavenugo referenced this pull request in mavenugo/docker Oct 12, 2015
introduced --subnet, --ip-range and --gateway options in docker network
command. Also, user can allocate driver specific ip-address if any using
the --aux-address option.
Supports multiple subnets per network and also sharing ip range
across networks if the network-driver and ipam-driver supports it.
Example, Bridge driver doesnt support sharing same ip range across
networks.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@guybrush
Copy link

guybrush commented Nov 3, 2015

i cant reproduce the example in the UX-doc with 1.9.0-rc4, am i doing something wrong?

/etc/hosts does not get updated: https://gist.github.com/guybrush/28bbf90b9c32244465f7

@thaJeztah
Copy link
Member

@guybrush network docs will be updated soon; updating /etc/hosts (service discovery) will not be done on the default "bridge" network, only with other networks (i.e. docker network create foo and docker run -itd --name c1 --net foo nginx)

@guybrush
Copy link

guybrush commented Nov 3, 2015

woah that was fast response, i cant wait to use the new feature :D

thanks!

@manoj0077
Copy link

@mavenugo:- Can I pass multiple networks in --net option ? or is there any plan for supporting it. Thanks in advance for letting me know.

@thaJeztah
Copy link
Member

@manoj0077 it's not supported at this moment (it would be a nice addition though), but you can dynamically add a container to a network;

docker network connect <networkname> <containername>

For documentation related to the new networking, see https://github.com/docker/docker/blob/master/docs/userguide/networking/index.md

openstack-gerrit pushed a commit to openstack/kuryr that referenced this pull request Nov 3, 2015
Updating devref to address following changes:
1. Removal of 'service' from docker
2. Vendoring in IPAM driver support from libnetwork to docker. This
allows user to pass subnet of their choice using docker cli. In case
if user does not provide subnet related configuration and neither
remote ipam driver while creating network, built-in ipam driver will
provide default subnet configuration.

refer to the links for details:
moby/moby#16645
https://github.com/docker/libnetwork/blob/master/docs/ipam.md

Change-Id: I30f5cb54d4feadafc954f6e4b09bcbd6243e6c00
Closes-Bug: #1505489
@manoj0077
Copy link

@thaJeztah So is there a possibility to add multiple interfaces at the time of starting of container itself....

@mavenugo
Copy link
Contributor Author

mavenugo commented Nov 3, 2015

@manoj0077 no. it is not possible currently.

@zrml
Copy link

zrml commented Mar 4, 2016

Is this still an experimental feature in 1.10?
Thank you

@thaJeztah
Copy link
Member

@zrml no, it's no longer experimental https://docs.docker.com/engine/userguide/networking/dockernetworks/

stevvooe added a commit to stevvooe/engine-api that referenced this pull request Apr 11, 2016
This was never used in a non-experimental branch. It was removed from
docker in moby/moby#16645.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet