Skip to content

Commit

Permalink
Don't enqueue ingress for some service changes (#365)
Browse files Browse the repository at this point in the history
When a service changes we get a service change event and an endpoint change event. Previously we'd enqueue the associated ingresses for all service changes. Now we only enqueue it when the service changes in a way that won't be covered by the endpoint changes.

Specifically for Service.spec.selector changes, which users sometimes use to do blue/green deployments, we will no longer enqueue an ingress - instead the changes will be handled (correctly) by the endpoint handler.
  • Loading branch information
isaachawley authored Sep 10, 2018
1 parent 7e54578 commit d100319
Show file tree
Hide file tree
Showing 2 changed files with 200 additions and 5 deletions.
49 changes: 44 additions & 5 deletions internal/handlers/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package handlers

import (
"reflect"
"sort"

"github.com/golang/glog"
"github.com/nginxinc/kubernetes-ingress/internal/controller"
Expand Down Expand Up @@ -47,14 +48,52 @@ func CreateServiceHandlers(lbc *controller.LoadBalancerController) cache.Resourc
},
UpdateFunc: func(old, cur interface{}) {
if !reflect.DeepEqual(old, cur) {
svc := cur.(*api_v1.Service)
if lbc.IsExternalServiceForStatus(svc) {
lbc.AddSyncQueue(svc)
curSvc := cur.(*api_v1.Service)
if lbc.IsExternalServiceForStatus(curSvc) {
lbc.AddSyncQueue(curSvc)
return
}
glog.V(3).Infof("Service %v changed, syncing", svc.Name)
lbc.EnqueueIngressForService(svc)
oldSvc := old.(*api_v1.Service)
if hasServicePortChanges(oldSvc.Spec.Ports, curSvc.Spec.Ports) {
glog.V(3).Infof("Service %v changed, syncing", curSvc.Name)
lbc.EnqueueIngressForService(curSvc)
}
}
},
}
}

type portSort []api_v1.ServicePort

func (a portSort) Len() int {
return len(a)
}

func (a portSort) Swap(i, j int) {
a[i], a[j] = a[j], a[i]
}

func (a portSort) Less(i, j int) bool {
if a[i].Name == a[j].Name {
return a[i].Port < a[j].Port
}
return a[i].Name < a[j].Name
}

// hasServicePortChanges only compares ServicePort.Name and .Port.
func hasServicePortChanges(oldServicePorts []api_v1.ServicePort, curServicePorts []api_v1.ServicePort) bool {
if len(oldServicePorts) != len(curServicePorts) {
return true
}

sort.Sort(portSort(oldServicePorts))
sort.Sort(portSort(curServicePorts))

for i := range oldServicePorts {
if oldServicePorts[i].Port != curServicePorts[i].Port ||
oldServicePorts[i].Name != curServicePorts[i].Name {
return true
}
}
return false
}
156 changes: 156 additions & 0 deletions internal/handlers/service_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
package handlers

import (
"testing"

"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

func TestHasServicePortChanges(t *testing.T) {
cases := []struct {
a []v1.ServicePort
b []v1.ServicePort
result bool
reason string
}{
{
[]v1.ServicePort{},
[]v1.ServicePort{},
false,
"Empty should report no changes",
},
{
[]v1.ServicePort{{
Port: 80,
}},
[]v1.ServicePort{{
Port: 8080,
}},
true,
"Different Ports",
},
{
[]v1.ServicePort{{
Port: 80,
}},
[]v1.ServicePort{{
Port: 80,
}},
false,
"Same Ports",
},
{
[]v1.ServicePort{{
Name: "asdf",
Port: 80,
}},
[]v1.ServicePort{{
Name: "asdf",
Port: 80,
}},
false,
"Same Port and Name",
},
{
[]v1.ServicePort{{
Name: "foo",
Port: 80,
}},
[]v1.ServicePort{{
Name: "bar",
Port: 80,
}},
true,
"Different Name same Port",
},
{
[]v1.ServicePort{{
Name: "foo",
Port: 8080,
}},
[]v1.ServicePort{{
Name: "bar",
Port: 80,
}},
true,
"Different Name different Port",
},
{
[]v1.ServicePort{{
Name: "foo",
}},
[]v1.ServicePort{{
Name: "fooo",
}},
true,
"Very similar Name",
},
{
[]v1.ServicePort{{
Name: "asdf",
Port: 80,
TargetPort: intstr.IntOrString{
IntVal: 80,
},
}},
[]v1.ServicePort{{
Name: "asdf",
Port: 80,
TargetPort: intstr.IntOrString{
IntVal: 8080,
},
}},
false,
"TargetPort should be ignored",
},
{
[]v1.ServicePort{{
Name: "foo",
}, {
Name: "bar",
}},
[]v1.ServicePort{{
Name: "foo",
}, {
Name: "bar",
}},
false,
"Multiple same names",
},
{
[]v1.ServicePort{{
Name: "foo",
}, {
Name: "bar",
}},
[]v1.ServicePort{{
Name: "foo",
}, {
Name: "bars",
}},
true,
"Multiple different names",
},
{
[]v1.ServicePort{{
Name: "foo",
}, {
Port: 80,
}},
[]v1.ServicePort{{
Port: 80,
}, {
Name: "foo",
}},
false,
"Some names some ports",
},
}

for _, c := range cases {
if c.result != hasServicePortChanges(c.a, c.b) {
t.Errorf("hasServicePortChanges returned %v, but expected %v for %q case", c.result, !c.result, c.reason)
}
}
}

0 comments on commit d100319

Please sign in to comment.