-
Notifications
You must be signed in to change notification settings - Fork 881
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
Conversation
import ( | ||
"testing" | ||
|
||
"github.com/docker/libnetwork/driverapi" |
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.
You need to add this import for the CI to pass:
_ "github.com/docker/libnetwork/testutils"
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.
@nerdalert 👏 thanks for the contribution. We will review it and provide feedback. |
d.Lock() | ||
networks := d.networks | ||
d.Unlock() | ||
n, ok := networks[nid] |
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.
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' |
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 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) |
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.
Is this true ?
I thought macvlan is supported in earlier kernels ?
@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 ? |
Agree with @mavenugo. Being two new drivers, it will take some time with incremental changes to get them where we want them to be. |
@nerdalert I understand the need for
But, as discussed,in the above case, if the user has not specified the host interface, we should consider it as an |
@nerdalert few more comments as discussed. (commenting it here for better tracking)
|
@mavenugo great suggestions, experimental sounds perfect.
|
} | ||
if !parentExists(config.Parent) { | ||
// if the parent iface was not specified, create a dummy link | ||
if config.Parent == stringid.TruncateID(config.ID) { |
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 think it is better to check for config.Internal
here.
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.
Done!
@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. |
@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.
Also the same issue when I tried to create an entirely new parent interface
Can you pls take a look @ it ? |
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: ipvlan L2 mode with 802.1q: Let me know if there are any specific tests you would like to have run against physical hardware. |
Thanks @fredhsu appreciate you testing the physical side of the equation. |
Regarding the |
Thanks @nerdalert. LGTM |
LGTM ping @chenchun |
Signed-off-by: Madhu Venugopal <madhu@docker.com>
👍 Introducing them in experimental. LGTM, thanks for your work @nerdalert |
Added macvlan and ipvlan drivers
Signed-off-by: Brent Salisbury brent@docker.com