Modify the NEG controller to also create NEGs for L4 External Load Ba… #2155

mmamczur · 2023-05-29T13:51:20Z

…lancers that are using multinetworking.

k8s-ci-robot · 2023-05-29T13:51:29Z

Hi @mmamczur. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

code-elinka · 2023-05-29T14:21:31Z

nit: modify the commit message:

Change NEG controller to support multinetwork

The NEG controller should create NEGs for the
L4 External Load Balancers that use multinetworking.

https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

code-elinka · 2023-05-29T14:23:47Z

pkg/annotations/service.go

+		return false
+	}
+
+	if val, ok := service.Annotations[RBSAnnotationKey]; ok && val == RBSEnabled {


nit: Do you want to add check for ok as well? Like it was done in func WantsL4NetLB(

I'm not sure what you mean, this code does check the ok variable: ok && val == RBSEnabled

I actually just copied an old function from netlb controller and use this one instead not to duplicate

but this reminded me that I forgot to remove the old function

I mean that maybe it's nice to return the error in case it is not ok.

But I don't have a strong opinion on it

not ok means the annotation is not present at all, which means the service doesn't have it, that isn't an error state

mmamczur · 2023-05-29T14:37:50Z

nit: modify the commit message:
...

done

code-elinka · 2023-05-30T12:24:16Z

cmd/glbc/main.go

@@ -371,7 +372,7 @@ func runControllers(ctx *ingctx.ControllerContext) {
 		flags.F.NumNegGCWorkers,
 		flags.F.EnableReadinessReflector,
 		flags.F.RunIngressController,
-		flags.F.RunL4Controller,
+		flags.F.RunL4Controller || enableNEGsForNetLB,


Why is it OR?

discussed offline: RunL4Controller is a name for Run Internal L4 Controller. Usually there is a naming:

just LB means internal

NetLB means external

Does that mean we do not support having ILB subsetting disabled when L4 Net LB negs is enabled?

I think I forgot to hit send on the comment I wrote before

there is another check to see if the service is really subsetting. So it will be possible to have non subsetting ILBs and multinet at the same time (but legacy ILB won't support multinet so I see no point in doing that)

code-elinka · 2023-05-30T12:27:32Z

pkg/l4lb/l4netlbcontroller.go

-		return true
-	}
-	return false
+	return annotations.HasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)


I know that this is not related to your change, but maybe you know why do we say that it's RBS if it has Netlb Finalizer? Or it is about the fact that it's Finalizer V2?

@cezarygerard Could you please elaborate on this logic?

@code-elinka

this is just a bad naming

HasL4NetLBFinalizerV2 maybe it should be HasRBSFinalizer

code-elinka · 2023-05-30T13:17:45Z

/ok-to-test

swetharepakula · 2023-05-31T22:54:10Z

cmd/glbc/main.go

@@ -371,7 +372,7 @@ func runControllers(ctx *ingctx.ControllerContext) {
 		flags.F.NumNegGCWorkers,
 		flags.F.EnableReadinessReflector,
 		flags.F.RunIngressController,
-		flags.F.RunL4Controller,
+		flags.F.RunL4Controller || enableNEGsForNetLB,


Does that mean we do not support having ILB subsetting disabled when L4 Net LB negs is enabled?

swetharepakula · 2023-05-31T23:15:46Z

pkg/neg/controller.go

-	if wantsILB, _ := annotations.WantsL4ILB(service); !wantsILB {
+	wantsILB, _ := annotations.WantsL4ILB(service)
+	wantsNetLB, _ := annotations.WantsL4NetLB(service)
+	needsNEGForNetLB := wantsNetLB && !networkInfo.IsDefault && annotations.HasRBSAnnotation(service)


does this mean we only support NEGs for NEGLB if using the non default network?

yes
The reason for this is that NetLB NEG will not be GA when we preview this. With non multinet NetLB being GA, I don't think we can use backends that are not GA.

as for the quetion about - subsetting, there is another check https://github.com/kubernetes/ingress-gce/pull/2155/files/813d3551b391e33b4a00a9bb1003555fad0b4ce8#diff-6fdd3264dcdaccfcfd6c0a41aee9fbf719529a11b24c94609de61b1f038306efR599 if wantsILB && !utils.IsSubsettingL4ILBService(service) - the controller will ignore the service if it isn't subsetting ILB

swetharepakula · 2023-06-05T22:51:28Z

pkg/neg/controller_test.go

+	validateSyncerManagerWithPortInfoMap(t, controller, testServiceNamespace, testServiceName, expectedPortInfoMap)
+	validateServiceAnnotationWithPortInfoMap(t, svc, expectedPortInfoMap, expectZones)
+
+	time.Sleep(1 * time.Second)


What is the sleep here for?

I actually copied and modified an existing testcase (check line 414 in this file). I assume it has something to do with waiting before GC?

swetharepakula · 2023-06-05T22:55:22Z

Mostly looks good to me. Just a few clarification questions

cezarygerard · 2023-05-30T13:18:01Z

pkg/l4lb/l4netlbcontroller.go

-		return true
-	}
-	return false
+	return annotations.HasRBSAnnotation(svc) || utils.HasL4NetLBFinalizerV2(svc) || lc.hasRBSForwardingRule(svc)


@code-elinka

this is just a bad naming

HasL4NetLBFinalizerV2 maybe it should be HasRBSFinalizer

cezarygerard · 2023-06-14T08:28:00Z

pkg/neg/controller.go

@@ -589,11 +589,14 @@ func (c *Controller) mergeStandaloneNEGsPortInfo(service *apiv1.Service, name ty

 // mergeVmIpNEGsPortInfo merges the PortInfo for ILB services using GCE_VM_IP NEGs into portInfoMap


please update this comment

this is just bad naming

HasL4NetLBFinalizerV2 maybe it should be HasRBSFinalizer

I haven't touched this function actually, I only moved HasRBSAnnotation to another place

I can rename HasL4NetLBFinalizerV2 but it's not directly related to this change

please update this comment

updated

pkg/neg/controller_test.go

cezarygerard · 2023-06-14T08:48:42Z

it's LGTM overall from my side

The NEG controller should create NEGs for the L4 External Load Balancers that use multinetworking.

cezarygerard · 2023-06-14T15:37:58Z

/lgtm

k8s-ci-robot · 2023-06-14T15:38:06Z

[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]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 29, 2023

k8s-ci-robot requested review from code-elinka and gauravkghildiyal May 29, 2023 13:51

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 29, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2023

code-elinka reviewed May 29, 2023

View reviewed changes

mmamczur force-pushed the mult-networking-neg-netlb branch from 6d972f9 to bd657d2 Compare May 29, 2023 14:36

mmamczur force-pushed the mult-networking-neg-netlb branch from bd657d2 to 813d355 Compare May 29, 2023 14:39

code-elinka reviewed May 30, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 30, 2023

swetharepakula reviewed Jun 5, 2023

View reviewed changes

cezarygerard reviewed Jun 14, 2023

View reviewed changes

Change NEG controller to support multinetwork L4 external LBs

917c531

The NEG controller should create NEGs for the L4 External Load Balancers that use multinetworking.

mmamczur force-pushed the mult-networking-neg-netlb branch from 813d355 to 917c531 Compare June 14, 2023 12:44

k8s-ci-robot assigned cezarygerard Jun 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2023

k8s-ci-robot merged commit 6e5fafd into kubernetes:master Jun 14, 2023
5 checks passed

mmamczur mentioned this pull request Aug 9, 2023

REQUEST: New membership for mmamczur kubernetes/org#4372

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the NEG controller to also create NEGs for L4 External Load Ba… #2155

Modify the NEG controller to also create NEGs for L4 External Load Ba… #2155

mmamczur commented May 29, 2023

k8s-ci-robot commented May 29, 2023

code-elinka commented May 29, 2023

code-elinka May 29, 2023

mmamczur May 29, 2023

mmamczur May 29, 2023

code-elinka May 30, 2023

code-elinka May 30, 2023

mmamczur May 30, 2023

mmamczur commented May 29, 2023

code-elinka May 30, 2023

code-elinka May 30, 2023

swetharepakula May 31, 2023

mmamczur Jun 12, 2023

code-elinka May 30, 2023

code-elinka May 30, 2023

cezarygerard May 30, 2023 •

edited

Loading

code-elinka commented May 30, 2023

swetharepakula May 31, 2023

swetharepakula May 31, 2023

mmamczur Jun 7, 2023

mmamczur Jun 7, 2023

swetharepakula Jun 5, 2023

mmamczur Jun 7, 2023

swetharepakula commented Jun 5, 2023

cezarygerard May 30, 2023 •

edited

Loading

cezarygerard Jun 14, 2023

mmamczur Jun 14, 2023

mmamczur Jun 14, 2023

cezarygerard commented Jun 14, 2023

cezarygerard commented Jun 14, 2023

k8s-ci-robot commented Jun 14, 2023

		@@ -589,11 +589,14 @@ func (c Controller) mergeStandaloneNEGsPortInfo(service apiv1.Service, name ty

		// mergeVmIpNEGsPortInfo merges the PortInfo for ILB services using GCE_VM_IP NEGs into portInfoMap

Modify the NEG controller to also create NEGs for L4 External Load Ba… #2155

Modify the NEG controller to also create NEGs for L4 External Load Ba… #2155

Conversation

mmamczur commented May 29, 2023

k8s-ci-robot commented May 29, 2023

code-elinka commented May 29, 2023

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

mmamczur commented May 29, 2023

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 May 30, 2023 • edited Loading

Choose a reason for hiding this comment

code-elinka commented May 30, 2023

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

swetharepakula commented Jun 5, 2023

cezarygerard May 30, 2023 • edited Loading

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 Jun 14, 2023

cezarygerard commented Jun 14, 2023

k8s-ci-robot commented Jun 14, 2023

cezarygerard May 30, 2023 •

edited

Loading

cezarygerard May 30, 2023 •

edited

Loading