replace macvlan with veth pairs #2414
Conversation
Co-authored-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>
Omarabdul3ziz
requested review from
muhamadazmy,
delandtj,
MarioBassem,
ashraffouda,
rawdaGastan and
xmonader
as code owners
pkg/network/networker.go
Outdated
| name := filepath.Base(netNs.Path()) | ||
| peerPrefix := name | ||
| if len(name) > 4 { | ||
| peerPrefix = name[0:4] | ||
| } | ||
|
|
||
| if !ifaceutil.Exists(ZDBYggIface, netNs) { | ||
| yggLink, err := ifaceutil.MakeVethPair(ZDBYggIface, types.YggBridge, 1500, peerPrefix) | ||
| if err != nil { | ||
| return net.IPNet{}, fmt.Errorf("failed to create ygg link: %w", err) | ||
| } | ||
|
|
||
| err = netlink.LinkSetNsFd(yggLink, int(netNs.Fd())) | ||
| if err != nil { | ||
| return net.IPNet{}, fmt.Errorf("failed to move ygg link: %s to namespace:%s : %w", ZDBYggIface, netNs.Path(), err) | ||
| } | ||
|
|
||
| if err := netNs.Do(func(_ ns.NetNS) error { | ||
| err = setLinkAddr(ZDBYggIface, &ip) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := ifaceutil.SetLoUp(); err != nil { | ||
| return fmt.Errorf("failed to set lo up for namespace '%s': %w", name, err) | ||
| } | ||
|
|
||
| if err := options.SetIPv6Forwarding(true); err != nil { | ||
| return fmt.Errorf("failed to enable ipv6 forwarding in namespace %q: %w", name, err) | ||
| } | ||
|
|
||
| return netlink.RouteAdd(&route) | ||
| }); err != nil { | ||
| return net.IPNet{}, fmt.Errorf("failed to setup link addresses %s: %w", ZDBYggIface, err) | ||
| } |
There was a problem hiding this comment.
Looks like this code block does the same thing as https://github.com/threefoldtech/zos/pull/2414/files#diff-4e15b6f2760eb52c8292eadf92ce4a6ba3afcf08d34b528d56081b9bc1623554R252-R285
extract it to a func?
Co-authored-by: Ashraf Fouda <ashraf.m.fouda@gmail.com>
pkg/network/ifaceutil/interface.go
Outdated
| @@ -179,6 +217,40 @@ func MakeVethPair(name, master string, mtu int) (netlink.Link, error) { | |||
| return veth2, nil | |||
| } | |||
|
|
|||
| // moveLinkToNamespace moves the veth link to the specified namespace and renames it | |||
| func moveLinkToNamespace(veth netlink.Link, tmpName, finalName string, netNs ns.NetNS, peerLink netlink.Link) (netlink.Link, error) { | |||
There was a problem hiding this comment.
i suggest we make tmpName > name the underlying method shouldn't know about tmpName I think
and finalName > renameTo which would be a pointer because it maybe nil if we need to just move it to the name space
pkg/network/networker.go
Outdated
| return fmt.Errorf("failed to set link address: %w", err) | ||
| } | ||
|
|
||
| // if err := options.Set(name, options.IPv6Disable(false)); err != nil { |
There was a problem hiding this comment.
remove unneeded commented coder
pkg/network/networker.go
Outdated
| return netlink.LinkSetUp(link) | ||
| } | ||
|
|
||
| func createInterface(ifName, bridge string, netNs ns.NetNS, ip *net.IPNet, route *netlink.Route) error { |
There was a problem hiding this comment.
rename it to CreateVethInterface
- create the tmp name inside the veth creation - rename the function/argument
There was a problem hiding this comment.
thanks @Omarabdul3ziz for the good work :) I left few comments below that might be helpful
pkg/network/ifaceutil/interface.go
Outdated
| return nil, fmt.Errorf("master link: %s not found: %v", master, err) | ||
| } | ||
|
|
||
| nsName := filepath.Base(netNs.Path()) |
There was a problem hiding this comment.
What if netNs is nil I see you already calling the function with nil as a last argument already in some places
pkg/network/ifaceutil/interface.go
Outdated
| tmpName, err := ip.RandomVethName() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| ifName := name | ||
| if netNs != nil { | ||
| ifName = tmpName | ||
| } |
There was a problem hiding this comment.
Would be easier to read if it's re-written like
ifName := name
if netNs != nil {
ifName, err = ip.RandomVethName()
if err != nil {
return nil, err
}
}
pkg/network/ifaceutil/interface.go
Outdated
| // either from the host, or from the namespace | ||
| var veth2 netlink.Link | ||
|
|
||
| veth2, err = netlink.LinkByName(name) |
There was a problem hiding this comment.
Does a link with name name even exist here? Note that so far the 2 links that exists are ifName and peer, not name!
Also veth2 is not a good varible name. i don't know what you refer too.
| if err := netlink.LinkSetUp(peerLink); err != nil { | ||
| return nil, fmt.Errorf("failed to set veth peer up: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't believe this function should return anything because the veth can be inside a namespace. So returning a link that is inside a namespace does not make any sense. Normally to work with links that lives inside a namespace you have to do
ns.Run(func() {
// load the nic here
// work with the nic
}
pkg/network/networker.go
Outdated
| }, | ||
| Gw: gw.IP, | ||
| // LinkIndex:... this is set by macvlan.Install | ||
| route := netlink.Route{ |
There was a problem hiding this comment.
I think even if this now has only a single route. It's important to understand that this won't always be the case. It's totally legal to configure multiple routes and taking away this ability is not recommended.
- refetch the created link when needed instead of returning it from the creation method - organize and enhance naming
- validate the peer absence
Description
replace macvlan with veth pairs
Related Issues
List of related issues
Checklist