Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not create instance group with NEG backends #440

Merged
merged 1 commit into from
Aug 21, 2018
Merged

do not create instance group with NEG backends #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions pkg/controller/controller.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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,
}
}