Skip to content

Commit fc8725b

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #17155 from knobunc/bug/bz1507664-suppress-health-checks
Automatic merge from submit-queue. Bug/bz1507664 suppress health checks Fix the "supress health checks when only one backing service" logic This reverts commit 9835dca because it is wrong, then the second commit implements the correct change. Previously we had attempted to suppress the health checks, but it was looking at the endpoints of the service, not the servers that back the route. The problem is that if we just look at endpoints, then if a route has two backing services, each with one endpoint, we would incorrectly suppress the health checks. This is the third attempt. I had missed a case in the last one, so now the logic is moved to be computed and recorded by the router controller, but used in the template. Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)
2 parents b65ef46 + 8cb7b95 commit fc8725b

File tree

5 files changed

+47
-200
lines changed

5 files changed

+47
-200
lines changed

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -410,10 +410,8 @@ backend be_secure:{{$cfgIdx}}
410410
{{- else if or (eq $cfg.TLSTermination "") (eq $cfg.TLSTermination "edge") }}
411411
{{- end }}{{/* end type specific options*/}}
412412

413-
{{- if not $endpoint.NoHealthCheck }} check inter {{firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms"}}
413+
{{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms"}}
414414
{{- end }}{{/* end else no health check */}}
415-
416-
417415
{{- end }}{{/* end if cg.TLSTermination */}}
418416
{{- end }}{{/* end range processEndpointsForAlias */}}
419417
{{- end }}{{/* end get serviceUnit from its name */}}
@@ -464,7 +462,7 @@ backend be_tcp:{{$cfgIdx}}
464462
{{- with $serviceUnit := index $.ServiceUnits $serviceUnitName }}
465463
{{- range $idx, $endpoint := processEndpointsForAlias $cfg $serviceUnit (env "ROUTER_BACKEND_PROCESS_ENDPOINTS" "") }}
466464
server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} weight {{$weight}}
467-
{{- if not $endpoint.NoHealthCheck }} check inter {{firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms"}}
465+
{{- if and (not $endpoint.NoHealthCheck) (gt $cfg.ActiveEndpoints 1) }} check inter {{firstMatch $timeSpecPattern (index $cfg.Annotations "router.openshift.io/haproxy.health.check.interval") (env "ROUTER_BACKEND_CHECK_INTERVAL") "5000ms"}}
468466
{{- end }}{{/* end else no health check */}}
469467
{{- end }}{{/* end range processEndpointsForAlias */}}
470468
{{- end }}{{/* end get ServiceUnit from serviceUnitName */}}

‎pkg/router/template/router.go‎

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,9 @@ func (r *templateRouter) writeConfig() error {
387387
// called here to make sure we have the actual number of endpoints.
388388
cfg.ServiceUnitNames = r.calculateServiceWeights(cfg.ServiceUnits)
389389

390+
// Calculate the number of active endpoints for the route.
391+
cfg.ActiveEndpoints = r.getActiveEndpoints(cfg.ServiceUnits)
392+
390393
cfg.Status = ServiceAliasConfigStatusSaved
391394
r.state[k] = cfg
392395
}
@@ -877,28 +880,49 @@ func getServiceUnits(route *routeapi.Route) map[string]int32 {
877880

878881
// get the weight and number of endpoints for each service
879882
key := endpointsKeyFromParts(route.Namespace, route.Spec.To.Name)
880-
serviceUnits[key] = 0
881-
if route.Spec.To.Weight != nil {
882-
serviceUnits[key] = int32(*route.Spec.To.Weight)
883-
}
884-
if serviceUnits[key] < 0 || serviceUnits[key] > 256 {
885-
serviceUnits[key] = 0
886-
}
883+
serviceUnits[key] = getServiceUnitWeight(route.Spec.To.Weight)
887884

888885
for _, svc := range route.Spec.AlternateBackends {
889886
key = endpointsKeyFromParts(route.Namespace, svc.Name)
890-
serviceUnits[key] = 0
891-
if svc.Weight != nil {
892-
serviceUnits[key] = int32(*svc.Weight)
893-
}
894-
if serviceUnits[key] < 0 || serviceUnits[key] > 256 {
895-
serviceUnits[key] = 0
896-
}
887+
serviceUnits[key] = getServiceUnitWeight(svc.Weight)
897888
}
898889

899890
return serviceUnits
900891
}
901892

893+
// getServiceUnitWeight takes a reference to a weight and returns its value or the default.
894+
// It also checks that it is in the correct range.
895+
func getServiceUnitWeight(weightRef *int32) int32 {
896+
// Default to 1 if there is no weight
897+
var weight int32 = 1
898+
if weightRef != nil {
899+
weight = *weightRef
900+
}
901+
902+
// Do a bounds check
903+
if weight < 0 {
904+
weight = 0
905+
} else if weight > 256 {
906+
weight = 256
907+
}
908+
909+
return weight
910+
}
911+
912+
// getActiveEndpoints calculates the number of endpoints that are not associated
913+
// with service units with a zero weight and returns the count.
914+
func (r *templateRouter) getActiveEndpoints(serviceUnits map[string]int32) int {
915+
var activeEndpoints int32 = 0
916+
917+
for key, weight := range serviceUnits {
918+
if weight > 0 {
919+
activeEndpoints += r.numberOfEndpoints(key)
920+
}
921+
}
922+
923+
return int(activeEndpoints)
924+
}
925+
902926
// calculateServiceWeights returns a map of service keys to their weights.
903927
// Each service gets (weight/sum_of_weights) fraction of the requests.
904928
// For each service, the requests are distributed among the endpoints.

‎pkg/router/template/template_helper.go‎

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -157,28 +157,16 @@ func processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action
157157
}
158158

159159
func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
160-
if len(alias.PreferPort) == 0 && len(svc.EndpointTable) != 1 {
161-
// We can skip the work. They are selecting everything, and since they don't have one endpoint,
162-
// we can't disable the health checks. So we can just use their list.
160+
if len(alias.PreferPort) == 0 {
163161
return svc.EndpointTable
164162
}
165-
166163
endpoints := make([]Endpoint, 0, len(svc.EndpointTable))
167-
for _, endpoint := range svc.EndpointTable {
168-
// Filter the list:
169-
// - If PreferPort length is 0, match everything
170-
// - Otherwise, if the PortName or Port matches PreferPort, then we have a match
171-
if len(alias.PreferPort) == 0 || endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
164+
for i := range svc.EndpointTable {
165+
endpoint := svc.EndpointTable[i]
166+
if endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
172167
endpoints = append(endpoints, endpoint)
173168
}
174169
}
175-
176-
// We want to disable endpoint checks if there is only one endpoint since there's no point in
177-
// testing and removing it from the set ahead of time since there are no other eps to replace it.
178-
if len(endpoints) == 1 {
179-
endpoints[0].NoHealthCheck = true
180-
}
181-
182170
return endpoints
183171
}
184172

‎pkg/router/template/template_helper_test.go‎

Lines changed: 0 additions & 166 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package templaterouter
22

33
import (
4-
"reflect"
54
"regexp"
65
"testing"
76
)
@@ -298,168 +297,3 @@ func TestMatchPattern(t *testing.T) {
298297
}
299298
}
300299
}
301-
302-
func TestEndpointsForAlias(t *testing.T) {
303-
testEndpoints := []struct {
304-
name string
305-
preferPort string
306-
eps []Endpoint
307-
expected []Endpoint
308-
}{
309-
{
310-
name: "Test no preference with a single endpoint",
311-
preferPort: "",
312-
eps: []Endpoint{
313-
{
314-
ID: "ep1",
315-
Port: "80",
316-
PortName: "http",
317-
},
318-
},
319-
expected: []Endpoint{
320-
{
321-
ID: "ep1",
322-
Port: "80",
323-
PortName: "http",
324-
NoHealthCheck: true,
325-
},
326-
},
327-
},
328-
{
329-
name: "Test no preference with multiple",
330-
preferPort: "",
331-
eps: []Endpoint{
332-
{
333-
ID: "ep1",
334-
Port: "80",
335-
PortName: "http",
336-
},
337-
{
338-
ID: "ep1",
339-
Port: "443",
340-
PortName: "https",
341-
},
342-
{
343-
ID: "ep2",
344-
Port: "80",
345-
PortName: "http",
346-
},
347-
{
348-
ID: "ep3",
349-
Port: "80",
350-
PortName: "http",
351-
},
352-
},
353-
expected: []Endpoint{
354-
{
355-
ID: "ep1",
356-
Port: "80",
357-
PortName: "http",
358-
},
359-
{
360-
ID: "ep1",
361-
Port: "443",
362-
PortName: "https",
363-
},
364-
{
365-
ID: "ep2",
366-
Port: "80",
367-
PortName: "http",
368-
},
369-
{
370-
ID: "ep3",
371-
Port: "80",
372-
PortName: "http",
373-
},
374-
},
375-
},
376-
{
377-
name: "Test a preference resulting in multiple",
378-
preferPort: "http",
379-
eps: []Endpoint{
380-
{
381-
ID: "ep1",
382-
Port: "80",
383-
PortName: "http",
384-
},
385-
{
386-
ID: "ep1",
387-
Port: "443",
388-
PortName: "https",
389-
},
390-
{
391-
ID: "ep2",
392-
Port: "80",
393-
PortName: "http",
394-
},
395-
{
396-
ID: "ep3",
397-
Port: "80",
398-
PortName: "http",
399-
},
400-
},
401-
expected: []Endpoint{
402-
{
403-
ID: "ep1",
404-
Port: "80",
405-
PortName: "http",
406-
},
407-
{
408-
ID: "ep2",
409-
Port: "80",
410-
PortName: "http",
411-
},
412-
{
413-
ID: "ep3",
414-
Port: "80",
415-
PortName: "http",
416-
},
417-
},
418-
},
419-
{
420-
name: "Test a preference resulting in one",
421-
preferPort: "https",
422-
eps: []Endpoint{
423-
{
424-
ID: "ep1",
425-
Port: "80",
426-
PortName: "http",
427-
},
428-
{
429-
ID: "ep1",
430-
Port: "443",
431-
PortName: "https",
432-
},
433-
{
434-
ID: "ep2",
435-
Port: "80",
436-
PortName: "http",
437-
},
438-
{
439-
ID: "ep3",
440-
Port: "80",
441-
PortName: "http",
442-
},
443-
},
444-
expected: []Endpoint{
445-
{
446-
ID: "ep1",
447-
Port: "443",
448-
PortName: "https",
449-
NoHealthCheck: true,
450-
},
451-
},
452-
},
453-
}
454-
455-
for _, te := range testEndpoints {
456-
eps := endpointsForAlias(
457-
ServiceAliasConfig{PreferPort: te.preferPort},
458-
ServiceUnit{EndpointTable: te.eps},
459-
)
460-
461-
if !reflect.DeepEqual(te.expected, eps) {
462-
t.Errorf("%s: expected result: %#v, got: %#v", te.name, te.expected, eps)
463-
}
464-
}
465-
}

‎pkg/router/template/types.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ type ServiceAliasConfig struct {
6464

6565
// ActiveServiceUnits is a count of the service units with a non-zero weight
6666
ActiveServiceUnits int
67+
68+
// ActiveEndpoints is a count of the route endpoints that are part of a service unit with a non-zero weight
69+
ActiveEndpoints int
6770
}
6871

6972
type ServiceAliasConfigStatus string

0 commit comments

Comments
 (0)