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.

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

Why was LoadBalancerAlgorithm removed from configuration? #3929

Closed
schancel opened this issue Mar 25, 2019 · 11 comments · Fixed by #3954
Closed

Why was LoadBalancerAlgorithm removed from configuration? #3929

schancel opened this issue Mar 25, 2019 · 11 comments · Fixed by #3954

Comments

@schancel
Copy link

In April of 2017 the following PR was merged to add support for overriding the load balancing algorithm: #673

On the 28th of November, this PR was merged, deleting it saying that it's unused:
#3478

It's not clear to me that it was in fact unused. The documentation still references it:
https://github.com/kubernetes/ingress-nginx/blob/master/docs/user-guide/nginx-configuration/configmap.md#load-balance

@ElvinEfendi @aledbf @jeffpearce

@alexkursell
Copy link
Contributor

@schancel It looks like we moved to specifying the load balancing algorithm per-ingress as an annotation, rather than globally as a configmap value. Looks like the docs never got updated to reflect that.

@schancel
Copy link
Author

Thank you

@ElvinEfendi
Copy link
Member

@schancel have you tried what's documented in there? It should work.

What was deleted is not about load-balance setting.

@alexkursell no, we should still have support for configuring load balancing algorithm globally.

@panpan0000
Copy link
Contributor

panpan0000 commented Mar 27, 2019

@ElvinEfendi

the load-balance global config-map is not working.
when changed to 'ip_hash', the nginx.conf never gets updated.

here is my test:

[root@ingress-master-1 ~]# kubectl describe  cm -n kube-system   ingress01|grep load -A2
load-balance:
----
ip_hash
[root@ingress-master-1 ~]# kubectl exec -it -n kube-system  ingress01-687b55d6fd-lcdqf grep hash nginx.conf
	types_hash_max_size             2048;
	server_names_hash_max_size      1024;
	server_names_hash_bucket_size   64;
	map_hash_bucket_size            64;
	proxy_headers_hash_max_size     512;
	proxy_headers_hash_bucket_size  64;
	variables_hash_bucket_size      128;
	variables_hash_max_size         2048;

I'm based on v0.21, but checking the code ,seems no releavant fix from 0.21 to latest.
maybe you guys or @schancel can have a try.

@ElvinEfendi
Copy link
Member

@panpan0000 are you following the note in the page?

ip_hash: to use a hash of the server for routing (note that this is available only in non-dynamic mode: --enable-dynamic-configuration=false, but alternatively you can consider using nginx.ingress.kubernetes.io/upstream-hash-by)

ip_hash is not supported in dynamic mode. And since 0.22.0 there's no non-dynamic mode, so I agree that the docs need an update regarding to ip_hash, but load-balance should work, if it does not then that's a bug.

Also in recent versions changing load-balance setting does not require Nginx reload. If you wanna inspect whether it was applied or not you can use ingress-nginx kubectl plugin and run

kubectl ingress-nginx backends --backend=<name of the backend>

to list the backend names

kubectl ingress-nginx backends --list

You can find more info about the plugin and how to install it at https://kubernetes.github.io/ingress-nginx/troubleshooting/, but it requires you use ingress-nginx version 0.23.0.

@alexkursell
Copy link
Contributor

alexkursell commented Mar 27, 2019

@schancel After talking with @ElvinEfendi in person it does look like the configmap value isn't being applied properly. I'll start working on a fix when I get the time. In the meantime, you can use the ingress annotation instead.

@panpan0000
Copy link
Contributor

@alexkursell @ElvinEfendi Thank you both for the clarification !
Most appreciated . For some reason, I'm still using 0.21 for production, but will upgrade once this fix is done and try this amazing plugin.
Thank you again!

@panpan0000
Copy link
Contributor

panpan0000 commented Apr 1, 2019

It's a pity. Since 0.21 is already dynamic-mode, so least_conn/ip_hash is not working.
only ewma left.

But when I add annotation to ingress instance as below

# kubectl describe  ingress my-l7  |grep Annotations -C 3
 Annotations:
  nginx.ingress.kubernetes.io/affinity:               cookie
  nginx.ingress.kubernetes.io/load-balance:           ewma
  nginx.ingress.kubernetes.io/proxy-connect-timeout:  100

The backend doesn't change at all. I expected the ewma weight or something will change from below status port API to reflect Lua changes, but no difference than what it was ( default round-robin)

curl localhost:18080/configuration/backends| jq '.'

@ElvinEfendi
Copy link
Member

so least_conn/ip_hash is not working.

while least_conn support is lacking, there's an alternative for ip_hash: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-nginx-upstream-hashing

he backend doesn't change at all

When you set affinity load-balance gets ignored. affinity of itself is a type of load balancing algorithm. If you remove affinity and change load-balance annotation and not see any difference in the backend configuration please create another issue with relevant details - that is not an expected behaviour.

@panpan0000
Copy link
Contributor

panpan0000 commented Apr 2, 2019

@ElvinEfendi , Yes! when nginx.ingress.kubernetes.io/affinity: cookie annotation removed,
the "load-balance": "ewma", appears in the /configuration/backends API output.
Thank you for your patient for my silly questions !

@aledbf
Copy link
Member

aledbf commented Apr 2, 2019

@alexkursell maybe ^^ is another possible validation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants