Skip to content

Add SO_KEEPALIVE option for upstream connections. #614

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.

Sign up for GitHub

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

Closed
JonathanO wants to merge 6 commits into from
Closed

Add SO_KEEPALIVE option for upstream connections. #614

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d38f06
Add SO_KEEPALIVE option for upstream connections.
JonathanO Apr 9, 2018
780f785
Fixes code review nits.
JonathanO Apr 10, 2018
b60ba8a
Added disable flag for keepalive option.
JonathanO Apr 10, 2018
83543bd
Moved TcpKeepalive setting into new message
JonathanO Apr 11, 2018
3580cf6
Fix field number conflict and bump next free.
JonathanO Apr 11, 2018
51d7f1a
Remove support for bootstrap config of keepalive
JonathanO Apr 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ Version history
* tracing: the sampling decision is now delegated to the tracers, allowing the tracer to decide when and if
to use it. For example, if the :ref:`x-b3-sampled <config_http_conn_man_headers_x-b3-sampled>` header
is supplied with the client request, its value will override any sampling decision made by the Envoy proxy.
* sockets: added `SO_KEEPALIVE` socket option for upstream connections via :ref:`cluster manager wide

@mattklein123 mattklein123 Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: alpha order

<envoy_api_field_config.bootstrap.v2.ClusterManager.upstream_bind_config>` options.

1.6.0 (March 20, 2018)
======================
Expand Down
17 changes: 17 additions & 0 deletions envoy/api/v2/core/address.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,20 @@ message SocketAddress {
bool ipv4_compat = 6;
}

message TcpKeepalive {
// Maximum number of keepalive probes to send without response before deciding
// the connection is dead. Default is to use the OS level configuration (unless
// overridden, Linux defaults to 9.)

@mattklein123 mattklein123 Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: extra " " after ,

google.protobuf.UInt32Value keepalive_probes = 1;
// The number of seconds a connection needs to be idle before keep-alive probes
// start being sent. Default is to use the OS level configuration (unless
// overridden, Linux defaults to 7200s (ie 2 hours.)
google.protobuf.UInt32Value keepalive_time = 2;
// The number of seconds between keep-alive probes. Default is to use the OS
// level configuration (unless overridden, Linux defaults to 75s.
google.protobuf.UInt32Value keepalive_interval = 3;
}

message BindConfig {
// The address to bind to when creating a socket.
SocketAddress source_address = 1
Expand All @@ -71,6 +85,9 @@ message BindConfig {
// flag is not set (default), the socket is not modified, i.e. the option is
// neither enabled nor disabled.
google.protobuf.BoolValue freebind = 2;

// If set then set SO_KEEPALIVE on the socket to enable TCP Keepalives.
TcpKeepalive tcp_keepalive = 3;

@htuch htuch Apr 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this tied to the prebind configuration? Or can it be set at any time (e.g. post bind, after listen(), etc)?

@JonathanO JonathanO Apr 10, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My current implementation is only applicable to upstream connections, and is therefore prebind since ClientConnectionImpl only invokes PreBind.
Making it usable for inbound connections will be a bit more work, since I believe the options would need to be set on the newly accept()ed socket rather than the listen() socket, and there's no existing pattern for doing that.

@htuch htuch Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still have a uncomfortable feeling about this grouping. So far, BindConfig is all the socket address binding config, this is more of a general connection property. Can we make this a new ConnectionOptions message that belongs to Cluster?

@ggreenway ggreenway Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I'm going to eventually add keepalive settings on listeners too. So having these in a separate message makes sense to me. But we could put this message into a ConnectionOptions.

}

// Addresses specify either a logical or physical address and port, which are
Expand Down