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

fix: Limit HAProxy maximum concurrent connections #3115

Merged
merged 1 commit into from
Mar 5, 2023

Conversation

dlipovetsky
Copy link
Contributor

@dlipovetsky dlipovetsky commented Mar 3, 2023

If the limit is not configured, HAProxy derives it from the file descriptor limit. The higher the limit, the more memory HAProxy allocates. That limit can be so high on modern Linux distros that HAproxy allocates all available memory.

I think this change is preferable to #3028. Please see my explanation in #2954 (comment). 🙏

Why 100000 (10^5) connections? I assume that most kind clusters do not see 100000 concurrent connections. But I must admit that I have no evidence.

Increasing the limit to 1000000 (10^6) causes memory usage* to go from 18MB to 128MB (for a reproducible demonstration, see https://gist.github.com/dlipovetsky/23443bef17371a56acd8cf0579e3f6b4, which I've also linked in my issue comment). If the extra connections go unused, I think 110MB is too much additional overhead.

  • To be clear, memory usage means resident memory usage. This is memory that is allocated and cannot be used by other processes.

Fixes: #2954

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 3, 2023
@k8s-ci-robot k8s-ci-robot requested review from aojea and neolit123 March 3, 2023 18:16
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 3, 2023
@aojea
Copy link
Contributor

aojea commented Mar 3, 2023

/test pull-kind-build

@aojea
Copy link
Contributor

aojea commented Mar 3, 2023

/lgtm

/assign @BenTheElder

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2023
@dlipovetsky
Copy link
Contributor Author

@aojea Thanks for reviewing!

@dlipovetsky
Copy link
Contributor Author

(I looked at the CI failures. They failures appear unrelated to the change. They also appear for other PRs, so I think they may be flakes. Unfortunately, I don't have enough context to dive deeper.)

@aojea
Copy link
Contributor

aojea commented Mar 4, 2023

(I looked at the CI failures. They failures appear unrelated to the change. They also appear for other PRs, so I think they may be flakes. Unfortunately, I don't have enough context to dive deeper.)

CI doesn't exercise haproxy

/retest

If the limit is not configured, HAProxy derives it from the file
descriptor limit. The higher the limit, the more memory HAProxy
allocates. That limit can be so high on modern Linux distros that
HAproxy allocates all available memory.
@dlipovetsky dlipovetsky changed the title fix: Limit HAProxy maxiumum concurrent connections fix: Limit HAProxy maximum concurrent connections Mar 4, 2023
@dlipovetsky dlipovetsky force-pushed the fix-haproxy-maxconn branch from bc78a97 to 47dfc9e Compare March 4, 2023 23:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2023
@dlipovetsky
Copy link
Contributor Author

I fixed a typo in the commit subject and force-pushed.

@aojea
Copy link
Contributor

aojea commented Mar 5, 2023

/lgtm
/approve

this is on the same ballpark that the example of the official docs https://www.haproxy.com/blog/protect-servers-with-haproxy-connection-limits-and-queues/

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, dlipovetsky

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7767500 into kubernetes-sigs:main Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

limit loadbalancer max sockets
4 participants