L4 RBS External LB NEG support #2565

mmamczur · 2024-05-23T13:24:34Z

This PR contains the implementation of NEG variant of the L4 RBS LoadBalancer

mmamczur · 2024-05-23T13:52:04Z

/assign @cezarygerard

mmamczur · 2024-05-28T14:56:55Z

/assign @swetharepakula
/assign @sawsa307

mmamczur · 2024-05-29T07:38:19Z

/unassign @swetharepakula

mmamczur · 2024-05-29T07:38:40Z

/unassign @sawsa307

mmamczur · 2024-05-29T15:54:16Z

/hold

let's not merge this before the next code freeze ends

cezarygerard · 2024-09-08T23:03:21Z

pkg/neg/syncers/transaction.go

@@ -191,15 +191,15 @@ func NewTransactionSyncer(
 	return syncer
 }

-func GetEndpointsCalculator(podLister, nodeLister, serviceLister cache.Indexer, zoneGetter *zonegetter.ZoneGetter, syncerKey negtypes.NegSyncerKey, mode negtypes.EndpointsCalculatorMode, logger klog.Logger, enableDualStackNEG bool, syncMetricsCollector *metricscollector.SyncerMetrics, networkInfo *network.NetworkInfo) negtypes.NetworkEndpointsCalculator {
+func GetEndpointsCalculator(podLister, nodeLister, serviceLister cache.Indexer, zoneGetter *zonegetter.ZoneGetter, syncerKey negtypes.NegSyncerKey, mode negtypes.EndpointsCalculatorMode, logger klog.Logger, enableDualStackNEG bool, syncMetricsCollector *metricscollector.SyncerMetrics, networkInfo *network.NetworkInfo, l4LBType negtypes.L4LBType) negtypes.NetworkEndpointsCalculator {


this arg list begs for refactor in the future

cezarygerard · 2024-09-08T23:12:16Z

pkg/neg/syncers/subsets.go

+	// Max number of subsets for NetLB in ExternalTrafficPolicy:Local
+	maxSubsetSizeNetLBLocal = 1000
+	// Max number of subsets for NetLB in ExternalTrafficPolicy:Cluster
+	maxSubsetSizeNetLBCluster = 250


what subsets sizes were used for netlb multinetworking?

does this change sizes used for netlb multinetworking now?

multinet uses the same sizes as ILB.
I see no harm in changing this, actually this whole effort will improve multinet

cezarygerard · 2024-09-08T23:15:31Z

pkg/neg/syncers/endpoints_calculator.go

 // It exposes methods to calculate Network endpoints for GCE_VM_IP NEGs when the service
 // uses "ExternalTrafficPolicy: Local" mode.
 // In this mode, the endpoints of the NEG are calculated by listing the nodes that host the service endpoints(pods)
 // for the given service. These candidate nodes picked as is, if the count is less than the subset size limit(250).
 // Otherwise, a subset of nodes is selected.
 // In a cluster with nodes node1... node 50. If nodes node10 to node 45 run the pods for a given ILB service, all these
 // nodes - node10, node 11 ... node45 will be part of the subset.
-type LocalL4ILBEndpointsCalculator struct {
+type LocalL4EndpointsCalculator struct {


struct name is changing now, but we could have done it in multintworking change, right?

cezarygerard · 2024-09-09T06:49:03Z

pkg/l4lb/l4netlbcontroller.go

-		return &loadbalancers.L4NetLBSyncResult{Error: fmt.Errorf("Failed to attach L4 External LoadBalancer finalizer to service %s/%s, err %w", service.Namespace, service.Name, err)}
+	usesNegBackends := false
+
+	if lc.enableNEGSupport || utils.HasL4NetLBFinalizerV3(service) {


what if we enabled neg support, but there is v2 finalizer?

we should not migrate automatically

yeah this is actually wrong, thanks for pointing it out. It's a leftover of changing the logic of how we introduce the NEG variant

ok I changed this

cezarygerard · 2024-09-09T06:52:24Z

I have 1 major comment

you should take V2 finalizer into account and not migrate to V3 event if negs are supported.

cezarygerard · 2024-09-11T09:29:33Z

pkg/l4lb/l4netlbcontroller.go

-	if err := common.EnsureServiceFinalizer(service, common.NetLBFinalizerV2, lc.ctx.KubeClient, svcLogger); 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)}
+	usesNegBackends := false
+


NIT: please add comment above this if explaining why we usesNegBackends

we should not orphan sevices with NetLBFinalizerV3
in casewe had to rollback the feature

if utils.HasL4NetLBFinalizerV3(service) || (lc.enableNEGSupport && !utils.HasL4NetLBFinalizerV2(service) )

please don't hesitate to use brackets, even if they are redundant

I extracted this to a separate function which should be easier to follow, it was becoming a mess.

cezarygerard · 2024-09-11T09:33:05Z

pkg/l4lb/l4netlbcontroller_test.go

@@ -560,6 +560,101 @@ func TestProcessMultinetServiceCreate(t *testing.T) {
 	deleteNetLBService(lc, svc)
 }

+func TestProcessNEGServiceCreate(t *testing.T) {


does it make sense to add quick test on scenario with service using V3 finalizer but lc.enableNEGSupport = false

I added a few tests including one for what you describe.
Others are mostly testing 'rollback like' scenarios

cezarygerard · 2024-09-11T09:34:04Z

good job Michał!

lgtm overall

please read my comments

pkg/l4lb/l4netlbcontroller.go

cezarygerard · 2024-09-11T21:55:25Z

pkg/l4lb/l4netlbcontroller.go

+	if usesNegBackends {
+		ensureFinalizer = common.NetLBFinalizerV3
+	}
+	if err := common.EnsureServiceFinalizer(service, ensureFinalizer, lc.ctx.KubeClient, svcLogger); err != nil {


this will change finalizer on existing multinet services, did you consider it?
OTOH probably there is no way to avoid it

it won't, multinet is treated differently for now - usesNegBackends won't be true for them, there is a separate condition below. But it would be good to move them to V3 at some point and unify that logic.

pkg/l4lb/l4netlbcontroller_test.go

cezarygerard · 2024-09-11T22:12:23Z

pkg/l4lb/l4netlbcontroller.go

 		linkType = negLink
 	}

 	if err = lc.ensureBackendLinking(service, linkType, svcLogger); err != nil {
 		lc.ctx.Recorder(service.Namespace).Eventf(service, v1.EventTypeWarning, "SyncExternalLoadBalancerFailed",
-			"Error linking instance groups to backend service, err: %v", err)
+			"Error linking backends to backend service, err: %v", err)


what if Neg is not provisioned yet?

syncInternal should fail and retry immediately, right?

yes, the same mechanism as for ILB

cezarygerard · 2024-09-11T22:17:03Z

pkg/utils/common/finalizer.go

@@ -123,3 +125,26 @@ func EnsureDeleteServiceFinalizer(service *corev1.Service, key string, kubeClien
 	svcLogger.V(2).Info("Removing finalizer from service", "finalizerKey", key)
 	return patch.PatchServiceObjectMetadata(kubeClient.CoreV1(), service, *updatedObjectMeta)
 }
+
+// EnsureServiceDeleteFinalizers patches the service to ensure the specified finalizers are not present in the service finalizers list.
+// This function is needed if more than one finalikzer has to be removed since you can't invoke the 1 param version multiple times.


// This function is needed if more than one finalizer has to be removed since it is very inefficient to invoke 1-param version multiple times.

modified, discussed offline

cezarygerard · 2024-09-11T22:32:40Z

I only have few readability comments

the logic looks good to me

cezarygerard · 2024-09-12T12:01:48Z

/lgtm

k8s-ci-robot · 2024-09-12T12:01:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, mmamczur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cezarygerard,mmamczur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-09-12T14:57:34Z

New changes are detected. LGTM label has been removed.

Adds 2 new flags related to NetLB RBS NEG support. `--enable-l4-netlb-neg` that will allow to create NEGs for L4 NetLBs. `--enable-l4-netlb-neg-default` which will make all new RBS services use NEG backends.

NetLBFinalizerV3 will be added to the L4 RBS NetLBs that opt-in for NEG support.

When enabled, the NEG controller will create NEGs for L4 RBS NetLBs that are marked by the L4 NetLB controller to be NEG based.

When the feature is enabled the controller can create NEG backed LBs. When the 'NEG default flag' is enabled, all new RBS NetLB services will be created with NEG backends.

k8s-ci-robot requested review from gauravkghildiyal and sawsa307 May 23, 2024 13:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2024

mmamczur force-pushed the ext_l4_lb_negs branch 2 times, most recently from e6b84dd to 2117076 Compare May 23, 2024 13:49

k8s-ci-robot assigned cezarygerard May 23, 2024

mmamczur force-pushed the ext_l4_lb_negs branch 3 times, most recently from df95f72 to c9795fc Compare May 24, 2024 11:00

k8s-ci-robot assigned sawsa307 and swetharepakula May 28, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2024

mmamczur force-pushed the ext_l4_lb_negs branch from c9795fc to f557345 Compare May 29, 2024 07:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2024

k8s-ci-robot unassigned swetharepakula May 29, 2024

k8s-ci-robot unassigned sawsa307 May 29, 2024

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 29, 2024

mmamczur force-pushed the ext_l4_lb_negs branch from f557345 to d3eab54 Compare June 10, 2024 10:35

mmamczur force-pushed the ext_l4_lb_negs branch from d3eab54 to c5d4633 Compare July 17, 2024 08:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2024

mmamczur force-pushed the ext_l4_lb_negs branch 2 times, most recently from f2e439f to cb279cf Compare August 13, 2024 13:56

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024

mmamczur force-pushed the ext_l4_lb_negs branch from cb279cf to dcff02d Compare September 6, 2024 08:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2024

cezarygerard reviewed Sep 9, 2024

View reviewed changes

mmamczur force-pushed the ext_l4_lb_negs branch from dcff02d to 24e11c3 Compare September 9, 2024 09:07

cezarygerard reviewed Sep 11, 2024

View reviewed changes

mmamczur force-pushed the ext_l4_lb_negs branch from 24e11c3 to 528fadb Compare September 11, 2024 14:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2024

cezarygerard reviewed Sep 11, 2024

View reviewed changes

mmamczur force-pushed the ext_l4_lb_negs branch from 528fadb to 9265ae3 Compare September 12, 2024 10:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

cezarygerard mentioned this pull request Sep 12, 2024

NEG controller part - L4 RBS External LB NEG support #2566

Open

mmamczur force-pushed the ext_l4_lb_negs branch from 9265ae3 to 34f18ff Compare September 12, 2024 14:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

mmamczur added 4 commits September 19, 2024 11:16

Add flags for L4 RBS NetLB NEGs.

d117176

Adds 2 new flags related to NetLB RBS NEG support. `--enable-l4-netlb-neg` that will allow to create NEGs for L4 NetLBs. `--enable-l4-netlb-neg-default` which will make all new RBS services use NEG backends.

NetLBFinalizerV3 for NEG variant of L4 RBS NetLB.

03463b1

NetLBFinalizerV3 will be added to the L4 RBS NetLBs that opt-in for NEG support.

NEG Controller support for L4 NetLB.

09281f9

When enabled, the NEG controller will create NEGs for L4 RBS NetLBs that are marked by the L4 NetLB controller to be NEG based.

L4 RBS External LB NEG support.

d196aa1

When the feature is enabled the controller can create NEG backed LBs. When the 'NEG default flag' is enabled, all new RBS NetLB services will be created with NEG backends.

mmamczur force-pushed the ext_l4_lb_negs branch from 34f18ff to d196aa1 Compare September 19, 2024 09:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L4 RBS External LB NEG support #2565

L4 RBS External LB NEG support #2565

mmamczur commented May 23, 2024

mmamczur commented May 23, 2024

mmamczur commented May 28, 2024

mmamczur commented May 29, 2024

mmamczur commented May 29, 2024

mmamczur commented May 29, 2024

cezarygerard Sep 8, 2024

cezarygerard Sep 8, 2024

mmamczur Sep 9, 2024

cezarygerard Sep 8, 2024

mmamczur Sep 9, 2024

cezarygerard Sep 9, 2024

mmamczur Sep 9, 2024

mmamczur Sep 9, 2024

cezarygerard commented Sep 9, 2024

cezarygerard Sep 11, 2024

cezarygerard Sep 11, 2024

mmamczur Sep 11, 2024

cezarygerard Sep 11, 2024

mmamczur Sep 11, 2024

cezarygerard commented Sep 11, 2024

cezarygerard Sep 11, 2024

mmamczur Sep 12, 2024

cezarygerard Sep 11, 2024

mmamczur Sep 12, 2024

cezarygerard Sep 11, 2024

mmamczur Sep 12, 2024

cezarygerard commented Sep 11, 2024

cezarygerard commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

L4 RBS External LB NEG support #2565

Are you sure you want to change the base?

L4 RBS External LB NEG support #2565

Conversation

mmamczur commented May 23, 2024

mmamczur commented May 23, 2024

mmamczur commented May 28, 2024

mmamczur commented May 29, 2024

mmamczur commented May 29, 2024

mmamczur commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezarygerard commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezarygerard commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezarygerard commented Sep 11, 2024

cezarygerard commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024