Add a controller for handling L4 Internal LoadBalancer services #991

prameshj · 2020-01-13T21:00:16Z

This change adds a controller that will create the GCE resources(Forwarding rule, firewall rules, healthcheck, backend services) for Internal LoadBalancer services. The NEG creation will be done in the NEG controller and those changes are in PR - #959

Most of the code is similar to the service controller code in k/k.

@skmatti
/assign @MrHohn @freehan

pkg/backends/syncer.go

skmatti · 2020-01-14T18:20:37Z

pkg/controller/service_controller.go

+}
+
+func (l4c *L4Controller) processServiceDeletion(key string, svc *v1.Service) error {
+	nm, err := utils.ToNamespacedName(key)


Do we need this as nm.Namespace and svc.Namespace are same?.

method can be update to
func (l4c *L4Controller) processServiceDeletion(svc *v1.Service) error

I had added this because svc could be nil. But then I modified the main sync() to handle nil services. I am keeping the 'key' parameter to lookup the L4Handler.

pkg/controller/service_controller.go

pkg/utils/namer/namer.go

skmatti · 2020-01-14T19:57:30Z

pkg/utils/utils_test.go

+		{Desc: "Empty", Input: []int{}, Result: nil},
+	} {
+		result := GetPortRanges(tc.Input)
+		if !reflect.DeepEqual(result, tc.Result) {


The following implementation is much easier to debug test failures:
Refer: https://godoc.org/github.com/google/go-cmp/cmp#Diff

if diff := cmp.Diff(tc.Result, result); diff != "" { t.Errorf("GetPortRanges(%s) mismatch (-want +got):\n%s", tc.Desc, diff) }

Nice.. fixed.

pkg/utils/utils.go

MrHohn

Some drive-by comments. Will look more later:)

pkg/healthchecks/healthchecks_l4.go

pkg/backends/backends.go

pkg/controller/service_controller.go

pkg/healthchecks/healthchecks_l4.go

prameshj · 2020-01-17T01:08:36Z

Will work on rebasign and ping back for review. Thanks for the comments so far!

prameshj · 2020-01-17T20:53:20Z

I have fixed the merge conflicts and incorporated most of the comments. This PR is ready for another review.

@skmatti @MrHohn @freehan

skmatti

LGTM. small nits.

pkg/backends/backends.go

pkg/controller/service_controller.go

pkg/controller/service_controller_test.go

pkg/controller/translator/translator.go

freehan

Some structural suggestions.

freehan · 2020-02-01T00:22:50Z

pkg/controller/service_controller.go

@@ -0,0 +1,384 @@
+/*


maybe put this service controller into a separate pkg/l4 package.

freehan · 2020-02-01T00:47:11Z

pkg/controller/service_controller.go

+	}
+	updated := current.DeepCopy()
+	updated.Status.LoadBalancer = *newStatus
+	if _, err := svcClient.UpdateStatus(updated); err != nil {


There is a patch util in ingress-gce
https://github.com/kubernetes/ingress-gce/blob/master/pkg/utils/patch.go#L32

freehan · 2020-02-01T00:58:41Z

pkg/loadbalancers/forwarding_rules.go

+// getForwardingRule with the given name
+// rule with the given name exists with the correct parameters and creates a forwarding rule if
+// it does not exist already. If shouldExist parameter is true, this function returns if forwarding rule is not found.
+func (l *L4) ensureForwardingRule(loadBalancerName, bsLink string, options gce.ILBOptions) (*composite.ForwardingRule, error) {


Probably worth putting protocol in the forwarding rule name.

This is to leave future room to enable dual-protocol ILB

prameshj · 2020-02-04T18:28:20Z

/retest

freehan · 2020-02-07T00:50:55Z

cmd/glbc/main.go

@@ -218,6 +219,12 @@ func runControllers(ctx *ingctx.ControllerContext) {

 	fwc := firewalls.NewFirewallController(ctx, flags.F.NodePortRanges.Values())

+	if flags.F.RunL4Controller {
+		l4Controller := l4controller.NewL4Controller(ctx, stopCh)


nit: l4controller.NewL4Controller seems a bit redundant.

I would recommend naming package to l4. I would imagine this package to grow significantly over time.

freehan · 2020-02-07T21:15:35Z

pkg/l4controller/l4controller.go

+
+	l4c.handlerMap = make(map[string]*loadbalancers.L4)
+	l4c.svcQueue = utils.NewPeriodicTaskQueue("l4", "services", l4c.sync)
+	ctx.ServiceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{


I think the event handling here may be problematic. Consider this case:

The controller is down. Service with ILB is added. Then controller starts back up. Then the event will come in as Update instead of Add. (Please verify!) Then Update will get ignored because it does not have the finalizer.

I think the most conservative handling is to have Add/Update to be handled consistently.

Discussed in person. The exact case mentioned here will actually show up as an Add instead of Update upon controller restart. However, the logic here does not handle cases where a non-ILB service was updated to use ILB, as Minhan pointed out. Fixed this logic and added a test. Thanks!

freehan · 2020-02-07T21:15:46Z

pkg/l4controller/l4controller.go

+				} else {
+					oldSvc := old.(*v1.Service)
+					svcKey := utils.ServiceKeyFunc(curSvc.Namespace, curSvc.Name)
+					if l4c.needsUpdate(curSvc, oldSvc) {


I am wondering if this is needed.
needsUpdate or not. It may be worth reasserting the LB config?

I think you can filter service by if it has 1. finalizer V2 or 2. if it wants ILB.

yes, reworked this logic.
I included needsUpdate so we can avoid processing an update every time a finalizer is added/neg annotation is added/ status field is updated.

freehan · 2020-02-07T21:16:36Z

pkg/l4controller/l4controller.go

+	l4c.svcQueue.Shutdown()
+}
+
+func (l4c *L4Controller) getL4Handler(svcKey string, service *v1.Service) *loadbalancers.L4 {


why do you need to cache L4?

No strong reason. Mainly to avoid creating it over and over for the same service. I thought I could store references to already created resources in it, in future. Removed now, we can add it id the need arises.

freehan · 2020-02-07T21:22:32Z

pkg/l4controller/l4controller.go

+	needsILB, svcType := annotations.WantsL4ILB(svc)
+	if !needsILB {
+		klog.V(2).Infof("Service %s of type %s does not require L4 ILB, cleaning up resources", key, svcType)
+		return l4c.processServiceDeletion(key, svc)


nit:
I recommend merging the deletion condition handling into one function needsDeletion

`
func needToDeleteILB(....) bool {
must have ILBFinalizerV2
AND (has deletion timestamp OR is not type ILB)
to return true
}

`

good idea, done.

freehan · 2020-02-07T21:40:20Z

pkg/l4controller/l4controller.go

+	}
+	// Use the same function for both create and updates. If controller crashes and restarts,
+	// all existing services will show up as Service Adds.
+	status, err := l4.EnsureInternalLoadBalancer(nodeNames, service)


i see a bunch of SyncLoadBalancerFailed events. But no success event?

freehan · 2020-02-07T23:16:32Z

pkg/healthchecks/healthchecks_l4.go

+	return composite.DeleteHealthCheck(cloud, key, meta.VersionGA)
+}
+
+func HealthCheckName(shared bool, clusteruid, lbName string) (string, string) {


Can you add comment on the naming? For instance, why there is 2 firewall names

freehan · 2020-02-07T23:24:11Z

pkg/loadbalancers/l4.go

+		}
+		return nil
+	}
+	err = deleteFunc(name)


is this the correct name?

add a comment why needs to delete 2 firewalls.

prameshj · 2020-02-18T21:49:47Z

/assign @freehan

freehan

This can be a follow up. Remember to collect usage metrics for primary_VM_IP NEG. https://github.com/kubernetes/ingress-gce/blob/master/pkg/metrics/features.go#L78

/lgtm
/approve

freehan · 2020-02-19T22:49:29Z

pkg/backends/syncer.go

@@ -178,6 +178,10 @@ func (s *backendSyncer) GC(svcPorts []utils.ServicePort) error {
 // gc deletes the provided backends
 func (s *backendSyncer) gc(backends []*composite.BackendService, knownPorts sets.String) error {
 	for _, be := range backends {
+		// Skip L4 LB backend services


Add comment:
backendSyncer currently only GC backend services for L7 XLB/ILB.
L4 LB is GC as part of the deletion follow as there is no shared backend services among L4 ILBs.

translator imports loadbalancer which imports firewalls, which again depends on translator. Removed the translator -> loadbalancer dependency.

Fixes including adding CustomSubnet support and self links for backend service, healthcheck. Removed the operation from queue key Added connectiondraining to backend service. Added a new flag to run L4 controller. Used namer.PrimaryIPNEG() instead of namer.NEG() Use a common description for all ILB resources. Added locking for shared healthchecks. Only process relevant updates in l4 controller Renamed files, used patch instead of update. unit tests for L4 ILB Most of the tests in l4_test.go are from gce_loadbalancer_internal_test.go

prameshj · 2020-02-20T01:03:44Z

This can be a follow up. Remember to collect usage metrics for primary_VM_IP NEG. https://github.com/kubernetes/ingress-gce/blob/master/pkg/metrics/features.go#L78

/lgtm
/approve

Good point, we need to add usage metrics for this L4 controller as well. Will cover PRIMARY_VM_IP NEG as part of that too. Thanks!

freehan · 2020-02-20T01:18:39Z

/lgtm

k8s-ci-robot · 2020-02-20T01:19:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj

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

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 assigned freehan and MrHohn Jan 13, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 13, 2020

k8s-ci-robot requested review from MrHohn and rramkumar1 January 13, 2020 21:00

skmatti reviewed Jan 14, 2020

View reviewed changes

MrHohn reviewed Jan 16, 2020

View reviewed changes

prameshj force-pushed the ilb-fr branch from 43b284a to c9dc68c Compare January 17, 2020 01:04

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

prameshj force-pushed the ilb-fr branch from c9dc68c to a39607f Compare January 17, 2020 07:37

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

prameshj force-pushed the ilb-fr branch from a39607f to 99775b5 Compare January 17, 2020 20:29

prameshj force-pushed the ilb-fr branch 2 times, most recently from bc46338 to 44fdbb5 Compare January 31, 2020 00:16

skmatti reviewed Jan 31, 2020

View reviewed changes

pkg/backends/backends.go Show resolved Hide resolved

pkg/controller/service_controller.go Outdated Show resolved Hide resolved

pkg/controller/service_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/translator/translator.go Outdated Show resolved Hide resolved

prameshj force-pushed the ilb-fr branch from 44fdbb5 to f6a1b2c Compare January 31, 2020 23:57

freehan reviewed Feb 1, 2020

View reviewed changes

prameshj force-pushed the ilb-fr branch 2 times, most recently from aaa50b1 to 734e3d5 Compare February 6, 2020 19:19

freehan reviewed Feb 7, 2020

View reviewed changes

prameshj force-pushed the ilb-fr branch from 734e3d5 to b0993d0 Compare February 8, 2020 14:45

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

prameshj force-pushed the ilb-fr branch from b0993d0 to 417eedb Compare February 11, 2020 07:20

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

freehan reviewed Feb 19, 2020

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 19, 2020

prameshj added 3 commits February 19, 2020 16:53

Moved constants to remove cyclic dependency.

b8a4d8a

translator imports loadbalancer which imports firewalls, which again depends on translator. Removed the translator -> loadbalancer dependency.

Autogenerated vendor changes.

e2c4c2d

prameshj force-pushed the ilb-fr branch from 417eedb to e2c4c2d Compare February 20, 2020 01:02

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2020

k8s-ci-robot merged commit 444fd52 into kubernetes:master Feb 20, 2020

Add a controller for handling L4 Internal LoadBalancer services #991

Add a controller for handling L4 Internal LoadBalancer services #991

Conversation

prameshj commented Jan 13, 2020

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

MrHohn left a comment

Choose a reason for hiding this comment

prameshj commented Jan 17, 2020

prameshj commented Jan 17, 2020

skmatti left a comment

Choose a reason for hiding this comment

freehan left a 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

prameshj commented Feb 4, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prameshj commented Feb 18, 2020

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prameshj commented Feb 20, 2020

freehan commented Feb 20, 2020

k8s-ci-robot commented Feb 20, 2020