Return 0 records if all are unhealthy

(The previous commits had the health checks not work at all, I think)

- Reformat Picker() a bit so it's not wrapped in one big if{}
- Update a few comments

server/metrics.go

@@ -7,8 +7,6 @@ import (
 	metrics "github.com/rcrowley/go-metrics"
 )
 
-// todo: make this not have global variables ...
-
 type ServerMetrics struct {
 	qCounter         metrics.Meter
 	lastQueryCount   int64

server/serve.go

@@ -240,7 +240,8 @@ func (srv *Server) serve(w dns.ResponseWriter, req *dns.Msg, z *zones.Zone) {
 		}
 		if len(m.Answer) > 0 {
 			// maxHosts only matter within a "targeting group"; at least that's
-			// how it has been working
+			// how it has been working, so we stop looking for answers as soon
+			// as we have some.
 
 			if qle != nil {
 				qle.LabelName = label.Label

zones/picker.go

@@ -1,7 +1,6 @@
 package zones
 
 import (
-	"log"
 	"math/rand"
 
 	"github.com/abh/geodns/health"
@@ -10,19 +9,18 @@ import (
 	"github.com/miekg/dns"
 )
 
-func (zone *Zone) filterHealth(servers Records) int {
+func (zone *Zone) filterHealth(servers Records) (Records, int) {
 	// Remove any unhealthy servers
 	tmpServers := servers[:0]
-	sum := 0
 
+	sum := 0
 	for i, s := range servers {
 		if len(servers[i].Test) == 0 || zone.HealthStatus.GetStatus(servers[i].Test) == health.StatusHealthy {
 			tmpServers = append(tmpServers, s)
 			sum += s.Weight
 		}
 	}
-	servers = tmpServers
-	return sum
+	return tmpServers, sum
 }
 
 func (zone *Zone) Picker(label *Label, qtype uint16, max int, location *targeting.Location) Records {
@@ -43,110 +41,112 @@ func (zone *Zone) Picker(label *Label, qtype uint16, max int, location *targetin
 		return result
 	}
 
-	if labelRR := label.Records[qtype]; labelRR != nil {
+	labelRR := label.Records[qtype]
+	if labelRR == nil {
+		// we don't have anything of the correct type
+		return nil
 
-		sum := label.Weight[qtype]
+	}
 
-		servers := make(Records, len(labelRR))
-		copy(servers, labelRR)
+	sum := label.Weight[qtype]
 
-		if label.Test != nil {
-			sum = zone.filterHealth(servers)
-			if sum == 0 {
-				return servers
-			}
-		}
+	servers := make(Records, len(labelRR))
+	copy(servers, labelRR)
 
-		// not "balanced", just return all -- It's been working
-		// this way since the first prototype, it might not make
-		// sense anymore. This probably makes NS records and such
-		// work as expected.
-		if label.Weight[qtype] == 0 {
+	if label.Test != nil {
+		servers, sum = zone.filterHealth(servers)
+		if sum == 0 {
 			return servers
 		}
+	}
 
-		if qtype == dns.TypeCNAME || qtype == dns.TypeMF {
-			max = 1
-		}
+	// not "balanced", just return all -- It's been working
+	// this way since the first prototype, it might not make
+	// sense anymore. This probably makes NS records and such
+	// work as expected.
+	if label.Weight[qtype] == 0 {
+		return servers
+	}
+
+	if qtype == dns.TypeCNAME || qtype == dns.TypeMF {
+		max = 1
+	}
 
-		rrCount := len(servers)
-		if max > rrCount {
-			max = rrCount
+	rrCount := len(servers)
+	if max > rrCount {
+		max = rrCount
+	}
+	result := make(Records, max)
+
+	// Find the distance to each server, and find the servers that are
+	// closer to the querier than the max'th furthest server, or within
+	// 5% thereof. What this means in practice is that if we have a nearby
+	// cluster of servers that are close, they all get included, so load
+	// balancing works
+	if qtype == dns.TypeA && location != nil && max < rrCount {
+		// First we record the distance to each server
+		distances := make([]float64, rrCount)
+		for i, s := range servers {
+			distance := location.Distance(s.Loc)
+			distances[i] = distance
 		}
-		result := make(Records, max)
-
-		// Find the distance to each server, and find the servers that are
-		// closer to the querier than the max'th furthest server, or within
-		// 5% thereof. What this means in practice is that if we have a nearby
-		// cluster of servers that are close, they all get included, so load
-		// balancing works
-		if qtype == dns.TypeA && location != nil && max < rrCount {
-			// First we record the distance to each server
-			distances := make([]float64, rrCount)
-			for i, s := range servers {
-				distance := location.Distance(s.Loc)
-				distances[i] = distance
-			}
 
-			// though this looks like O(n^2), typically max is small (e.g. 2)
-			// servers often have the same geographic location
-			// and rrCount is pretty small too, so the gain of an
-			// O(n log n) sort is small.
-			chosen := 0
-			choose := make([]bool, rrCount)
-
-			for chosen < max {
-				// Determine the minimum distance of servers not yet chosen
-				minDist := location.MaxDistance()
-				for i, _ := range servers {
-					if !choose[i] && distances[i] <= minDist {
-						minDist = distances[i]
-					}
+		// though this looks like O(n^2), typically max is small (e.g. 2)
+		// servers often have the same geographic location
+		// and rrCount is pretty small too, so the gain of an
+		// O(n log n) sort is small.
+		chosen := 0
+		choose := make([]bool, rrCount)
+
+		for chosen < max {
+			// Determine the minimum distance of servers not yet chosen
+			minDist := location.MaxDistance()
+			for i, _ := range servers {
+				if !choose[i] && distances[i] <= minDist {
+					minDist = distances[i]
 				}
-				// The threshold for inclusion on the this pass is 5% more
-				// than the minimum distance
-				minDist = minDist * 1.05
-				// Choose all the servers within the distance
-				for i := range servers {
-					if !choose[i] && distances[i] <= minDist {
-						choose[i] = true
-						chosen++
-					}
+			}
+			// The threshold for inclusion on the this pass is 5% more
+			// than the minimum distance
+			minDist = minDist * 1.05
+			// Choose all the servers within the distance
+			for i := range servers {
+				if !choose[i] && distances[i] <= minDist {
+					choose[i] = true
+					chosen++
 				}
 			}
+		}
 
-			// Now choose only the chosen servers, using filtering without allocation
-			// slice trick. Meanwhile recalculate the total weight
-			tmpServers := servers[:0]
-			sum = 0
-			for i, s := range servers {
-				if choose[i] {
-					tmpServers = append(tmpServers, s)
-					sum += s.Weight
-				}
+		// Now choose only the chosen servers, using filtering without allocation
+		// slice trick. Meanwhile recalculate the total weight
+		tmpServers := servers[:0]
+		sum = 0
+		for i, s := range servers {
+			if choose[i] {
+				tmpServers = append(tmpServers, s)
+				sum += s.Weight
 			}
-			servers = tmpServers
 		}
+		servers = tmpServers
+	}
 
-		for si := 0; si < max; si++ {
-			n := rand.Intn(sum + 1)
-			s := 0
+	for si := 0; si < max; si++ {
+		n := rand.Intn(sum + 1)
+		s := 0
 
-			for i := range servers {
-				s += int(servers[i].Weight)
-				if s >= n {
-					sum -= servers[i].Weight
-					result[si] = servers[i]
-
-					// remove the server from the list
-					servers = append(servers[:i], servers[i+1:]...)
-					break
-				}
+		for i := range servers {
+			s += int(servers[i].Weight)
+			if s >= n {
+				sum -= servers[i].Weight
+				result[si] = servers[i]
+
+				// remove the server from the list
+				servers = append(servers[:i], servers[i+1:]...)
+				break
 			}
 		}
-
-		return result
 	}
-	log.Printf("returning nil ...!")
-	return nil
+
+	return result
 }

zones/reader.go

@@ -170,7 +170,6 @@ func setupZoneData(data map[string]interface{}, zone *Zone) {
 				continue
 			case "health":
 				zone.addHealthReference(label, rdata)
-				log.Printf("health status: '%+v'", label.Test.String())
 				continue
 			}
 

zones/zone.go

@@ -323,17 +323,12 @@ func (z *Zone) addHealthReference(l *Label, data interface{}) {
 }
 
 func (z *Zone) setupHealthTests() {
-
-	log.Println("Setting up Health Tests on records")
-
 	for _, label := range z.Labels {
 		if label.Test == nil {
 			// log.Printf("label.Test for '%s' == nil", label.Label)
 			continue
 		}
 
-		log.Printf("====  setting up '%s'", label.Label)
-
 		// todo: document which record types are processed
 		// or process all ...
 		for _, rrs := range label.Records {
@@ -352,10 +347,8 @@ func (z *Zone) setupHealthTests() {
 				default:
 					continue
 				}
-				log.Printf("t='%s'", t)
 				rec.Test = t
 			}
-			log.Printf("rrs: %+v", rrs)
 		}
 	}
 }

zones/zone_health_test.go

@@ -1,6 +1,7 @@
 package zones
 
 import (
+	"math/rand"
 	"testing"
 
 	"github.com/abh/geodns/health"
@@ -8,21 +9,32 @@ import (
 )
 
 type HealthStatus struct {
-	t *testing.T
+	t      *testing.T
+	status health.StatusType
+	odds   float64
 }
 
 func (hs *HealthStatus) GetStatus(name string) health.StatusType {
 	hs.t.Logf("GetStatus(%s)", name)
-
 	// hs.t.Fatalf("in get status")
-	return health.StatusUnknown
+
+	if hs.odds >= 0 {
+		switch rand.Float64() < hs.odds {
+		case true:
+			return health.StatusHealthy
+		case false:
+			return health.StatusUnhealthy
+		}
+	}
+
+	return hs.status
 }
 
 func TestHealth(t *testing.T) {
 	muxm := loadZones(t)
 	t.Log("setting up health status")
 
-	hs := &HealthStatus{t: t}
+	hs := &HealthStatus{t: t, odds: -1, status: health.StatusUnhealthy}
 
 	tz := muxm.zonelist["hc.example.com"]
 	tz.HealthStatus = hs
@@ -32,11 +44,12 @@ func TestHealth(t *testing.T) {
 	matches := tz.FindLabels("tucs", []string{"@"}, []uint16{dns.TypeA})
 	// t.Logf("qt: %d, label: '%+v'", qt, label)
 	records := tz.Picker(matches[0].Label, matches[0].Type, 2, nil)
+	if len(records) > 0 {
+		t.Errorf("got %d records when expecting 0", len(records))
+	}
 
 	// t.Logf("label.Test: '%+v'", label.Test)
 
-	t.Logf("records: '%+v'", records)
-
 	if len(records) == 0 {
 		t.Log("didn't get any records")
 	}