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

Add max_connections to TCP input #12691

Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Jun 26, 2019

New configuration option is added to the TCP input of Filebeat to limit the number of incoming connections. By default it is set to 0 which means, it's unlimited.

# Max number of concurrent connections, or 0 for no limit. Default: 0
#max_connections: 0

Vendored

  • golang.org/x/net/netutil

@kvch kvch added the review label Jun 26, 2019
@kvch kvch requested a review from faec June 26, 2019 10:57
@kvch kvch requested a review from a team as a code owner June 26, 2019 10:57
@michalpristas
Copy link
Contributor

this looks ok, what i'm thinking about is whether or not is this solving root cause.
issue mentioned that by introducing weird networking issue we're running out of fd's in the system. this means, that connections are not getting closed properly or are created in much higher speed than closed.
if later is the case this PR will fix that, but if we have a problem with closing connections, LimitListener will just block any new connections from being opened. not sure if this makes sense

@@ -40,6 +40,7 @@ var defaultConfig = config{
Config: tcp.Config{
Timeout: time.Minute * 5,
MaxMessageSize: 20 * humanize.MiByte,
MaxConnections: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the effects of this default value on a typical node? Are we confident that this is high enough not to artificially bottleneck a node that was previously handling a higher load? I'd be happier if this value was higher, or if the default was "no maximum"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Updated the configuration to be unlimited by default.

@kvch
Copy link
Contributor Author

kvch commented Jun 27, 2019

@michalpristas I have addressed your concerns in the relevant issue. It is indeed a workaround, proper solution, if it exists at all, needs more investigation.

@kvch kvch added the Filebeat Filebeat label Jun 28, 2019
Copy link
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

looks good, just doc fixes

# Max number of concurrent connections. Default: 10
#maxconn: 10
# Max number of concurrent connections. By default it is not limited.
#maxconn: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like "maxconn" doesn't exist in the configuration, was this supposed to be removed?

Also make the documentation more explicit (after moving it below), e.g. "Max number of concurrent connections, or 0 for no limit. Default: 0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

filebeat/input/tcp/config.go Outdated Show resolved Hide resolved
@kvch
Copy link
Contributor Author

kvch commented Jul 1, 2019

Failing tests are unrelated.

@kvch kvch merged commit fec6b95 into elastic:master Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants