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.

Sign up for GitHub

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

Better interface selection heuristic #3546

Merged
merged 4 commits into from
Nov 15, 2017
Merged

Better interface selection heuristic #3546

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35cb143
Better interface selection heuristic
dadgar Nov 13, 2017
00557af
Use interface attached to default route
dadgar Nov 15, 2017
8ebca4b
use revised wording
dadgar Nov 15, 2017
6a434bc
changelog
dadgar Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 49 additions & 7 deletions client/fingerprint/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ func (n *NetworkFingerprint) isDeviceLoopBackOrPointToPoint(intf *net.Interface)
// and finds one which is routable and marked as UP
// It excludes PPP and lo devices unless they are specifically asked
func (f *NetworkFingerprint) findInterface(deviceName string) (*net.Interface, error) {
var interfaces []net.Interface
var err error

if deviceName != "" {
Expand All @@ -200,14 +199,57 @@ func (f *NetworkFingerprint) findInterface(deviceName string) (*net.Interface, e
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the comments on the PR indicated that the returned list here is not stable (can return things out of order). What happens if there is more than one interface with a global unicast address, thus resulting in them getting the same score?
Reading the code below, it seems like it will choose the first one, but if the order changes when calling interfaceDetector.Interfaces() that will change the results.

}

// Since we aren't given an interface to use we have to pick using a
// heuristic. To choose between interfaces we avoid down interfaces, those
// that are loopback, point to point, or don't have an address. Of the
// remaining interfaces we rank them positively for each global unicast
// address and slightly negatively for a link local address. This makes
// us select interfaces that have more globally routable address over those
// that do not.
var bestChoice *net.Interface
bestScore := -1000
for _, intf := range intfs {
if f.isDeviceEnabled(&intf) && !f.isDeviceLoopBackOrPointToPoint(&intf) && f.deviceHasIpAddress(&intf) {
interfaces = append(interfaces, intf)
// We require a non-loopback interface that is enabled with a valid
// address.
if !f.isDeviceEnabled(&intf) || f.isDeviceLoopBackOrPointToPoint(&intf) || !f.deviceHasIpAddress(&intf) {
continue
}

addrs, err := f.interfaceDetector.Addrs(&intf)
if err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

🤔 We could WARN and continue here, but after peeking at the Addrs(...) code it seems like all of the errors it returns indicate pretty bad syscall failures. Seems like just bailing is probably safest as users can manually set this anyway.

}
}

if len(interfaces) == 0 {
return nil, nil
seenLinkLocal := false
intfScore := 0
for _, addr := range addrs {
// Find the IP Addr and the CIDR from the Address
var ip net.IP
switch v := (addr).(type) {
case *net.IPNet:
ip = v.IP
case *net.IPAddr:
ip = v.IP
}

if !seenLinkLocal && (ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast()) {
intfScore -= 1
seenLinkLocal = true
}

if ip.IsGlobalUnicast() {
intfScore += 2
}
}

if intfScore > bestScore {
Copy link
Contributor

@preetapan preetapan Nov 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If infscore stays at zero and bestScore at -1000 this will pick the first intf from the intfs list, is that acceptable?

// Make a copy on the stack so we grab a pointer to the correct
// interface
local := intf
bestChoice = &local
bestScore = intfScore
}
}
return &interfaces[0], nil

return bestChoice, nil
}
7 changes: 5 additions & 2 deletions client/fingerprint/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ type NetworkInterfaceDetectorMultipleInterfaces struct {
}

func (n *NetworkInterfaceDetectorMultipleInterfaces) Interfaces() ([]net.Interface, error) {
return []net.Interface{lo, eth0, eth1, eth2}, nil
// Return link local first to test we don't prefer it
return []net.Interface{eth3, lo, eth0, eth1, eth2, eth4}, nil
}

func (n *NetworkInterfaceDetectorMultipleInterfaces) InterfaceByName(name string) (*net.Interface, error) {
Expand Down Expand Up @@ -301,7 +302,9 @@ func TestNetworkFingerPrint_default_device(t *testing.T) {
}
}

func TestNetworkFingerPrint_excludelo_down_interfaces(t *testing.T) {
// This tests that we prefer skipping link local and down interfaces as well as
// those without global unicast addresses
func TestNetworkFingerPrint_preferential_interfaces(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how any of these tests are exercising the new code path with the scoring introduced above.
I realize its not possible to test in a portable way across machines, but I would try to refactor the code so the scoring/selection logic is testable

f := &NetworkFingerprint{logger: testLogger(), interfaceDetector: &NetworkInterfaceDetectorMultipleInterfaces{}}
node := &structs.Node{
Attributes: make(map[string]string),
Expand Down