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

Pass container labels in endpoint create & join #13476

Closed
wants to merge 1 commit into from

Conversation

mavenugo
Copy link
Contributor

We need this patch in order to support the long standing request :
moby/libnetwork#161

Signed-off-by: Madhu Venugopal madhu@docker.com

@thaJeztah
Copy link
Member

Just for reference, this relates to #6743

@calavera
Copy link
Contributor

LGTM.

@mrjana
Copy link
Contributor

mrjana commented May 26, 2015

LGTM

@icecrime
Copy link
Contributor

I'm temporary NOT LGTM until I give this more thinking. Especially, using labels as a vehicle for driver specific network options makes me feel like a LABEL instruction in a Dockerfile could impact the running environment of a container down the road.

@mavenugo
Copy link
Contributor Author

@icecrime the idea is to send the container labels to libnetwork core and have the core filter out the labels based on the namespace the label belong to. For example, we should not pass all the labels to every driver. But if there is a driver named foo with a registered label namespace com.foo.xyz that is supposed to perform a specified action and if the user-label's prefix matches that particular driver's label-namespace, then that driver will get that specific label.

For any well-known labels supported by libnetwork core (namespaced by com.docker.libnetwork, we can filter them out entirely from the call to the driver & use well-known (non-label-based) configuration mechanism to pass the information to driver if required.

Does that address your concern ?

I see this as a genuine use-case for labels and provides flexibility in Docker core to not overload with many use-case specific configurations.

@cpuguy83
Copy link
Member

@mavenugo Yes, but users can pass in labels in a Dockerfile. This would in effect allow a Dockerfile writer to affect the networking of containers created from it.
Images should not be encoded with this information.

@mavenugo
Copy link
Contributor Author

@cpuguy83 @icecrime okay. will it be okay if we filter out the image labels from this list and make it purely a container labels driven operation ?

@icecrime
Copy link
Contributor

I have to say I'm uncomfortable, I just don't feel labels are the proper vehicle for that. It's one thing to say the container is blue, and a totally different thing that it should be in this or that subnet.

Furthermore, I could imagine actually being useful at some point in Docker to have a "rule mechanism" to bind label "blue" with "subnet X" (thus properly decoupling the build time and run time considerations). With this patch in, we make it impossible.

@mavenugo
Copy link
Contributor Author

@icecrime in the new CNM model, container/sandbox itself is not specified with the ip-address. Rather it is the service endpoint that carries this info. Containers can freely attach and detach to the service endpoint (as explained by Solomon in #13441.
I guess the reason you are uncomfortable is that we are passing the label to the service endpoint that is created by default for a container during the docker run for convenience. Lets discuss this further in the context of #13441 and see if we can agree on a solution.

We need this patch in order to support the long standing request :
moby/libnetwork#161

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

mavenugo commented Jun 3, 2015

ping @icecrime just trying to find a way to allow the users to get the functionality that I explained earlier. Also, since the container labels are made available via the APIs,... drivers (or) any other external system is free to get the exact same info without a more formal approach as defined in this PR.
Should we take the same approach as --mac-address and go with a more static and well-defined option such as --ip to set static IP instead of labels ?

@shykes
Copy link
Contributor

shykes commented Jun 5, 2015

I think I agree with @icecrime, I would prefer keeping labels portable and user-specific (eg. color:blue, prod:true), and have out-of-band matching of container label to driver config. That matching could be done in the driver itself or, even better, as a configurable feature of Docker. So a sysadmin could configure the daemon with css-like selectors eg. "if color == blue && prod == true { drivercfg.netmask = 10.42.42.0/24"

We could make that selector a lua scripttoo, would be cool :)

Also: image labels should NOT translate to container labels. They are not the same thing.

@duglin
Copy link
Contributor

duglin commented Jun 5, 2015

I'm not sure where this PR is headed but if it does head down the "merge" path can we please get some testcases? With the recent regressions that have been found (e.g. by @ibuildthecloud ) I think we need to push harder for more testing - especially w.r.t. what the users sees. I love how @vdemeester is working hard on adding tests for the existing code!

@mavenugo
Copy link
Contributor Author

mavenugo commented Jun 6, 2015

@shykes @icecrime one of the main reasons of opening this PR was to support a request that has come up multiple times on static IP Address assignment to a container by the user via the docker run command. It is certainly not declarative. #6743, moby/libnetwork#161 and others.

We thought of using Labels for this because we were not inclined towards introducing yet another static flag with a particular semantics assigned to it such as --ip or --ipv6 and make it docker engine managed flag. Label makes it a contract between the Driver and the user and the Engine itself is not directly involved.

Coming back to the declarative & out-of-band matching example : "if color == blue && prod == true { drivercfg.netmask = 10.42.42.0/24", how exactly will the driver get hold of these declarative labels ?
The example of assigning an IP to a container is real-time and an out-of-band matching may not work.

At this point, I think it would be better to keep these discussions separate and give a simpler and direct answer to the user requests for assigning IP/IPv6 addresses. Once we unblock that, we can have more indepth discussion on making a declarative approach and eventually a scriptable interface as you suggested.

@icecrime
Copy link
Contributor

What do we do regarding this PR? Was there out-of-band discussion that lead to some conclusion or is it still floating? Thanks!

@icecrime
Copy link
Contributor

Closing it as it seems the conversation is really about specifying IP and MAC addresses now, not about labels anymore.

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.

9 participants