Set specific keepalive options by default on supported platforms #59278

ywelsch · 2020-07-09T08:02:34Z

keepalives tell any intermediate devices that the connection remains alive, which helps with overzealous firewalls that are killing idle connections. keepalives are enabled by default in Elasticsearch, but use system defaults for their configuration, which often times do not have reasonable defaults (e.g. 7200s for TCP_KEEP_IDLE) in the context of distributed systems such as Elasticsearch.

This PR sets the socket-level keep_alive options for transport.tcp.{keep_idle,keep_interval} to 5 minutes on configurations that support it (>= Java 11 & (MacOS || Linux)) and where the system defaults are set to something higher than 5 minutes. This helps keep the connections alive while not interfering with system defaults or user-specified settings unless they are deemed to be set too high by providing better out-of-the-box defaults.

elasticmachine · 2020-07-09T08:02:36Z

Pinging @elastic/es-distributed (:Distributed/Network)

DaveCTurner

+1 from me, I left some small comments.

docs/reference/modules/transport.asciidoc

ywelsch · 2020-07-20T08:11:15Z

I've reworked this with a new approach (see 29b18d5), which makes it clearer that the main goal here is to keep the connections alive, while not interfering with system defaults or user-specified settings unless they are deemed to be set too high. I currently have a slight preference for this, but we can certainly do either of them. Let me know what you think @DaveCTurner

…settings

Tim-Brooks · 2020-07-22T23:55:10Z

I will take a look at this tomorrow morning.

DaveCTurner

The docs are pretty convoluted, I think we'll get questions about them. Suggested tidy-up:

diff --git a/docs/reference/modules/transport.asciidoc b/docs/reference/modules/transport.asciidoc
index e2cd92a9bdc..c33a2b7d9e7 100644
--- a/docs/reference/modules/transport.asciidoc
+++ b/docs/reference/modules/transport.asciidoc
@@ -79,36 +79,30 @@ this node connects to other nodes in the cluster.
 The following parameters can be configured on each transport profile, as in the
 example above:
 
-* `port`: The port to bind to
-* `bind_host`: The host to bind
-* `publish_host`: The host which is published in informational APIs
-* `tcp.no_delay`: Configures the `TCP_NO_DELAY` option for this socket
-* `tcp.keep_alive`: Configures the `SO_KEEPALIVE` option for this socket
+* `port`: The port to which to bind.
+* `bind_host`: The host to which to bind.
+* `publish_host`: The host which is published in informational APIs.
+* `tcp.no_delay`: Configures the `TCP_NO_DELAY` option for this socket.
+* `tcp.keep_alive`: Configures the `SO_KEEPALIVE` option for this socket, which
+   determines whether it sends TCP keepalive probes.
 * `tcp.keep_idle`: Configures the `TCP_KEEPIDLE` option for this socket, which
    determines the time in seconds that a connection must be idle before
-   starting to send TCP keepalive probes. A value of `-1` means not to set
-   this option at the socket level but to use the system default instead.
-   Only applicable on Linux and Mac, and requires Java 11 or newer.
-   Defaults to -1. On applicable configurations, this value is set to
-   300 seconds (5 minutes) if the system default or user-configured setting
-   is higher than that. Values above 300 seconds are rejected for this setting.
+   starting to send TCP keepalive probes. Defaults to `-1` which means to use
+   the smaller of 300 or the system default. May not be greater than 300. Only
+   applicable on Linux and macOS, and requires Java 11 or newer.
 * `tcp.keep_interval`: Configures the `TCP_KEEPINTVL` option for this socket,
    which determines the time in seconds between sending TCP keepalive probes.
-   A value of `-1` means not to set this option at the socket level but to
-   use the system default instead.
-   Only applicable on Linux and Mac, and requires Java 11 or newer.
-   Defaults to -1. On applicable configurations, this value is set to
-   300 seconds (5 minutes) if the system default or user-configured setting
-   is higher than that. Values above 300 seconds are rejected for this setting.
+   Defaults to `-1` which means to use the smaller of 300 or the system
+   default. May not be greater than 300. Only applicable on Linux and macOS,
+   and requires Java 11 or newer.
 * `tcp.keep_count`: Configures the `TCP_KEEPCNT` option for this socket, which
    determines the number of unacknowledged TCP keepalive probes that may be
