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

set maximum number of connections to 60000 #109

Merged
merged 8 commits into from
Feb 2, 2023

Conversation

breuerfelix
Copy link
Contributor

Signed-off-by: Felix Breuer fbreuer@pm.me

Signed-off-by: Felix Breuer <fbreuer@pm.me>
Copy link

@nightlyone nightlyone left a comment

Choose a reason for hiding this comment

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

Looks like a very good idea to manage it more explicitly.

internal/helper/yawollet.go Outdated Show resolved Hide resolved
internal/helper/yawollet.go Outdated Show resolved Hide resolved
@dergeberl
Copy link
Member

We do also limits in the createEnvoyCluster() function in internal/helper/yawollet.go:

	Thresholds: []*envoycluster.CircuitBreakers_Thresholds{
		{
			Priority:           envoycore.RoutingPriority_DEFAULT,
			MaxConnections:     &wrappers.UInt32Value{Value: 10000 * uint32(hostmetrics.GetCPUNum())},
			MaxRequests:        &wrappers.UInt32Value{Value: 8000 * uint32(hostmetrics.GetCPUNum())},
			MaxPendingRequests: &wrappers.UInt32Value{Value: 2000 * uint32(hostmetrics.GetCPUNum())},
		},
	},

This is set per upstream Port which even do not make sense. If I have 10 Ports the sum of all MaxConnections are 100.000 (with 1 CPU).
Should we setMaxConnections here always to a high value (default is 1024) and set it by CPUCount in the new Filter?

But the new setting dedicated for each listener port or is it global. Because it do also not make sense to limit each listener port to 60.000 connections.
If you run a LoadBalancer with 2 ports you still can get problems with no ports left on the LoadBalancer host.

@breuerfelix
Copy link
Contributor Author

We do also limits in the createEnvoyCluster() function in internal/helper/yawollet.go:

	Thresholds: []*envoycluster.CircuitBreakers_Thresholds{
		{
			Priority:           envoycore.RoutingPriority_DEFAULT,
			MaxConnections:     &wrappers.UInt32Value{Value: 10000 * uint32(hostmetrics.GetCPUNum())},
			MaxRequests:        &wrappers.UInt32Value{Value: 8000 * uint32(hostmetrics.GetCPUNum())},
			MaxPendingRequests: &wrappers.UInt32Value{Value: 2000 * uint32(hostmetrics.GetCPUNum())},
		},
	},

This is set per upstream Port which even do not make sense. If I have 10 Ports the sum of all MaxConnections are 100.000 (with 1 CPU). Should we setMaxConnections here always to a high value (default is 1024) and set it by CPUCount in the new Filter?

But the new setting dedicated for each listener port or is it global. Because it do also not make sense to limit each listener port to 60.000 connections. If you run a LoadBalancer with 2 ports you still can get problems with no ports left on the LoadBalancer host.

Good point, i haven't found any option which limits the overall connections for envoy across all listeners / clusters yet.
I will put this PR into draft mode since i requires another architecture.

@breuerfelix breuerfelix changed the title set maximum number of connections to 60000 [WIP] set maximum number of connections to 60000 Jan 30, 2023
Signed-off-by: Felix Breuer <fbreuer@pm.me>
@breuerfelix
Copy link
Contributor Author

breuerfelix commented Jan 31, 2023

I thought about this and i guess a solid way is to handle this on a system level. We can set the maximum number of open files for the yawol user to 60k and create another user which runs yawollet.
That way the yawollet is still able to open connections to the k8s api, even though we are close to our limits since envoy is not able to open more files.
Also, envoy does not not panic if there are no open files left.

What do you think about this @dergeberl @nightlyone ?

@breuerfelix breuerfelix changed the title [WIP] set maximum number of connections to 60000 [WIP] sjet maximum number of connections to 60000 Feb 1, 2023
@breuerfelix breuerfelix changed the title [WIP] sjet maximum number of connections to 60000 set maximum number of connections to 60000 Feb 1, 2023
Signed-off-by: Felix Breuer <fbreuer@pm.me>
Copy link
Member

@dergeberl dergeberl left a comment

Choose a reason for hiding this comment

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

LGTM

@breuerfelix breuerfelix enabled auto-merge (squash) February 2, 2023 09:02
@breuerfelix breuerfelix merged commit 36ee53a into main Feb 2, 2023
@breuerfelix breuerfelix deleted the feature/max-connections branch February 2, 2023 09:25
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 this pull request may close these issues.

4 participants