-
Notifications
You must be signed in to change notification settings - Fork 298
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.
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
NEG controller part - L4 RBS External LB NEG support #2566
base: master
Are you sure you want to change the base?
NEG controller part - L4 RBS External LB NEG support #2566
Conversation
3633223
to
d167e04
Compare
d167e04
to
8c10715
Compare
/assign @swetharepakula |
/hold let's not merge this before the next code freeze ends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though the core logic of selecting endpoints has not changed, I think we need more tests to cover this new knob.
@@ -37,7 +37,7 @@ import ( | |||
"k8s.io/klog/v2" | |||
) | |||
|
|||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4ILBEndpointsCalculator. | |||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4EndpointsCalculator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests to cover the logic of switching between netlb and ilb?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good point, adding a test here won't help much. To sum up
The NetLB and ILB will use the same syncing logic but with different subset sizes.
The endpoint calculators are created together with the syncer and keep 'living' in the controller.
There is no mechanism to update the running syncers. What we can do here is to treat these syncers as different. When the service is moved ILB <-> NetLB just stop the old syncer and create a new one. I added the LB type to the syncer key, this will achieve just that.
I intended to add a test for this in the controller_test, it would work but there is a problem with the fake SvcNEG indexer and client. When a SvcNEG is created the indexer doesn't know. There is code that checks if an object extsts using the indexer and then uses the client to delete
ingress-gce/pkg/neg/manager.go
Lines 457 to 475 in c6b0781
obj, exists, err := manager.svcNegLister.GetByKey(fmt.Sprintf("%s/%s", namespace, negName)) | |
if err != nil { | |
return fmt.Errorf("failed retrieving neg %s/%s to delete: %w", namespace, negName, err) | |
} | |
if !exists { | |
return nil | |
} | |
neg := obj.(*negv1beta1.ServiceNetworkEndpointGroup) | |
if neg.GetDeletionTimestamp().IsZero() { | |
start := time.Now() | |
err = manager.svcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(namespace).Delete(context.Background(), negName, metav1.DeleteOptions{}) | |
metrics.PublishK8sRequestCountMetrics(start, metrics.DeleteRequest, err) | |
if err != nil { | |
return fmt.Errorf("errored while deleting neg cr %s/%s: %w", negName, namespace, err) | |
} | |
manager.logger.V(2).Info("Deleted neg cr", "svcneg", klog.KRef(namespace, negName)) | |
} | |
return nil |
Not sure how to resolve it. Maybe just e2e test that handles that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unfortunately we have had to manage it by keep the K8s store and the indexer in sync which is not ideal.
Adding the LBType to the syncer is a good option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the change to the syncer key. Did you mean the portinfo struct?
@@ -158,6 +158,7 @@ func newTestControllerWithParamsAndContext(kubeClient kubernetes.Interface, test | |||
labels.PodLabelPropagationConfig{}, | |||
true, | |||
false, | |||
false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add tests where this is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a test case to verify combinations of this being turned off etc
8c10715
to
c6b0781
Compare
c6b0781
to
59cfac4
Compare
7acfed4
to
8645452
Compare
2f44c5c
to
7a219f0
Compare
I have reviewed this changes in #2565 it looks good to me please merge these as soon as we have LGMT from Swetha and Gaurav |
7a219f0
to
4f80499
Compare
Looks good to me, will leave the final approval to @swetharepakula /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmamczur, sawsa307 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:
Approvers can indicate their approval by writing |
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.
4f80499
to
09281f9
Compare
@@ -281,6 +283,8 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 | |||
flag.BoolVar(&F.RunL4NetLBController, "run-l4-netlb-controller", false, `Optional, if enabled then the L4NetLbController will be run.`) | |||
flag.BoolVar(&F.EnableNEGController, "enable-neg-controller", true, `Optional, if enabled then the NEG controller will be run.`) | |||
flag.BoolVar(&F.EnableL4NEG, "enable-l4-neg", false, `Optional, if enabled then the NEG controller will process L4 NEGs.`) | |||
flag.BoolVar(&F.EnableL4NetLBNEG, "enable-l4-netlb-neg", false, `Optional, if enabled then the NetLB controller can create L4 NetLB services with NEG backends.`) | |||
flag.BoolVar(&F.EnableL4NetLBNEGDefault, "enable-l4-netlb-neg-default", false, `Optional, if enabled then newly created L4 NetLB services will use NEG backends. Has effect only if '--enable-l4-netlb-neg' is set to true.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these interact with EnableL4NEG? I think there should be a requirement that EnableL4NEG is enabled too for these new flags
@@ -97,13 +97,16 @@ type Controller struct { | |||
// syncerMetrics collects NEG controller metrics | |||
syncerMetrics *syncMetrics.SyncerMetrics | |||
|
|||
// runL4 indicates whether to run NEG controller that processes L4 services | |||
runL4 bool | |||
// runL4ForILB indicates whether to run NEG controller that processes L4 ILB services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this is to preserve the existing behavior and be able to gate the new logic.
nit, the existing behavior also turns on NEGs for multi-networking which isn't captured by this boolean.
@@ -503,7 +508,8 @@ func (c *Controller) processService(key string) error { | |||
} | |||
} | |||
|
|||
if c.runL4 { | |||
// Create L4 PortInfo if subsetting is enable `runL4ForILB==true` or RBS NEG support is enabled `runL4ForNetLB==true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or if multi networking is enabled?
@@ -597,13 +603,13 @@ func (c *Controller) mergeStandaloneNEGsPortInfo(service *apiv1.Service, name ty | |||
// mergeVmIpNEGsPortInfo merges the PortInfo for ILB and multinet NetLB services using GCE_VM_IP NEGs into portInfoMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update the comments to include NEG default
@@ -503,7 +508,8 @@ func (c *Controller) processService(key string) error { | |||
} | |||
} | |||
|
|||
if c.runL4 { | |||
// Create L4 PortInfo if subsetting is enable `runL4ForILB==true` or RBS NEG support is enabled `runL4ForNetLB==true`. | |||
if c.runL4ForILB || c.runL4ForNetLB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we check for the variables again in the utils function, do we need the check here still?
@@ -724,8 +724,8 @@ func TestEndpointsCalculatorMode(t *testing.T) { | |||
portInfoMap PortInfoMap | |||
expectMode EndpointsCalculatorMode | |||
}{ | |||
{"L4 Local Mode", NewPortInfoMapForVMIPNEG("testns", "testsvc", testContext.L4Namer, true, defaultNetwork), L4LocalMode}, | |||
{"L4 Cluster Mode", NewPortInfoMapForVMIPNEG("testns", "testsvc", testContext.L4Namer, false, defaultNetwork), L4ClusterMode}, | |||
{"L4 Local Mode", NewPortInfoMapForVMIPNEG("testns", "testsvc", testContext.L4Namer, true, defaultNetwork, L4InternalLB), L4LocalMode}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add L4ExternalLB cases here too?
@@ -37,7 +37,7 @@ import ( | |||
"k8s.io/klog/v2" | |||
) | |||
|
|||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4ILBEndpointsCalculator. | |||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4EndpointsCalculator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, unfortunately we have had to manage it by keep the K8s store and the indexer in sync which is not ideal.
Adding the LBType to the syncer is a good option
@@ -37,7 +37,7 @@ import ( | |||
"k8s.io/klog/v2" | |||
) | |||
|
|||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4ILBEndpointsCalculator. | |||
// TestLocalGetEndpointSet verifies the GetEndpointSet method implemented by the LocalL4EndpointsCalculator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the change to the syncer key. Did you mean the portinfo struct?
This PR contains changes for the NEG controller to support L4 NetLB services.
The NEG controller will be triggered for NetLBs with the presence of the new netlb V3 finalizer
Also the PortInfo had to be extended to contain the type of L4 LB since the calculators need this info in order to pick the right subset sizes. NetLBs will use larger subsets than ILB. Also this allows to differentiate a NEG syncer for ILB vs one for NetLB, when the service moves between ILB and NetLB the syncer for the old service spec will be stopped and a new one created for the changed service.