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

Add "--network-opt" option as a way to pass arbitrary options to network drivers #27638

Closed
wants to merge 2 commits into from

Conversation

axqx
Copy link

@axqx axqx commented Oct 21, 2016

  • Fixes Passing arbitrary parameters/labels to a remote driver  libnetwork#910
    The docker plugin model has made it easier to extend docker with custom
    drivers that can be maintained independently by third parties. However,
    as reported by issue Passing arbitrary parameters/labels to a remote driver  libnetwork#910
    a way of passing arbitrary parameters from docker client to remote
    driver is needed.
    Many use cases require passing network configuration to a network driver
    that might vary from container to container and might only be known at
    container creation time. Examples are configuring the MTU or queue
    length of a network interface or passing ip aliases, routes or iptables
    rules for containers. This patch proposes adding a new --net-opt option
    to docker to allow passing arbitrary information to network drivers.
    net-opt options are used with docker run command and are directly copied
    to the container's NetworkSettings and then passed to the network driver
    on endpoint create.

Signed-off-by: Alina Quereilhac alina@medallia.com

@mavenugo
Copy link
Contributor

@axqx thanks for the PR. am generally 👍 for providing such flexibility for users to make use of the driver capabilities. I initially tried to get this via labels (#13476), but was not accepted for good reason.

If we have an explicit option (I will change it to --network-opt to be consistent with other flags), that will make it clear. I am in general positive with this design.

@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 24, 2016
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 24, 2016
@axqx
Copy link
Author

axqx commented Oct 24, 2016

@mavenugo, thank you for your feedback. I added the change you suggested (--net-opt to --network-opt) in commit 4a5768604535a867d16c8509970eece1e6b42a6d.

@thaJeztah thaJeztah changed the title Add net-opt option as a way to pass arbitrary options to network drivers Add "--network-opt" option as a way to pass arbitrary options to network drivers Oct 24, 2016
@thaJeztah
Copy link
Member

@axqx don't forget to update the API changes; docs/reference/api/docker_remote_api.md#v125-api-changes, and the networks section in the API reference itself docs/reference/api/docker_remote_api_v1.25.md#35-networks

@axqx
Copy link
Author

axqx commented Oct 27, 2016

Thank you @thaJeztah. The API documentation is now updated in commit d24405599bb314da4de3c3a509bc7d607e463c24

@mavenugo
Copy link
Contributor

@axqx while we are at this, shall we expand this a little more to make it more effective ?

  • add --ipam-opt to be passed to IPAM drivers/plugins
  • add --network-opt and --ipam-opt to docker service command as well. The end-to-end integration will require changes in swarmkit project as well cc @aluzzardi . I had a brief chat with @aluzzardi regarding adding such an option in swarmkit to benefit from the work done here.

@icecrime @mrjana given we have multiple requests for such a support and we couldn't get #13476 in ... can you share your views on adding such an option ?

@jc-m
Copy link

jc-m commented Oct 27, 2016

@mavenugo Don't you think that we should make this PR as atomic as possible to avoid blocking forward progress ?

@axqx
Copy link
Author

axqx commented Oct 27, 2016

@mavenugo , I agree with @jc-m , while adding an --ipam-opt option might also be useful, I think this should probably be addressed in a different PR. I would rather keep this PR to address only one issue and one new functionality.

@icecrime
Copy link
Contributor

@mavenugo Seems fine by me, I'm happy as long as we're not using labels as the vehicle for arbitrary driver-specific options. A couple things that we maybe need to look into:

  • Is there going to be any relationship between the --network-opt flag to run and the --opt flag to network create? I can easily see some confusion if different drivers have different conventions (e.g., anything can be specified at run time versus different subsets of options for run and network create).
  • What about the consistency with volumes?

@mavenugo
Copy link
Contributor

@axqx @jc-m yes. agreed. We can move the docker service requirement out of this PR and atleast try to cover both network and ipam options. It will be so annoying otherwise for users who have been asking for such feature using labels. Now that we have a proposal with a driver specific option, lets try to knock these 2 down in 1 go. WDYT ?

@mavenugo
Copy link
Contributor

@icecrime thanks for the confirmation.

Is there going to be any relationship between the --network-opt flag to run and the --opt flag to network create? I can easily see some confusion if different drivers have different conventions (e.g., anything can be specified at run time versus different subsets of options for run and network create).

These options are directly related to CNM objects of Network and Endpoint. Driver option for network goes via --opt in network create, while the driver options for endpoint goes via this proposed flag. And since we allow just 1 network in docker run and network connect, we can associate which --network-opt is for which network-endpoint combination and I guess that is the intended behavior.

What about the consistency with volumes?

Good question. Is there such a requirement ? @cpuguy83

@mavenugo
Copy link
Contributor

@axqx also, this should address the docker network connect use-case as well. We cannot have it just in docker run and not in docker network connect.

@cpuguy83
Copy link
Member

Volumes have parity via --mount.
We could introduce a volume-opt, but I think it would be overly complex when someone could/should just use --mount.

@mauri
Copy link
Contributor

mauri commented Oct 27, 2016

+1 on adding these options to docker network connect. I'm not sure about introducing IPAM here, today they are separated concerns for a reason. I don't think that we should just piggyback unrelated changes.

@mavenugo
Copy link
Contributor

@mauri it is not unrelated. They are closely related concepts with different hook points. A good example of this is the --ip option that we introduced in docker run and --subnet option in docker network create. I do believe the --ipam-opt belong here.

@icecrime
Copy link
Contributor

@mavenugo Playing devil's advocate here:

These options are directly related to CNM objects of Network and Endpoint. Driver option for network goes via --opt in network create, while the driver options for endpoint goes via this proposed flag. And since we allow just 1 network in docker run and network connect, we can associate which --network-opt is for which network-endpoint combination and I guess that is the intended behavior.

Is it docker run --endpoint-opt then? 😇

@mavenugo
Copy link
Contributor

@icecrime

Is it docker run --endpoint-opt then? 😇

😄 True. But the user really don't need to worry about that. Since this is a per-container option for that "network", --network-opt does indicate endpoint option. (Since we allow only 1 network per container operation).

@icecrime
Copy link
Contributor

Fair enough 😉 I think it makes sense when properly explained: this configures driver-specific options for attaching that particular container to the network.

Design LGTM 👍

@axqx
Copy link
Author

axqx commented Oct 27, 2016

@mavenugo , thanks for pointing out the docker network connect use-case, indeed it needs to be addressed here. Regarding the --ipam-opt, it is certainly a related feature but implementation wise is independent from this change, so I really think it belongs to a different PR. I believe that separating independent functionality into different PRs allows to progress faster towards successfully integrating new features.

@mavenugo
Copy link
Contributor

@m-kostrzewa @axqx apologies on expanding the scope of this PR. We are between rock and hard place here. We recognize that the option is useful for network drivers. But we have made mistakes with the UX/API in the past which makes it hard to provide any reasonable compatibility. So as @thaJeztah suggested, I think it is better to start investing on the advanced --network syntax and use this as the first entry to it (along side network name). Let us not expand the scope of this PR beyond that and we will take up additional PRs to move other network specific options such as -network-alias and others into the advanced format which will let us support multiple networks in docker run and docker service together.

@axqx axqx force-pushed the net-opt branch 2 times, most recently from 7cbcd2a to 3552b6e Compare February 21, 2017 20:05
@axqx
Copy link
Author

axqx commented Feb 22, 2017

@vdemeester, @mavenugo, @thaJeztah, added some integration tests and will work on the advanced --network syntax next.

@thaJeztah
Copy link
Member

Thanks @axqx - let me put this PR back in "design review" pending those changes 👍

Alina Quereilhac added 2 commits March 3, 2017 14:04
…o network drivers

- Fixes moby#910

The docker plugin model has made it easier to extend docker with custom
drivers that can be maintained independently by third parties. However,
as reported by issue moby/libnetwork#910
a way of passing arbitrary parameters from docker client to remote
driver is needed.
Many use cases require passing network configuration to a network driver
that might vary from container to container and might only be known at
container creation time. Examples are configuring the MTU or queue
length of a network interface or passing ip aliases, routes or iptables
rules for containers. This patch proposes adding a new --net-opt option
to docker to allow passing arbitrary information to network drivers.
net-opt options are used with docker run command and are directly copied
to the container's NetworkSettings and then passed to the network driver
on endpoint create.

Signed-off-by: Alina Quereilhac <alina@medallia.com>
… and ipam driver options

Syntax example:

docker run --network "name=network-name,key1=val1,key2=val2" \
	--ipam "name=network-name,key1=val1,key2=val2" --ip 10.1.0.2 alpine sh

Signed-off-by: Alina Quereilhac <alina@medallia.com>
@axqx
Copy link
Author

axqx commented Mar 3, 2017

@mavenugo, @aboch, @sanimej, @thaJeztah, the PR is now updated with the advanced --network/ipam syntax for docker run. Could you please take a look at the changes?

Some usage examples:

docker run -d --name test busybox top # same as now, will use default network

docker run -d --name test --net my-net busybox top # same as now, will use my-net network

docker run -d --name test --net "name=my-net" busybox top # will use my-net network

docker run -d --name test --net "name=my-net,key1=val1,key2=val2" busybox top # advanced syntax, will use my-net passing options

docker run -d --name test --net "name=my-net,key1=val1,key2=val2" --ipam "name=my-net,key3=val3,key4=val4" busybox top # advanced syntax, will use my-net passing options to network and ipam drivers

@geisbruch
Copy link

Hi, are you planning include this feature in the next release? It's very useful for custom driver. Today we should create many different networks to be hable to configure different ipam options on a custom driver.

@mavenugo
Copy link
Contributor

@axqx thanks for your effort on this.

--net "name=my-net,key1=val1,key2=val2"

This doesn't capture the requirement well. Since there are many more network specific options other than name and driver opts. PTAL #31964 for an idea of what the proposal will look like.

Ofcourse for this PR, you are worried only about --opt and --ipam-opt. But as a generic UX, we have to cover all the network specific options under this new CSV format.
cc @thaJeztah . How realistic is it for us to get this all wrapped up for 17.05 ?

@thaJeztah
Copy link
Member

@mavenugo good question, IIUC, changes will all be in the CLI/client, so could be realistic to get it in if a maintainer or contributor is able to reserve some time to implement it.

mcastelino added a commit to mcastelino/libnetwork that referenced this pull request Mar 22, 2017
docker supports alternate OCI runtimes including virtual machine
based runtimes. In certian cases network plugins can optionally
choose to support creation of  virtual machine friendly interfaces
using optional network options.

This is illustrated here with the hint being used by the macvlan
driver to create a macvtap interface vs a macvlan interface when
the runtime is known to be a VM based runtime.

docker run --runtime=cor -it --net=pub_net --network "name=pub_net,runtime=namespace" alpine sh

This is currently based off of
moby/moby#27638

However this will be implemented as per the proposal
moby/moby#31964

Signed-off-by: Manohar Castelino <manohar.r.castelino@intel.com>
@thaJeztah
Copy link
Member

Let me close this one for now, pending the changes for the --network flag (#31964). But @axqx if you have time and want to work on that, let me know!

Happy to reopen if you want to start working on this again

@thaJeztah thaJeztah closed this Apr 6, 2017
@axqx
Copy link
Author

axqx commented Apr 7, 2017

@thaJeztah, thanks for the update. Unfortunately I don't have time now to work on #31964, so I will wait until a stable form of the --network flag is achieved before resuming work on passing network options to drivers.

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