Add support for LoadBalancerClass by fedepaol · Pull Request #1417 · metallb/metallb

fedepaol · 2022-06-07T12:50:21Z

Loadbalancer class enable different loadbalancers to co-exist. We add a parameter to the speaker and the controller to process only the services consistent with the loadbalancer class provided to metallb.

Fixes #685 and #996

oribon

nice!

oribon · 2022-06-08T06:00:35Z

charts/metallb/README.md

 | controller.tolerations | list | `[]` |  |
 | fullnameOverride | string | `""` |  |
 | imagePullSecrets | list | `[]` |  |
+| loadbalancerClass | string | `""` |  |


nit: should be loadBalancerClass

oribon · 2022-06-08T06:20:00Z

internal/k8s/controllers/service_controller.go

 	}

+	if filterByLoadbalancerClass(service, r.LoadBalancerClass) {
+		level.Info(r.Logger).Log("controller", "ServiceReconciler", "filtered service", req.NamespacedName)


wdyt about having this and the other Debug? I'm inclined to think someone using this will apply the class to a small subset of services and could easily have a lot of these logs around.

oribon · 2022-06-08T06:22:56Z

internal/k8s/controllers/service_controller.go

 		return ctrl.Result{}, err
 	}

+	if filterByLoadbalancerClass(service, r.LoadBalancerClass) {


iiuc loadBalancerClass is immutable, can we use this in this controller as a predicate?

I tried, but I am getting failures because I am not getting passed service objects. I think it's because of when we get events for the endpoints, and at this point I am not sure it's worth having a complex predicate that switches over the object, considering there's no real performance benefit.
Also, I kind of like the symmetry between the reloader and the regular flow.

oribon · 2022-06-08T06:23:48Z

internal/k8s/controllers/service_controller.go

+func filterByLoadbalancerClass(service *v1.Service, loadBalancerClass string) bool {
+	// if it's a delete we can't make assumptions. We'll leverage the application code
+	// that will receive a delete of a non handled service.
+	if service == nil {


we can remove this if the service controller uses this func as a predicate (the service_reload_controller does not pass nils)

oribon · 2022-06-08T06:24:35Z

internal/k8s/controllers/service_controller.go

 	return &res, nil
 }
+
+func filterByLoadbalancerClass(service *v1.Service, loadBalancerClass string) bool {


nit: filterByLoadBalancerClass

oribon · 2022-06-08T06:36:30Z

controller/main.go

 		disableCertRotation = flag.Bool("disable-cert-rotation", false, "disable automatic generation and rotation of webhook TLS certificates/keys")
 		certDir             = flag.String("cert-dir", "/tmp/k8s-webhook-server/serving-certs", "The directory where certs are stored")
 		certServiceName     = flag.String("cert-service-name", "webhook-service", "The service name used to generate the TLS cert's hostname")
+		loadBalancerClass   = flag.String("lbclass", "", "load balancer class. When enabled, metallb will handle only those services with the given lb class")


nit: lbclass -> lb-class (seems more consistent with the others)
suggestion for the description: only services whose spec.loadBalancerClass matches the given lb class will be handled. (I think it's worth mentioning that this corresponds to the loadBalancerClass field)

oribon · 2022-06-08T06:43:10Z

speaker/main.go

+		logLevel          = flag.String("log-level", "info", fmt.Sprintf("log level. must be one of: [%s]", logging.Levels.String()))
+		disableEpSlices   = flag.Bool("disable-epslices", false, "Disable the usage of EndpointSlices and default to Endpoints instead of relying on the autodiscovery mechanism")
+		enablePprof       = flag.Bool("enable-pprof", false, "Enable pprof profiling")
+		loadBalancerClass = flag.String("lbclass", "", "load balancer class. When enabled, metallb will handle only those services with the given lb class")


oribon · 2022-06-08T06:47:31Z

e2etest/l2tests/loadbalancer_class.go

+			framework.ExpectNoError(err)
+
+			jig := e2eservice.NewTestJig(cs, f.Namespace.Name, "lbclass")
+			ginkgo.By("creating a service " + jig.Namespace + "/" + jig.Name + " with type=LoadBalancer")


this is already logged by the framework

oribon · 2022-06-08T06:48:36Z

e2etest/pkg/service/tweak.go

+func WithLoadbalancerClass(loadbalancerClass string) func(*corev1.Service) {
+	return func(svc *corev1.Service) {
+		svc.Spec.LoadBalancerClass = pointer.StrPtr(loadbalancerClass)
+		fmt.Println("FEDE lb class", svc.Spec.LoadBalancerClass)


oribon · 2022-06-08T06:52:06Z

website/content/installation/_index.md

+MetalLB supports [LoadBalancerClass](https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-class),
+which allows multiple LoadBalancers to co-exist. In order to set the loadbalancer class MetalLB should be listening
+for, the `--lbclass=<CLASS_NAME>` parameter must be provided to both the spekaer and the controller.
+
+The helm charts support it via the `loadbalancerClass` parameter.


nits:
multiple load balancer implementations/providers (like the k8s docs) ...
spekaer -> speaker

oribon

2 small nits (sorry), other than that looks good

oribon · 2022-06-09T12:31:42Z

controller/main.go

 		disableCertRotation = flag.Bool("disable-cert-rotation", false, "disable automatic generation and rotation of webhook TLS certificates/keys")
 		certDir             = flag.String("cert-dir", "/tmp/k8s-webhook-server/serving-certs", "The directory where certs are stored")
 		certServiceName     = flag.String("cert-service-name", "webhook-service", "The service name used to generate the TLS cert's hostname")
+		loadBalancerClass   = flag.String("lb-class", "", "load balancer class. When enabled, metallb will handle only services whose spec.loadBalancerClass matches the given lb class will be handled")


nit: remove "will be handled"

oribon · 2022-06-09T12:40:45Z

speaker/main.go

+		logLevel          = flag.String("log-level", "info", fmt.Sprintf("log level. must be one of: [%s]", logging.Levels.String()))
+		disableEpSlices   = flag.Bool("disable-epslices", false, "Disable the usage of EndpointSlices and default to Endpoints instead of relying on the autodiscovery mechanism")
+		enablePprof       = flag.Bool("enable-pprof", false, "Enable pprof profiling")
+		loadBalancerClass = flag.String("lb-class", "", "load balancer class. When enabled, metallb will handle only services whose spec.loadBalancerClass matches the given lb class will be handled")


oribon

lgtm

Loadbalancer class enable different loadbalancers to co-exist. We add a parameter to the speaker and the controller to process only the services consistent with the loadbalancer class provided to metallb.

We expose a knob to set the loadbalancer class supported by metallb. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We can't change settings on the fly, but we want to cover the fact that metallb with no settings will ignore a service with loadbalancer class set. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

We extend the docs with the newly added loadbalancer class parameter. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

The operator was using an old kubernetes version with no loadbalancer class support. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Add the ability to set a 'loadBalancerClass' value on the 'MetalLB' CRD. The 'Service.Spec.LoadBalancerClass' field reached GA in Kubernetes 1.24 and support was introduced in MetalLB in v0.13 [1]. [1] metallb/metallb#1417 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> Closes: #267

Add the ability to set a 'loadBalancerClass' value on the 'MetalLB' CRD. The 'Service.Spec.LoadBalancerClass' field reached GA in Kubernetes 1.24 and support was introduced in MetalLB in v0.13 [1]. [1] metallb/metallb#1417 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> Closes: metallb#267

fedepaol requested review from gclawes, johananl, oribon, rata and russellb as code owners June 7, 2022 12:50

fedepaol force-pushed the loadbalancerclass branch 3 times, most recently from 8a331bb to cd3800b Compare June 7, 2022 13:40

oribon reviewed Jun 8, 2022

View reviewed changes

fedepaol force-pushed the loadbalancerclass branch 6 times, most recently from 75d566c to 8f52444 Compare June 9, 2022 10:08

oribon reviewed Jun 9, 2022

View reviewed changes

fedepaol force-pushed the loadbalancerclass branch from 8f52444 to 26ab89c Compare June 9, 2022 12:44

oribon approved these changes Jun 9, 2022

View reviewed changes

fedepaol and others added 4 commits June 9, 2022 18:46

Add loadbalancer class support

7d61e9c

Loadbalancer class enable different loadbalancers to co-exist. We add a parameter to the speaker and the controller to process only the services consistent with the loadbalancer class provided to metallb.

Helm Charts: provide support to loadbalancer class

9d17b00

We expose a knob to set the loadbalancer class supported by metallb. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

E2E: check metallb ignores non supported lb class

0b655c2

We can't change settings on the fly, but we want to cover the fact that metallb with no settings will ignore a service with loadbalancer class set. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

Doc: add a note about loadbalancer class

aff8adc

We extend the docs with the newly added loadbalancer class parameter. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol force-pushed the loadbalancerclass branch from f22f69d to aff8adc Compare June 9, 2022 16:46

CI: bump the operator version

dd7f792

The operator was using an old kubernetes version with no loadbalancer class support. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>

fedepaol merged commit 9065cb4 into metallb:main Jun 10, 2022

fedepaol mentioned this pull request Jun 10, 2022

Load Balancer Class #996

Closed

fedepaol mentioned this pull request Oct 12, 2022

Provide an option to set the loadbalancer class of the deployed metallb metallb/metallb-operator#267

Closed

cprivitere mentioned this pull request Oct 12, 2022

Add MetalLB 0.13 support kubernetes-sigs/cloud-provider-equinix-metal#322

Closed

eyenx mentioned this pull request Oct 19, 2022

chore(misc/metallb): update to 0.13.x adfinis/helm-charts#834

Merged

6 tasks

NLX-SeokHwanKong mentioned this pull request Mar 27, 2023

Question about working mechanism when adding a new worker node loxilb-io/kube-loxilb#2

Closed

stephenfin mentioned this pull request Apr 28, 2023

Enable configuration of loadbalancer class metallb/metallb-operator#342

Merged

tommasopozzetti mentioned this pull request Jul 29, 2024

Add option to keep watching services without a loadbalancer class #2474

Closed

Conversation

fedepaol commented Jun 7, 2022

Uh oh!

oribon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oribon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oribon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants