Skip to content
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.

Sign up for GitHub

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

Metallb --lb-class cmd arg to support multiple LoadBalancer implementations #10550

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Metallb --lb-class cmd arg to support multiple LoadBalancer implementations #10550

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
688cce8
metallb --lb-class cmd arg to support multiple load balancer implemen…
Seal1998 Oct 20, 2023
7a1e078
removed loadbalancer_class from metallb_config; metallb_loadbalancer_…
Seal1998 Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions roles/kubernetes-apps/metallb/templates/metallb.yaml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,6 +1724,9 @@ spec:
- args:
- --port={{ metallb_port }}
- --log-level={{ metallb_log_level }}
{% if 'loadbalancer_class' in metallb_config.keys() and metallb_config.loadbalancer_class != "" %}
- --lb-class={{ metallb_config.loadbalancer_class }}
{% endif %}
Copy link
Member

@yankay yankay Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Seal1998

Is is better to define the loadbalancer_class at the https://github.com/kubernetes-sigs/kubespray/blob/v2.23.0/roles/kubernetes-apps/metallb/defaults/main.yml#L4 instead of the metallb_config .

The same as the metallb_port :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yankay, thanks for review! I have changed the condition and role defaults accordingly. Unfortunately, to my understanding, we cannot have the --lb-class flag defined by default, similar to --port. This is because it would instruct MetalLB to listen for a specific class of LoadBalancer. However, Kubernetes does not have any "default" class to listen for, as per its documentation. The loadBalancerClass is undefined by default on all LoadBalancer services. (https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-class)

By default, .spec.loadBalancerClass is not set and a LoadBalancer type of Service uses the cloud provider's default load balancer implementation if the cluster is configured with a cloud provider using the --cloud-provider component flag.

In my opinion, we should keep --lb-class undefined until the user specifically sets the metallb_loadbalancer_class variable to something meaningful. In this way, MetalLB will act as the 'default' LoadBalancer implementation until we specifically set --lb-class

Official metallb helmchart also uses this approach

{{- if .Values.loadBalancerClass }}
        - --lb-class={{ .Values.loadBalancerClass }}
{{- end }}

Please review

env:
- name: METALLB_ML_SECRET_NAME
value: memberlist
Expand Down Expand Up @@ -1814,6 +1817,9 @@ spec:
- args:
- --port={{ metallb_port }}
- --log-level={{ metallb_log_level }}
{% if 'loadbalancer_class' in metallb_config.keys() and metallb_config.loadbalancer_class != "" %}
- --lb-class={{ metallb_config.loadbalancer_class }}
{% endif %}
env:
- name: METALLB_NODE_NAME
valueFrom:
Expand Down