-   sent on a connection before it is dropped. A value of `-1` means not to set
-   this option at the socket level but to use the system default instead.
-   Only applicable on Linux and Mac, and requires Java 11 or newer.
-   Defaults to -1.
-* `tcp.reuse_address`: Configures the `SO_REUSEADDR` option for this socket
-* `tcp.send_buffer_size`: Configures the send buffer size of the socket
-* `tcp.receive_buffer_size`: Configures the receive buffer size of the socket
+   sent on a connection before it is dropped. Defaults to `-1` which means to
+   use the system default. Only applicable on Linux and macOS, and requires
+   Java 11 or newer.
+* `tcp.reuse_address`: Configures the `SO_REUSEADDR` option for this socket.
+* `tcp.send_buffer_size`: Configures the send buffer size of the socket.
+* `tcp.receive_buffer_size`: Configures the receive buffer size of the socket.
 
 [[long-lived-connections]]
 ===== Long-lived idle connections

DaveCTurner · 2020-07-23T09:04:23Z

server/src/main/java/org/elasticsearch/common/network/NetworkService.java

@@ -51,9 +51,9 @@
    public static final Setting<Boolean> TCP_KEEP_ALIVE =
        Setting.boolSetting("network.tcp.keep_alive", true, Property.NodeScope);
    public static final Setting<Integer> TCP_KEEP_IDLE =
-        Setting.intSetting("network.tcp.keep_idle", -1, -1, Property.NodeScope);
+        Setting.intSetting("network.tcp.keep_idle", -1, -1, 300, Property.NodeScope);


This bit needs a breaking change tag (and docs in the backport)

ok, done so

libs/core/src/main/java/org/elasticsearch/core/internal/net/NetUtils.java

DaveCTurner

The implementation itself looks ok to me but I'll let Tim give a better-informed opinion there.

Tim-Brooks

Implementation looks good to me.

libs/core/src/main/java/org/elasticsearch/core/internal/net/NetUtils.java

modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4Transport.java

…settings

DaveCTurner

LGTM

…stic#59278) keepalives tell any intermediate devices that the connection remains alive, which helps with overzealous firewalls that are killing idle connections. keepalives are enabled by default in Elasticsearch, but use system defaults for their configuration, which often times do not have reasonable defaults (e.g. 7200s for TCP_KEEP_IDLE) in the context of distributed systems such as Elasticsearch. This PR sets the socket-level keep_alive options for network.tcp.{keep_idle,keep_interval} to 5 minutes on configurations that support it (>= Java 11 & (MacOS || Linux)) and where the system defaults are set to something higher than 5 minutes. This helps keep the connections alive while not interfering with system defaults or user-specified settings unless they are deemed to be set too high by providing better out-of-the-box defaults.

) keepalives tell any intermediate devices that the connection remains alive, which helps with overzealous firewalls that are killing idle connections. keepalives are enabled by default in Elasticsearch, but use system defaults for their configuration, which often times do not have reasonable defaults (e.g. 7200s for TCP_KEEP_IDLE) in the context of distributed systems such as Elasticsearch. This PR sets the socket-level keep_alive options for network.tcp.{keep_idle,keep_interval} to 5 minutes on configurations that support it (>= Java 11 & (MacOS || Linux)) and where the system defaults are set to something higher than 5 minutes. This helps keep the connections alive while not interfering with system defaults or user-specified settings unless they are deemed to be set too high by providing better out-of-the-box defaults.

The default interval for keepalives today is (mostly) 300s. This value was chosen to be short enough to keep connections alive even if the network drops idle connections after 600s, which is the most unreasonable idle-connection timeout we have seen in practice. This nice round number happens to coincide with the Linux kernel default for the apparently-unrelated `nf_conntrack_tcp_timeout_unacknowledged` timeout. We have observed that in rare cases if the keepalive timer expires within a few milliseconds of the conntrack unacknowledged timer then the connection may erroneously be dropped. Although the ultimate fix for this lies elsewhere, to reduce the probability of spurious connection drops in affected environments this commit extends the default keepalive interval to a more unusual 325s, avoiding the problematic coincidence. Relates elastic#59278

