Fix a race condition in getting the trunk name via its mac #15
Conversation
network/eni/eni.go
Outdated
| // getInterfaceByMac gets the interface with specified mac address, and picks the one with shortest name | ||
| // if there are multiple matches, which can happen temporarily when setting up a branch ENI, in which it | ||
| // can have the same mac address as the trunk during branch.AttachToLink. | ||
| func getInterfaceByMac(macAddress net.HardwareAddr, interfaces []net.Interface) *net.Interface { |
There was a problem hiding this comment.
Can you please call this getInterfaceByMACAddress, to comply with the Go naming conventions? Also please move to the end of the file to keep the ENI class methods together.
network/eni/eni.go
Outdated
| @@ -102,6 +97,27 @@ func (eni *ENI) AttachToLink() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // getInterfaceByMac gets the interface with specified mac address, and picks the one with shortest name | |||
There was a problem hiding this comment.
Let's not discuss the trunk vs. branch MAC address here. The ENI class doesn't know about trunks etc.
// getInterfaceByMACAddress returns the interface with the specified MAC address.
Move the rest of your comment block to just before the for loop (line 106):
// If there are multiple matches, pick the one with the shortest name.
network/eni/eni.go
Outdated
| // if there are multiple matches, which can happen temporarily when setting up a branch ENI, in which it | ||
| // can have the same mac address as the trunk during branch.AttachToLink. | ||
| func getInterfaceByMac(macAddress net.HardwareAddr, interfaces []net.Interface) *net.Interface { | ||
| var chosenInterface net.Interface |
There was a problem hiding this comment.
If you define this as a pointer (var chosen *net.Interface), then
the if check at line 108 can become simply
if chosen == nil || len(chosen.Name) > len(i.Name)
line 109 becomes a pointer copy instead of a struct copy
you can remove the if block lines 114-116.
There was a problem hiding this comment.
i originally did that. then i found the variable "i" in the for loop shares the same address, so i can't just save the address.
There was a problem hiding this comment.
discussed offline. fixed.
network/eni/eni.go
Outdated
| break | ||
| } | ||
| } | ||
| iface = getInterfaceByMac(eni.macAddress, interfaces) |
There was a problem hiding this comment.
Should we move the net.Interfaces() call to the new getInterfaceByMACAddress function? That way we wouldn't have to pass it as an argument.
There was a problem hiding this comment.
that makes it hard to add unit test i think..
| Index: 3, | ||
| Name: "eth1.1.1", | ||
| HardwareAddr: mac, | ||
| }, |
There was a problem hiding this comment.
Thanks for adding tests. For testing an interesting corner case, you can add one more interface here with a three letter name but a different MAC address. To make sure MAC address match is prioritized.
| @@ -0,0 +1,33 @@ | |||
| package eni | |||
There was a problem hiding this comment.
Should we add the copyright header to the top of the file?
fenxiong
force-pushed
the
fix-name
branch
2 times, most recently
from
c83e765
to
fda2912
Compare
During setting up a branch interface, it can have the same mac address as the trunk for a short period of time, and we may end up picking the branch's name when trying to get the trunk name by mac. Fixing it by picking the shortest name available since branch name is strictly longer than trunk.
During setting up a branch interface, the branch can have the same mac address as the trunk for a short period of time, and we may end up picking the branch's name when trying to get the trunk name by mac. Fixing it by picking the shortest name available since branch name is strictly longer than trunk.
Unit test added and passed. The e2e tests via vpc-branch-eni-e2e-tests target passed. Manually reproduced the race with unfixed version and verified that it's fixed with this pr.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.