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

Added macvlan and ipvlan drivers #964

Merged
merged 2 commits into from
Mar 10, 2016
Merged

Added macvlan and ipvlan drivers #964

merged 2 commits into from
Mar 10, 2016

Conversation

nerdalert
Copy link
Contributor

import (
"testing"

"github.com/docker/libnetwork/driverapi"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add this import for the CI to pass:
_ "github.com/docker/libnetwork/testutils"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks fellas! @aboch @mavenugo that fixed it.

@mavenugo
Copy link
Contributor

@nerdalert 👏 thanks for the contribution. We will review it and provide feedback.

d.Lock()
networks := d.networks
d.Unlock()
n, ok := networks[nid]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need networks, we only need to lock around this line only,

d.Lock()
n, ok := d.networks[nid]
d.Unlock()

for label, value := range labels {
switch label {
case hostIfaceOpt:
// parse driver option '-o --ipvlan_mode'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment and the one below should be swapped.

- Notes at: https://gist.github.com/nerdalert/c0363c15d20986633fda

Signed-off-by: Brent Salisbury <brent@docker.com>
return nil
}
}
return fmt.Errorf("required kernel module '%s' was not found in /proc/modules, kernel version >= 3.19 is recommended", macvlanType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this true ?
I thought macvlan is supported in earlier kernels ?

@mavenugo
Copy link
Contributor

mavenugo commented Mar 2, 2016

@nerdalert Given the importance of getting feedback from users, shall we get these drivers under experimental for 1.11 and get enough feedback & also fix the L3 routing use-case. WDYT ?

@aboch
Copy link
Contributor

aboch commented Mar 2, 2016

Agree with @mavenugo. Being two new drivers, it will take some time with incremental changes to get them where we want them to be.
Introducing them in experimental seems the right approach to me as well.

@mavenugo
Copy link
Contributor

mavenugo commented Mar 3, 2016

@nerdalert I understand the need for -o host_iface option in order for the macvlan/ipvlan to plumb to underlay.

$ sudo docker network create -d macvlan test
ERRO[0008] Handler for POST /v1.22/networks/create returned error: macvlan requires an interface from the docker host to be specified (usage: -o host_iface=eth)
Error response from daemon: macvlan requires an interface from the docker host to be specified (usage: -o host_iface=eth)

But, as discussed,in the above case, if the user has not specified the host interface, we should consider it as an --internal case and automatically create a dummy interface and let the network creation successful. WDYT ?

@mavenugo
Copy link
Contributor

mavenugo commented Mar 3, 2016

@nerdalert few more comments as discussed. (commenting it here for better tracking)

  • Supporting --internal by using dummy interface. We can infact implement --internal and a configuration without -o host_iface similarly with the dummy interface approach.
  • Handle --mac-address option appropriately. It should error out in ipvlan but macvlan driver must accept and honor it.
  • Port-Mapping is infact a tricky scenario for these drivers. I would suggest a warning message and ignoring the option is better than Error'ing out (considering the portability concerns.
  • When multiple networks try to share the same parent interface, we should error it out with just the Netlink's device is busy message. But if the message doesnt contain the parent interface info, then it is better to customize it by indicating that the user is trying to reuse the parent interface.

@nerdalert
Copy link
Contributor Author

@mavenugo great suggestions, experimental sounds perfect.

  • --internal is now supported. If either the --internal flag is set or the --parent= interface is not included then a netlink dummy interface is setup to act as a master parent to attach a macvlan/ipvlan space device to. Once containers are brought up on that network they have full communications with one another but no where outside of the dummy parent interface.
  • Changed port-mapping back to a warning for compose backward compatibility as you mentioned.
  • Changed -o host_iface= to -o parent=.
  • One outstanding is setting the MAC addr on Macvlan mode. I will open an issue to track it. Netlink can set a user specified MAC since it is intended to be unique for each interface while Ipvlan uses the same MAC as the parent by design and tracks IP --> port mappings instead of MAC --> port mappings in it's lookup tables.
  • Added 50+ networking scenarios for vetting bugs in this script ipvlan-macvlan-it.sh that has example usage and capabilities.

}
if !parentExists(config.Parent) {
// if the parent iface was not specified, create a dummy link
if config.Parent == stringid.TruncateID(config.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to check for config.Internal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@mavenugo
Copy link
Contributor

mavenugo commented Mar 7, 2016

@nerdalert tried your patch e2e & am happy to see tagged use-case working across multi-host with appropriate configuration (in my OSX & virtual-box bridged adapter). These 2 drivers are really going to make the underlay integration extremely simple and I cant wait to get user feedback on these.

Apart from a few minor comments above, this PR gets a big 👍 from me.

@mavenugo
Copy link
Contributor

mavenugo commented Mar 7, 2016

@nerdalert I have a couple of macvlan networks with and without parent interfaces. When I tried to create and use an ipvlan network, I get the following error. Which seems wrong.

$ sudo docker network create -d ipvlan ip300
51ceb9c3f7877e674da65861eb8a8e8770fe6a36a66d705c4f51d6a4c51b6a98

$ sudo docker run --net=ip300 -it busybox sh
WARN[9635] failed to cleanup ipc mounts:
failed to umount /var/lib/docker/containers/53dece5c8166d90457f6270fe9018b37ccd1f9c5567cfa09f45005d9734a2965/shm: no such file or directory
ERRO[9635] Handler for POST /v1.22/containers/53dece5c8166d90457f6270fe9018b37ccd1f9c5567cfa09f45005d9734a2965/start returned error: Failed creating veth2db7a06, ensure there are no macvlan networks using the same `-o parent`: operation not supported
docker: Error response from daemon: Failed creating veth2db7a06, ensure there are no macvlan networks using the same `-o parent`: operation not supported.

Also the same issue when I tried to create an entirely new parent interface

$ sudo docker network create -d ipvlan -o parent=eth3.345 ip345
df831819e495cd0d561bc6cd1f84afdd887acdbced4c2bfdabffcbbbe02cbbcf

$ sudo docker run --net=ip345 -it busybox sh
WARN[9809] failed to cleanup ipc mounts:
failed to umount /var/lib/docker/containers/0064e630b9f4df3902c04796ba23f63d7b0b1aa60567082e092caf09512f477c/shm: no such file or directory
ERRO[9809] Handler for POST /v1.22/containers/0064e630b9f4df3902c04796ba23f63d7b0b1aa60567082e092caf09512f477c/start returned error: Failed creating vethb59a154, ensure there are no macvlan networks using the same `-o parent`: operation not supported
docker: Error response from daemon: Failed creating vethb59a154, ensure there are no macvlan networks using the same `-o parent`: operation not supported.

Can you pls take a look @ it ?

@fredhsu
Copy link

fredhsu commented Mar 7, 2016

I've been running some tests with an Arista ToR acting as the gateway for macvlan/ipvlan, with and without 802.1q. So far so good!

macvlan bridge mode:
https://gist.github.com/fredhsu/63344b94fc18c87f8ed2

ipvlan L2 mode with 802.1q:
https://gist.github.com/fredhsu/38d75e7651dae8506ba2

Let me know if there are any specific tests you would like to have run against physical hardware.

@nerdalert
Copy link
Contributor Author

Thanks @fredhsu appreciate you testing the physical side of the equation.

@nerdalert
Copy link
Contributor Author

Regarding the operation not supported. was kernel versions < 3.19. Rather then warning in driver init(), it now fails network creates for macvlan < 3.09 and ipvlan < 3.19.

@mavenugo
Copy link
Contributor

mavenugo commented Mar 8, 2016

Thanks @nerdalert.

LGTM

@aboch
Copy link
Contributor

aboch commented Mar 9, 2016

LGTM

ping @chenchun

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

👍 Introducing them in experimental.

LGTM, thanks for your work @nerdalert

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.

6 participants