Skip to content

Commit

Permalink
Merge pull request #17077 from knobunc/fix/health-supression
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue.

Fix the "supress health checks when only one backing service" logic

This reverts commit 4833eb5 and then fixes the implementation.

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.

Fixes bug 1507664 (https://bugzilla.redhat.com/show_bug.cgi?id=1507664)
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Oct 31, 2017
2 parents 394c793 + 9835dca commit 1bae363
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 155 deletions.
7 changes: 0 additions & 7 deletions pkg/router/template/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,6 @@ func createRouterEndpoints(endpoints *kapi.Endpoints, excludeUDP bool, lookupSvc
wasIdled = true
}

// Take a reasonable guess at the number of endpoints to avoid reallocating the out array when we append
out := make([]Endpoint, 0, len(endpoints.Subsets)*4)

// Now build the actual endpoints we pass to the template
Expand Down Expand Up @@ -307,11 +306,5 @@ func createRouterEndpoints(endpoints *kapi.Endpoints, excludeUDP bool, lookupSvc
}
}

// We want to disable endpoint checks if there is only one endpoint
// We skip the case where it is idled, since we set NoHealthCheck above
if wasIdled == false && len(out) == 1 {
out[0].NoHealthCheck = true
}

return out
}
153 changes: 9 additions & 144 deletions pkg/router/template/plugin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,16 +275,15 @@ func TestHandleEndpoints(t *testing.T) {
Name: "foo/test", //service name from kapi.endpoints object
EndpointTable: []Endpoint{
{
ID: "ept:test:1.1.1.1:345",
IP: "1.1.1.1",
Port: "345",
NoHealthCheck: true,
ID: "ept:test:1.1.1.1:345",
IP: "1.1.1.1",
Port: "345",
},
},
},
},
{
name: "Endpoint mod (one ep, one address)",
name: "Endpoint mod",
eventType: watch.Modified,
endpoints: &kapi.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -300,143 +299,9 @@ func TestHandleEndpoints(t *testing.T) {
Name: "foo/test",
EndpointTable: []Endpoint{
{
ID: "pod:pod-1:test:2.2.2.2:8080",
IP: "2.2.2.2",
Port: "8080",
NoHealthCheck: true,
},
},
},
},
{
name: "Endpoint mod (second ep, one address each)",
eventType: watch.Modified,
endpoints: &kapi.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "test",
},
Subsets: []kapi.EndpointSubset{
{
Addresses: []kapi.EndpointAddress{{IP: "2.2.2.2", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
Ports: []kapi.EndpointPort{{Port: 8080}},
},
{
Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}}},
Ports: []kapi.EndpointPort{{Port: 8081}},
},
},
},
expectedServiceUnit: &ServiceUnit{
Name: "foo/test",
EndpointTable: []Endpoint{
{
ID: "pod:pod-1:test:2.2.2.2:8080",
IP: "2.2.2.2",
Port: "8080",
NoHealthCheck: false,
},
{
ID: "pod:pod-2:test:3.3.3.3:8081",
IP: "3.3.3.3",
Port: "8081",
NoHealthCheck: false,
},
},
},
},
{
name: "Endpoint mod (one ep, two addresses)",
eventType: watch.Modified,
endpoints: &kapi.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "test",
},
Subsets: []kapi.EndpointSubset{
{
Addresses: []kapi.EndpointAddress{
{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}},
{IP: "4.4.4.4", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-3"}},
},
Ports: []kapi.EndpointPort{{Port: 8080}},
},
},
},
expectedServiceUnit: &ServiceUnit{
Name: "foo/test",
EndpointTable: []Endpoint{
{
ID: "pod:pod-2:test:3.3.3.3:8080",
IP: "3.3.3.3",
Port: "8080",
NoHealthCheck: false,
},
{
ID: "pod:pod-3:test:4.4.4.4:8080",
IP: "4.4.4.4",
Port: "8080",
NoHealthCheck: false,
},
},
},
},
{
name: "Endpoint mod (one ep, two ports)",
eventType: watch.Modified,
endpoints: &kapi.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "test",
},
Subsets: []kapi.EndpointSubset{
{
Addresses: []kapi.EndpointAddress{
{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-2"}},
},
Ports: []kapi.EndpointPort{{Port: 8080}, {Port: 8081}},
},
},
},
expectedServiceUnit: &ServiceUnit{
Name: "foo/test",
EndpointTable: []Endpoint{
{
ID: "pod:pod-2:test:3.3.3.3:8080",
IP: "3.3.3.3",
Port: "8080",
NoHealthCheck: false,
},
{
ID: "pod:pod-2:test:3.3.3.3:8081",
IP: "3.3.3.3",
Port: "8081",
NoHealthCheck: false,
},
},
},
},
{
name: "Endpoint mod (back to one ep)",
eventType: watch.Modified,
endpoints: &kapi.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: "foo",
Name: "test",
},
Subsets: []kapi.EndpointSubset{{
Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
Ports: []kapi.EndpointPort{{Port: 8080}},
}},
},
expectedServiceUnit: &ServiceUnit{
Name: "foo/test",
EndpointTable: []Endpoint{
{
ID: "pod:pod-1:test:3.3.3.3:8080",
IP: "3.3.3.3",
Port: "8080",
NoHealthCheck: true,
ID: "pod:pod-1:test:2.2.2.2:8080",
IP: "2.2.2.2",
Port: "8080",
},
},
},
Expand All @@ -450,8 +315,8 @@ func TestHandleEndpoints(t *testing.T) {
Name: "test",
},
Subsets: []kapi.EndpointSubset{{
Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3", TargetRef: &kapi.ObjectReference{Kind: "Pod", Name: "pod-1"}}},
Ports: []kapi.EndpointPort{{Port: 8080}},
Addresses: []kapi.EndpointAddress{{IP: "3.3.3.3"}},
Ports: []kapi.EndpointPort{{Port: 0}},
}},
},
expectedServiceUnit: &ServiceUnit{
Expand Down
20 changes: 16 additions & 4 deletions pkg/router/template/template_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,16 +157,28 @@ func processEndpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit, action
}

func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
if len(alias.PreferPort) == 0 {
if len(alias.PreferPort) == 0 && len(svc.EndpointTable) != 1 {
// We can skip the work. They are selecting everything, and since they don't have one endpoint,
// we can't disable the health checks. So we can just use their list.
return svc.EndpointTable
}

endpoints := make([]Endpoint, 0, len(svc.EndpointTable))
for i := range svc.EndpointTable {
endpoint := svc.EndpointTable[i]
if endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
for _, endpoint := range svc.EndpointTable {
// Filter the list:
// - If PreferPort length is 0, match everything
// - Otherwise, if the PortName or Port matches PreferPort, then we have a match
if len(alias.PreferPort) == 0 || endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
endpoints = append(endpoints, endpoint)
}
}

// We want to disable endpoint checks if there is only one endpoint since there's no point in
// testing and removing it from the set ahead of time since there are no other eps to replace it.
if len(endpoints) == 1 {
endpoints[0].NoHealthCheck = true
}

return endpoints
}

Expand Down
Loading

0 comments on commit 1bae363

Please sign in to comment.