Changed the router to default to roundrobin when weights are used

If the route has weights set for the services it is associated with,
then we will set the default load balance policy to RoundRobin if no
policy is set with an annotation or as a global default with an
environment variable.  Without this change the user would need to both
set the weights, and then set an annotation to change the default
balancing algorithm... which people almost always forgot to do.

For bug 1416869 (https://bugzilla.redhat.com/show_bug.cgi?id=1416869)

Author: knobunc

images/router/haproxy/conf/haproxy-config.template

@@ -255,7 +255,7 @@ backend be_edge_http_{{$cfgIdx}}
       {{ if (matchPattern "roundrobin|leastconn|source" (env "ROUTER_LOAD_BALANCE_ALGORITHM" "")) }}
   balance {{ env "ROUTER_LOAD_BALANCE_ALGORITHM" "leastconn"}}
       {{ else }}
-  balance leastconn
+  balance {{ if cfg.HasWeights }}roundrobin{{ else }}leastconn{{ end }}
       {{ end }}
     {{ end }}
     {{ with $value := index $cfg.Annotations "haproxy.router.openshift.io/timeout"}}
@@ -337,7 +337,11 @@ backend be_tcp_{{$cfgIdx}}
   balance {{ $balanceAlgo }}
       {{ end }}
     {{ else }}
-  balance {{ env "ROUTER_TCP_BALANCE_SCHEME" "source" }}
+      {{ if (matchPattern "roundrobin|leastconn|source" (env "ROUTER_TCP_BALANCE_SCHEME" "")) }}
+  balance {{ env "ROUTER_TCP_BALANCE_SCHEME" "source"}}
+      {{ else }}
+  balance {{ if cfg.HasWeights }}roundrobin{{ else }}source{{ end }}
+      {{ end }}
     {{ end }}
     {{ with $value := index $cfg.Annotations "haproxy.router.openshift.io/timeout"}}
       {{if (matchPattern "[1-9][0-9]*(us|ms|s|m|h|d)?" $value) }}
@@ -403,7 +407,7 @@ backend be_secure_{{$cfgIdx}}
       {{ if (matchPattern "roundrobin|leastconn|source" (env "ROUTER_LOAD_BALANCE_ALGORITHM" "")) }}
   balance {{ env "ROUTER_LOAD_BALANCE_ALGORITHM" "leastconn"}}
       {{ else }}
-  balance leastconn
+  balance {{ if cfg.HasWeights }}roundrobin{{ else }}leastconn{{ end }}
       {{ end }}
     {{ end }}
     {{ with $value := index $cfg.Annotations "haproxy.router.openshift.io/timeout"}}

pkg/router/template/router.go

@@ -598,14 +598,17 @@ func (r *templateRouter) createServiceAliasConfig(route *routeapi.Route, routeKe
 	// The router config trumps what the route asks for/wants.
 	wildcard := r.allowWildcardRoutes && wantsWildcardSupport
 
+	serviceUnits, hasWeights := getServiceUnits(route)
+
 	config := ServiceAliasConfig{
 		Name:             route.Name,
 		Namespace:        route.Namespace,
 		Host:             route.Spec.Host,
 		Path:             route.Spec.Path,
 		IsWildcard:       wildcard,
 		Annotations:      route.Annotations,
-		ServiceUnitNames: getServiceUnits(route),
+		ServiceUnitNames: serviceUnits,
+		HasWeights:       hasWeights,
 	}
 
 	if route.Spec.Port != nil {
@@ -841,23 +844,26 @@ func generateDestCertKey(config *ServiceAliasConfig) string {
 // getServiceUnits returns a map of service keys to their weights.
 // Weight suggests the % of traffic that a given service will receive
 // compared to other services pointed to by the route.
-func getServiceUnits(route *routeapi.Route) map[string]int32 {
+func getServiceUnits(route *routeapi.Route) (map[string]int32, bool) {
 	serviceUnits := make(map[string]int32)
+	hasWeights := false
 	key := fmt.Sprintf("%s/%s", route.Namespace, route.Spec.To.Name)
 	if route.Spec.To.Weight == nil {
 		serviceUnits[key] = 0
 	} else {
 		serviceUnits[key] = *route.Spec.To.Weight
+		hasWeights = true
 	}
 	for _, svc := range route.Spec.AlternateBackends {
 		key = fmt.Sprintf("%s/%s", route.Namespace, svc.Name)
 		if svc.Weight == nil {
 			serviceUnits[key] = 0
 		} else {
 			serviceUnits[key] = *svc.Weight
+			hasWeights = true
 		}
 	}
-	return serviceUnits
+	return serviceUnits, hasWeights
 }
 
 // configsAreEqual determines whether the given service alias configs can be considered equal.

pkg/router/template/router_test.go

@@ -326,6 +326,11 @@ func TestCreateServiceAliasConfig(t *testing.T) {
 		config.PreferPort != route.Spec.Port.TargetPort.String() || !reflect.DeepEqual(expectedSUs, config.ServiceUnitNames) {
 		t.Errorf("Route %v did not match service alias config %v", route, config)
 	}
+
+	// Check that we are detecting weights are set
+	if !config.HasWeights {
+		t.Errorf("Route %v did not generate a service alias config with HasWeights set %v", route, config)
+	}
 }
 
 // TestAddRoute validates that adding a route creates a service alias config and associated service units
@@ -373,6 +378,9 @@ func TestAddRoute(t *testing.T) {
 	} else if config.Host != route.Spec.Host {
 		// This test is not validating createServiceAliasConfig, so superficial validation should be good enough.
 		t.Errorf("Route %v did not match service alias config %v", route, config)
+	} else if config.HasWeights {
+		// Check that we don't set HasWeights when there are no weights
+		t.Errorf("Route %v set HasWeights even though there are no weights defined, config %v", route, config)
 	}
 }
 

pkg/router/template/types.go

@@ -52,6 +52,10 @@ type ServiceAliasConfig struct {
 	// ServiceUnitNames is a collection of services that support this route, keyed by service name
 	// and valued on the weight attached to it with respect to other entries in the map
 	ServiceUnitNames map[string]int32
+
+	// HasWeights is a boolean that signals whether or not there are any weights set in the services
+	// that support this route.
+	HasWeights bool
 }
 
 type ServiceAliasConfigStatus string