From aea013fe855361e427ad663315a8cd1066286a98 Mon Sep 17 00:00:00 2001 From: Slavik Panasovets Date: Fri, 20 Jan 2023 16:09:54 +0000 Subject: [PATCH] Remove using node ports for L4 NetLB RBS Services NodePorts are not needed for NetLB RBS and were copied from legacy implementation. They were also slowing down our controller, because we were updating same instance group with node port for every service --- pkg/instances/instances.go | 2 +- pkg/l4lb/l4netlbcontroller.go | 33 ++++++++++++++++++++++++++---- pkg/l4lb/l4netlbcontroller_test.go | 6 ++---- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/pkg/instances/instances.go b/pkg/instances/instances.go index 1aeada67d4..55f2419115 100644 --- a/pkg/instances/instances.go +++ b/pkg/instances/instances.go @@ -122,7 +122,7 @@ func (i *Instances) ensureInstanceGroupAndPorts(name, zone string, ports []int64 return nil, err } } else { - klog.V(5).Infof("Instance group %v/%v already exists.", zone, name) + klog.V(2).Infof("Instance group %v/%v already exists.", zone, name) } // Build map of existing ports diff --git a/pkg/l4lb/l4netlbcontroller.go b/pkg/l4lb/l4netlbcontroller.go index 37d1653f33..130632615a 100644 --- a/pkg/l4lb/l4netlbcontroller.go +++ b/pkg/l4lb/l4netlbcontroller.go @@ -19,6 +19,7 @@ package l4lb import ( "fmt" "reflect" + "time" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" @@ -428,6 +429,12 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 return nil } + startTime := time.Now() + klog.Infof("Syncing L4 NetLB RBS service %s/%s", service.Namespace, service.Name) + defer func() { + klog.Infof("Finished syncing L4 NetLB RBS service %s/%s, time taken: %v", service.Namespace, service.Name, time.Since(startTime)) + }() + if err := common.EnsureServiceFinalizer(service, common.NetLBFinalizerV2, lc.ctx.KubeClient); err != nil { return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach L4 External LoadBalancer finalizer to service %s/%s, err %w", service.Namespace, service.Name, err)} } @@ -479,6 +486,11 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service) *loadbalancers.L4 } func (lc *L4NetLBController) ensureBackendLinking(service *v1.Service) error { + start := time.Now() + klog.V(2).Infof("Linking backend service with instance groups for service %s/%s", service.Namespace, service.Name) + defer func() { + klog.V(2).Infof("Finished linking backend service with instance groups for service %s/%s, time taken: %v", service.Namespace, service.Name, time.Since(start)) + }() zones, err := lc.translator.ListZones(utils.CandidateNodesPredicate) if err != nil { return err @@ -489,7 +501,6 @@ func (lc *L4NetLBController) ensureBackendLinking(service *v1.Service) error { servicePort := utils.ServicePort{ ID: portId, BackendNamer: lc.namer, - NodePort: utils.GetServiceNodePort(service), L4RBSEnabled: true, } @@ -499,16 +510,30 @@ func (lc *L4NetLBController) ensureBackendLinking(service *v1.Service) error { func (lc *L4NetLBController) ensureInstanceGroups(service *v1.Service, nodeNames []string) error { // TODO(kl52752) Move instance creation and deletion logic to NodeController // to avoid race condition between controllers - nodePorts := utils.GetNodePorts(service.Spec.Ports) - _, err := lc.instancePool.EnsureInstanceGroupsAndPorts(lc.ctx.ClusterNamer.InstanceGroup(), nodePorts) + start := time.Now() + klog.V(2).Infof("Ensuring instance groups for L4 NetLB Service %s/%s, len(nodeNames): %v", service.Namespace, service.Name, len(nodeNames)) + defer func() { + klog.V(2).Infof("Finished ensuring instance groups for L4 NetLB Service %s/%s, time taken: %v", service.Namespace, service.Name, time.Since(start)) + }() + + // L4 NetLB does not use node ports, so we provide empty slice + var nodePorts []int64 + igName := lc.ctx.ClusterNamer.InstanceGroup() + _, err := lc.instancePool.EnsureInstanceGroupsAndPorts(igName, nodePorts) if err != nil { - return err + return fmt.Errorf("lc.instancePool.EnsureInstanceGroupsAndPorts(%s, %v) returned error %w", igName, nodePorts, err) } + return lc.instancePool.Sync(nodeNames) } // garbageCollectRBSNetLB cleans-up all gce resources related to service and removes NetLB finalizer func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service) *loadbalancers.L4NetLBSyncResult { + startTime := time.Now() + klog.Infof("Deleting L4 NetLB RBS service %s/%s", svc.Namespace, svc.Name) + defer func() { + klog.Infof("Finished deleting L4 NetLB service %s/%s, time taken: %v", svc.Namespace, svc.Name, time.Since(startTime)) + }() l4NetLBParams := &loadbalancers.L4NetLBParams{ Service: svc, Cloud: lc.ctx.Cloud, diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index f08982f337..f816a2626a 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -650,18 +650,16 @@ func TestProcessServiceCreationFailed(t *testing.T) { addMockFunc func(*cloud.MockGCE) expectedError string }{{addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.GetHook = test.GetErrorInstanceGroupHook }, - expectedError: "GetErrorInstanceGroupHook"}, + expectedError: "lc.instancePool.EnsureInstanceGroupsAndPorts(k8s-ig--aaaaa, []) returned error GetErrorInstanceGroupHook"}, {addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.ListHook = test.ListErrorHook }, expectedError: "ListErrorHook"}, {addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.InsertHook = test.InsertErrorHook }, - expectedError: "InsertErrorHook"}, + expectedError: "lc.instancePool.EnsureInstanceGroupsAndPorts(k8s-ig--aaaaa, []) returned error InsertErrorHook"}, {addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.AddInstancesHook = test.AddInstancesErrorHook }, expectedError: "AddInstances: [AddInstancesErrorHook]"}, {addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.ListInstancesHook = test.ListInstancesWithErrorHook }, expectedError: "ListInstancesWithErrorHook"}, - {addMockFunc: func(c *cloud.MockGCE) { c.MockInstanceGroups.SetNamedPortsHook = test.SetNamedPortsErrorHook }, - expectedError: "SetNamedPortsErrorHook"}, } { lc := newL4NetLBServiceController() param.addMockFunc((lc.ctx.Cloud.Compute().(*cloud.MockGCE)))