-
Notifications
You must be signed in to change notification settings - Fork 228
Fix CNI networking issues (MAC duplication + bridge ageing) #638
Conversation
Signed-off-by: Dennis Marttinen <dennis@weave.works>
Signed-off-by: Dennis Marttinen <dennis@weave.works>
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.
Excellent work 🎉🎉✨! Thank you very much
LGTM!
@@ -8,6 +8,9 @@ replace ( | |||
k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20200121204235-bf4fb3bd569c | |||
) | |||
|
|||
// TODO: Remove this when https://github.com/vishvananda/netlink/pull/554 is merged |
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 can comment go.mod??? TIL, thx 🤯
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.
It ought to be possible, go.mod
files seem to just be regular go sources with some special sauce builtins.
go.mod
Outdated
@@ -8,6 +8,9 @@ replace ( | |||
k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20200121204235-bf4fb3bd569c | |||
) | |||
|
|||
// TODO: Remove this when https://github.com/vishvananda/netlink/pull/554 is merged | |||
replace github.com/vishvananda/netlink => github.com/twelho/netlink v1.1.1-0.20200709104403-94a3428ca41d |
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.
Maybe you could tag your repo to sth like v1.1.2-rc.1?
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 can do that, I'll just need come up with a tag that doesn't hinder upstream's versioning conventions.
pkg/container/network.go
Outdated
|
||
if handle, err = netlink.NewHandle(); err != nil { | ||
if eth, err = netlink.LinkByIndex(iface.Index); err != nil { |
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.
nit: I prefer := on the line before errcheck and no var declaration
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.
This is using named returns, if I do that then it won't assign the error to the named err
but create a new block-specific one, and that's a bug. But I'll refactor this to not use named returns, they don't benefit this function anymore.
|
||
// getMAC fetches the generated MAC address for the given link | ||
func getMAC(link netlink.Link) (addr net.HardwareAddr, err error) { | ||
if link, err = netlink.LinkByIndex(link.Attrs().Index); err != nil { |
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.
Why are you "overwriting" this variable?
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.
The passed link is the "constructor" and has no hardware address since we want it to be generated by the network subsystem, so I need to "reload" the link by requesting the properties from the system after it has been created, which fills all the fields. I'll add a comment to avoid confusion.
Signed-off-by: Dennis Marttinen <dennis@weave.works>
This implements downstream fixes for the two issues mentioned in #633. It
netlink
(upstream PR: Add support for configuring bridge ageing time vishvananda/netlink#554) to enable setting the bridge ageing time to 0.With this I'm able to connect VMs to Flannel and have networking working out of the box.
Resolves #633, resolves #628, closes #634.
cc @luxas, @stealthybox, @darkowlzz