diff --git a/director/sort.go b/director/sort.go index 1192a47de..ae3ac8822 100644 --- a/director/sort.go +++ b/director/sort.go @@ -68,6 +68,13 @@ const ( objAvailabilityFactor = 2 // Multiplier for knowing whether an object is present loadHalvingThreshold = 10.0 // Threshold where the load havling factor kicks in loadHalvingFactor = 4.0 // Halving interval for load + + // A rough lat/long bounding box for the contiguous US. We might eventually make this box + // a configurable value, but for now it's hardcoded + usLatMin = 30.0 + usLatMax = 50.0 + usLongMin = -125.0 + usLongMax = -65.0 ) func (me SwapMaps) Len() int { @@ -173,12 +180,31 @@ func getLatLong(addr netip.Addr) (lat float64, long float64, err error) { return } -func getClientLatLong(addr netip.Addr) (coord Coordinate, ok bool) { +// Given a bounding box, assign a random coordinate within that box. +func assignRandBoundedCoord(minLat, maxLat, minLong, maxLong float64) (lat, long float64) { + lat = rand.Float64()*(maxLat-minLat) + minLat + long = rand.Float64()*(maxLong-minLong) + minLong + return +} + +// Given a client address, attempt to get the lat/long of the client. If the address is invalid or +// the lat/long is not resolvable, assign a random location in the contiguous US. +func getClientLatLong(addr netip.Addr) (coord Coordinate) { var err error + if !addr.IsValid() { + log.Warningf("Unable to sort servers based on client-server distance. Invalid client IP address: %s", addr.String()) + coord.Lat, coord.Long = assignRandBoundedCoord(usLatMin, usLatMax, usLongMin, usLongMax) + log.Warningf("Assigning random location in the contiguous US to lat/long %f, %f ", coord.Lat, coord.Long) + return + } + coord.Lat, coord.Long, err = getLatLong(addr) - ok = (err == nil && !(coord.Lat == 0 && coord.Long == 0)) - if err != nil { - log.Warningf("failed to resolve lat/long for address %s: %v", addr, err) + if err != nil || (coord.Lat == 0 && coord.Long == 0) { + if err != nil { + log.Warningf("Error while getting the client IP address: %v", err) + } + coord.Lat, coord.Long = assignRandBoundedCoord(usLatMin, usLatMax, usLongMin, usLongMax) + log.Warningf("Client IP %s has been re-assigned a random location in the contiguous US to lat/long %f, %f ", addr.String(), coord.Lat, coord.Long) } return } @@ -192,7 +218,7 @@ func invertWeightIfNeeded(isRand bool, weight float64) float64 { return weight } -// The all-in-one method to sort serverAds based on Dirtector.CacheSortMethod configuration parameter +// The all-in-one method to sort serverAds based on the Director.CacheSortMethod configuration parameter // - distance: sort serverAds by the distance between the geolocation of the servers and the client // - distanceAndLoad: sort serverAds by the distance with gated halving factor (see details in the adaptive method) // and the server IO load @@ -207,15 +233,8 @@ func sortServerAds(clientAddr netip.Addr, ads []server_structs.ServerAd, availab // A larger weight is a higher priority. weights := make(SwapMaps, len(ads)) sortMethod := param.Director_CacheSortMethod.GetString() - - clientCoord := Coordinate{} - clientCoordOk := false - - if clientAddr.IsValid() { - clientCoord, clientCoordOk = getClientLatLong(clientAddr) - } else { - log.Warningf("Unable to sort servers based on client-server distance. Invalid client IP address: %s", clientAddr.String()) - } + // This will handle the case where the client address is invalid or the lat/long is not resolvable. + clientCoord := getClientLatLong(clientAddr) // For each ad, we apply the configured sort method to determine a priority weight. for idx, ad := range ads { @@ -232,20 +251,16 @@ func sortServerAds(clientAddr netip.Addr, ads []server_structs.ServerAd, availab case server_structs.DistanceAndLoadType: weight := 1.0 // Distance weight - var distance float64 - var isRand bool - if clientCoordOk { - distance, isRand = distanceWeight(clientCoord.Lat, clientCoord.Long, ad.Latitude, ad.Longitude, true) - if isRand { - // In distanceAndLoad/adaptive modes, pin random distance weights to the range [-0.475, -0.525)] in an attempt - // to make sure the weights from availability/load overpower the random distance weights while - // still having a stochastic element. We do this instead of ignoring the distance weight entirely, because - // it's possible load information and or availability information is not available for all servers. - weight = 0 - (0.475+rand.Float64())*(0.05) - } else { - dWeighted := gatedHalvingMultiplier(distance, distanceHalvingThreshold, distanceHalvingFactor) - weight *= dWeighted - } + distance, isRand := distanceWeight(clientCoord.Lat, clientCoord.Long, ad.Latitude, ad.Longitude, true) + if isRand { + // In distanceAndLoad/adaptive modes, pin random distance weights to the range [-0.475, -0.525)] in an attempt + // to make sure the weights from availability/load overpower the random distance weights while + // still having a stochastic element. We do this instead of ignoring the distance weight entirely, because + // it's possible load information and or availability information is not available for all servers. + weight = 0 - (0.475+rand.Float64())*(0.05) + } else { + dWeighted := gatedHalvingMultiplier(distance, distanceHalvingThreshold, distanceHalvingFactor) + weight *= dWeighted } // Load weight @@ -255,17 +270,14 @@ func sortServerAds(clientAddr netip.Addr, ads []server_structs.ServerAd, availab case server_structs.AdaptiveType: weight := 1.0 // Distance weight - var distance float64 - var isRand bool - if clientCoordOk { - distance, isRand = distanceWeight(clientCoord.Lat, clientCoord.Long, ad.Latitude, ad.Longitude, true) - if isRand { - weight = 0 - (0.475+rand.Float64())*(0.05) - } else { - dWeighted := gatedHalvingMultiplier(distance, distanceHalvingThreshold, distanceHalvingFactor) - weight *= dWeighted - } + distance, isRand := distanceWeight(clientCoord.Lat, clientCoord.Long, ad.Latitude, ad.Longitude, true) + if isRand { + weight = 0 - (0.475+rand.Float64())*(0.05) + } else { + dWeighted := gatedHalvingMultiplier(distance, distanceHalvingThreshold, distanceHalvingFactor) + weight *= dWeighted } + // Availability weight if availabilityMap == nil { weight *= 1.0 diff --git a/director/sort_test.go b/director/sort_test.go index dd03c223d..14087cc89 100644 --- a/director/sort_test.go +++ b/director/sort_test.go @@ -21,6 +21,7 @@ package director import ( "bytes" _ "embed" + "math" "math/rand" "net" "net/netip" @@ -420,3 +421,52 @@ func TestSortServerAdsByAvailability(t *testing.T) { sortServerAdsByAvailability(randomOrder, avaiMap) assert.EqualValues(t, expected, randomOrder) } + +func TestAssignRandBoundedCoord(t *testing.T) { + // Because of the test's randomness, do it a few times to increase the likelihood of catching errors + for i := 0; i < 10; i++ { + // Generate a random bounding box between -200, 200 + lat1 := rand.Float64()*400 - 200 + long1 := rand.Float64()*400 - 200 + lat2 := rand.Float64()*400 - 200 + long2 := rand.Float64()*400 - 200 + + // Assign mins and maxes + minLat, maxLat := math.Min(lat1, lat2), math.Max(lat1, lat2) + minLong, maxLong := math.Min(long1, long2), math.Max(long1, long2) + + // Assign a random coordinate within the bounding box + lat, long := assignRandBoundedCoord(minLat, maxLat, minLong, maxLong) + assert.True(t, lat >= minLat && lat <= maxLat) + assert.True(t, long >= minLong && long <= maxLong) + } +} + +func TestGetClientLatLong(t *testing.T) { + t.Run("invalid-ip", func(t *testing.T) { + // Capture the log and check that the correct error is logged + logOutput := &(bytes.Buffer{}) + log.SetOutput(logOutput) + log.SetLevel(log.DebugLevel) + + clientIp := netip.Addr{} + coord := getClientLatLong(clientIp) + + assert.True(t, coord.Lat <= usLatMax && coord.Lat >= usLatMin) + assert.True(t, coord.Long <= usLongMax && coord.Long >= usLongMin) + assert.Contains(t, logOutput.String(), "Unable to sort servers based on client-server distance. Invalid client IP address") + }) + + t.Run("valid-ip-no-geoip-match", func(t *testing.T) { + logOutput := &(bytes.Buffer{}) + log.SetOutput(logOutput) + log.SetLevel(log.DebugLevel) + + clientIp := netip.MustParseAddr("192.168.0.1") + coord := getClientLatLong(clientIp) + + assert.True(t, coord.Lat <= usLatMax && coord.Lat >= usLatMin) + assert.True(t, coord.Long <= usLongMax && coord.Long >= usLongMin) + assert.Contains(t, logOutput.String(), "Client IP 192.168.0.1 has been re-assigned a random location in the contiguous US to lat/long") + }) +}