Set specific keepalive options by default on supported platforms

05b46d4

ywelsch added >enhancement :Distributed/Network Http and internode communication implementations v8.0.0 v7.9.0 labels Jul 9, 2020

elasticmachine added the Team:Distributed Meta label for distributed team label Jul 9, 2020

ywelsch added 2 commits July 9, 2020 10:23

checkstyle

e3aa4c0

Oops

05c8606

ywelsch added the team-discuss label Jul 14, 2020

DaveCTurner reviewed Jul 15, 2020

View reviewed changes

docs/reference/modules/transport.asciidoc Outdated Show resolved Hide resolved

docs/reference/modules/transport.asciidoc Outdated Show resolved Hide resolved

docs/reference/modules/transport.asciidoc Outdated Show resolved Hide resolved

ywelsch added 3 commits July 16, 2020 11:37

revise docs

5239fe9

align code

b1b3bbc

New approach

29b18d5

ywelsch added 3 commits July 20, 2020 11:06

fix tests

33aa565

Merge remote-tracking branch 'elastic/master' into default-keepalive-…

75c7251

…settings

oh noes, an unused import

625ffc0

ywelsch requested a review from DaveCTurner July 21, 2020 11:59

ywelsch added 2 commits July 22, 2020 16:41

reject values above 5 minutes

d840b2d

Merge remote-tracking branch 'elastic/master' into default-keepalive-…

51b46d9

…settings

ywelsch added v7.10.0 and removed v7.9.0 labels Jul 22, 2020

ywelsch requested a review from Tim-Brooks July 22, 2020 16:07

DaveCTurner reviewed Jul 23, 2020

View reviewed changes

Tim-Brooks approved these changes Jul 23, 2020

View reviewed changes

rework docs

785e35f

ywelsch removed the team-discuss label Jul 27, 2020

ywelsch added the >breaking label Jul 27, 2020

ywelsch added 6 commits July 27, 2020 11:42

ClosedChannelException subclasses IOException

c5c8faf

shorter assertion

c9a582c

Naming

1dbe239

The given socket options won't return null

380abca

don't be so defensive

57f4916

Merge remote-tracking branch 'elastic/master' into default-keepalive-…

0e70945

…settings

ywelsch requested a review from DaveCTurner July 27, 2020 09:56

DaveCTurner approved these changes Jul 27, 2020

View reviewed changes

ywelsch merged commit 2078509 into elastic:master Jul 27, 2020

DaveCTurner mentioned this pull request Jul 27, 2020

Properly document keepalive and other tcp options #60216

Merged

DaveCTurner mentioned this pull request Jul 28, 2020

Missing documentation for configuring TCP keepalives #45081

Closed

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

original-brownbear mentioned this pull request May 11, 2021

possible request handler memory leak in TransportService #72852

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

fcofdez mentioned this pull request Jul 28, 2022

Keep alive settings are not applied by default #88897

Closed

henningandersen mentioned this pull request Jul 29, 2022

Ensure that the extended socket options TCP_KEEPXXX are available #88935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set specific keepalive options by default on supported platforms #59278

Set specific keepalive options by default on supported platforms #59278

ywelsch commented Jul 9, 2020 •

edited

Loading

elasticmachine commented Jul 9, 2020

DaveCTurner left a comment

ywelsch commented Jul 20, 2020 •

edited

Loading

Tim-Brooks commented Jul 22, 2020

DaveCTurner left a comment

DaveCTurner Jul 23, 2020

ywelsch Jul 27, 2020

DaveCTurner left a comment

Tim-Brooks left a comment

DaveCTurner left a comment

Set specific keepalive options by default on supported platforms #59278

Set specific keepalive options by default on supported platforms #59278

Conversation

ywelsch commented Jul 9, 2020 • edited Loading

elasticmachine commented Jul 9, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

ywelsch commented Jul 20, 2020 • edited Loading

Tim-Brooks commented Jul 22, 2020

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner Jul 23, 2020

Choose a reason for hiding this comment

ywelsch Jul 27, 2020

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Tim-Brooks left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

ywelsch commented Jul 9, 2020 •

edited

Loading

ywelsch commented Jul 20, 2020 •

edited

Loading