Skip to content

Commit

Permalink
do not create instance group with NEG backends
Browse files Browse the repository at this point in the history
  • Loading branch information
@freehan
freehan committed Aug 17, 2018
1 parent 4137d51 commit dbfd50e
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 37 deletions.
19 changes: 11 additions & 8 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,11 +316,13 @@ func (lbc *LoadBalancerController) ensureIngress(ing *extensions.Ingress, nodeNa
}

ingSvcPorts := urlMap.AllServicePorts()

igs, err := lbc.ensureInstanceGroupsAndPorts(ingSvcPorts, nodeNames)
if err != nil {
return err
}

// TODO: Remove this after deprecation
if utils.IsGCEMultiClusterIngress(ing) {
// Add instance group names as annotation on the ingress and return.
if ing.Annotations == nil {
Expand Down Expand Up @@ -480,12 +482,13 @@ func (lbc *LoadBalancerController) ToSvcPorts(ings *extensions.IngressList) []ut
return knownPorts
}

// ensureInstanceGroupsAndPorts creates instance group if necessary
// if all service ports have NEG enabled, then instance group creation will be skipped and return nil.
func (lbc *LoadBalancerController) ensureInstanceGroupsAndPorts(svcPorts []utils.ServicePort, nodeNames []string) ([]*compute.InstanceGroup, error) {
ports := []int64{}
for _, p := range uniq(svcPorts) {
if !p.NEGEnabled {
ports = append(ports, p.NodePort)
}
ports := nodePorts(svcPorts)
if len(ports) == 0 {
glog.V(2).Infof("Skip ensuring instance groups as all backend(s) have NEG enabled.")
return nil, nil
}

// Create instance groups and set named ports.
Expand All @@ -507,7 +510,7 @@ func (lbc *LoadBalancerController) ensureInstanceGroupsAndPorts(svcPorts []utils
// - nodePorts are the ports for which we want BackendServies. BackendServices
// for ports not in this list are deleted.
// This method ignores googleapi 404 errors (StatusNotFound).
func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.ServicePort) error {
func (lbc *LoadBalancerController) gc(lbNames []string, svcPorts []utils.ServicePort) error {
// On GC:
// * Loadbalancers need to get deleted before backends.
// * Backends are refcounted in a shared pool.
Expand All @@ -519,7 +522,7 @@ func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.Servic
// happen when an Ingress is updated, if we don't GC after the update
// we'll leak the backend.
lbErr := lbc.l7Pool.GC(lbNames)
beErr := lbc.backendSyncer.GC(nodePorts)
beErr := lbc.backendSyncer.GC(svcPorts)
if lbErr != nil {
return lbErr
}
Expand All @@ -528,7 +531,7 @@ func (lbc *LoadBalancerController) gc(lbNames []string, nodePorts []utils.Servic
}

// TODO(ingress#120): Move this to the backend pool so it mirrors creation
if len(lbNames) == 0 {
if len(lbNames) == 0 || len(nodePorts(svcPorts)) == 0 {
igName := lbc.ctx.ClusterNamer.InstanceGroup()
glog.Infof("Deleting instance group %v", igName)
if err := lbc.instancePool.DeleteInstanceGroup(igName); err != err {
Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,3 +86,15 @@ func convert(ings []*extensions.Ingress) (retVal []interface{}) {
}
return
}

// nodePorts returns the list of uniq NodePort from the input ServicePorts.
// Only NonNEG service backend need NodePort.
func nodePorts(svcPorts []utils.ServicePort) []int64 {
ports := []int64{}
for _, p := range uniq(svcPorts) {
if !p.NEGEnabled {
ports = append(ports, p.NodePort)
}
}
return ports
}
115 changes: 86 additions & 29 deletions pkg/controller/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
"testing"
"time"

compute "google.golang.org/api/compute/v1"
"google.golang.org/api/compute/v1"

api_v1 "k8s.io/api/core/v1"
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -30,6 +30,7 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/ingress-gce/pkg/annotations"
"k8s.io/ingress-gce/pkg/utils"
"reflect"
)

// Pods created in loops start from this time, for routines that
Expand Down Expand Up @@ -216,68 +217,68 @@ func TestUniq(t *testing.T) {
{
"Two service ports",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080),
testServicePort("ns", "name", "443", 443, 30443),
testServicePort("ns", "name", "80", 80, 30080, true),
testServicePort("ns", "name", "443", 443, 30443, true),
},
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080),
testServicePort("ns", "name", "443", 443, 30443),
testServicePort("ns", "name", "80", 80, 30080, true),
testServicePort("ns", "name", "443", 443, 30443, true),
},
},
{
"Two service ports with different names",
[]utils.ServicePort{
testServicePort("ns", "name1", "80", 80, 30080),
testServicePort("ns", "name2", "80", 80, 30880),
testServicePort("ns", "name1", "80", 80, 30080, true),
testServicePort("ns", "name2", "80", 80, 30880, true),
},
[]utils.ServicePort{
testServicePort("ns", "name1", "80", 80, 30080),
testServicePort("ns", "name2", "80", 80, 30880),
testServicePort("ns", "name1", "80", 80, 30080, true),
testServicePort("ns", "name2", "80", 80, 30880, true),
},
},
{
"Two duplicate service ports",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080),
testServicePort("ns", "name", "80", 80, 30080),
testServicePort("ns", "name", "80", 80, 30080, true),
testServicePort("ns", "name", "80", 80, 30080, true),
},
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080),
testServicePort("ns", "name", "80", 80, 30080, true),
},
},
{
"Two services without nodeports",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "80", 80, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "80", 80, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
},
{
"2 out of 3 are duplicates",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "80", 80, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "80", 80, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
},
{
"mix of named port and port number references",
[]utils.ServicePort{
testServicePort("ns", "name", "http", 80, 0),
testServicePort("ns", "name", "https", 443, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "http", 80, 0, true),
testServicePort("ns", "name", "https", 443, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
[]utils.ServicePort{
testServicePort("ns", "name", "http", 80, 0),
testServicePort("ns", "name", "443", 443, 0),
testServicePort("ns", "name", "http", 80, 0, true),
testServicePort("ns", "name", "443", 443, 0, true),
},
},
}
Expand All @@ -302,6 +303,61 @@ func TestUniq(t *testing.T) {

}

func TestGetNodePortsUsedByIngress(t *testing.T) {
testCases := []struct {
desc string
svcPorts []utils.ServicePort
expectPorts []int64
}{
{
"empty input",
[]utils.ServicePort{},
[]int64{},
},
{
" all NEG enabled",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080, true),
testServicePort("ns", "name", "443", 443, 30443, true),
},
[]int64{},
},
{
" all nonNEG enabled",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080, false),
testServicePort("ns", "name", "443", 443, 30443, false),
},
[]int64{30080, 30443},
},
{
" mixed SvcPorts",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080, false),
testServicePort("ns", "name", "443", 443, 30443, true),
},
[]int64{30080},
},
{
" mixed SvcPorts with duplicates",
[]utils.ServicePort{
testServicePort("ns", "name", "80", 80, 30080, false),
testServicePort("ns", "name", "80", 80, 30080, false),
testServicePort("ns", "name", "443", 443, 30443, false),
},
[]int64{30080, 30443},
},
}

for _, tc := range testCases {
res := nodePorts(tc.svcPorts)
if !reflect.DeepEqual(res, tc.expectPorts) {
t.Errorf("For case %q, expect %v, but got %v", tc.desc, tc.expectPorts, res)
}
}

}

func testNode() *api_v1.Node {
return &api_v1.Node{
ObjectMeta: meta_v1.ObjectMeta{
Expand All @@ -327,7 +383,7 @@ func testNode() *api_v1.Node {
}
}

func testServicePort(namespace, name, port string, servicePort, nodePort int) utils.ServicePort {
func testServicePort(namespace, name, port string, servicePort, nodePort int, enableNEG bool) utils.ServicePort {
return utils.ServicePort{
ID: utils.ServicePortID{
Service: types.NamespacedName{
Expand All @@ -336,7 +392,8 @@ func testServicePort(namespace, name, port string, servicePort, nodePort int) ut
},
Port: intstr.FromString(port),
},
Port: int32(servicePort),
NodePort: int64(nodePort),
Port: int32(servicePort),
NodePort: int64(nodePort),
NEGEnabled: enableNEG,
}
}

0 comments on commit dbfd50e

Please sign in to comment.