http2: add optional connection ping/keepalive #13152

ggreenway · 2020-09-17T18:44:47Z

Signed-off-by: Greg Greenway [email protected]

Commit Message:
Additional Description:
Risk Level: low (disabled by default)
Testing: Added new tests
Docs Changes: documented
Release Notes: added
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Greg Greenway <[email protected]>

repokitteh-read-only · 2020-09-17T18:44:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13152 was opened by ggreenway.

see: more, trace.

ggreenway · 2020-09-17T18:46:37Z

Should this be noted in the XDS docs somewhere as "we recommend setting this"? I didn't see a really good place to add it.

Any idea how to make the upstream integration test work? I expected what I wrote to work, but the test waits forever without the connection getting closed.

Signed-off-by: Greg Greenway <[email protected]>

mattklein123

Nice, thanks for working on this. Just starting with a few API comments before diving into the code.

/wait

mattklein123 · 2020-09-17T22:43:17Z

api/envoy/config/core/v3/protocol.proto

+  // If configured, this value must be less than :ref:`connection_keepalive_interval
+  // <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.connection_keepalive_interval>`.


Why must this be less? Can't it work similar to health checking where the interval and the timeout are disjoint? It seems like that might be what we want?

I did this for ease of implementation (so there can't be two outstanding pings at the same time). I figured it would easy to relax this at some point in the future.

I'm not sure how you can guarantee that no matter what you do, due to event loop delay, etc. It seems like it would be better if it worked like health checking where there is an outstanding ping with a timeout, and then an interval.

Yeah, I agree that doing it like the health checker will be better (and more consistent). I'll go that way.

mattklein123 · 2020-09-17T22:44:09Z

api/envoy/config/core/v3/protocol.proto

+  // Send HTTP/2 PING frames at this period, in order to test that the connection is still alive.
+  // If not specified, no keepalive PING frames will be sent.


Should we have any built-in jitter here, either just implicit or defined as a param?

Good point; we can probably borrow some of the stuff from health checks.

mattklein123 · 2020-09-17T22:46:45Z

api/envoy/config/core/v3/protocol.proto

@@ -345,6 +345,22 @@ message Http2ProtocolOptions {
  // <https://www.iana.org/assignments/http2-parameters/http2-parameters.xhtml#settings>`_ for
  // standardized identifiers.
  repeated SettingsParameter custom_settings_parameters = 13;
+
+  // Send HTTP/2 PING frames at this period, in order to test that the connection is still alive.


Thoughts on making this a dedicated message, with the sub-fields all required?

I don't care either way

I think I would recommend that if you don't mind either way.

Yeah, +1 here.

yanavlasov · 2020-09-19T00:01:51Z

source/common/http/http2/codec_impl_legacy.cc

+  ENVOY_CONN_LOG(trace, "Sending keepalive PING {}", connection_, ms_since_epoch);
+
+  // The last parameter is an opaque 8-byte buffer, so this cast is safe.
+  int rc = nghttp2_submit_ping(session_, 0 /*flags*/, reinterpret_cast<uint8_t*>(&ms_since_epoch));


Looks like processing of the PING ACK is missing in the legacy codec.

yanavlasov · 2020-09-19T00:23:56Z

It would also be good to add codec_impl_test.cc tests for the PINGs. There is a bit of an overlap with the integration tests, but having these lower level tests would be good too.

htuch · 2020-09-21T15:00:08Z

source/common/http/http2/codec_impl.cc

+
+  // The last parameter is an opaque 8-byte buffer, so this cast is safe.
+  int rc = nghttp2_submit_ping(session_, 0 /*flags*/, reinterpret_cast<uint8_t*>(&ms_since_epoch));
+  ASSERT(rc == 0);


How confident are you that this is always true?

I see, this is only NOMEM. In that case, I think a RELEASE_ASSERT is warranted.

I copied the pattern from elsewhere in the file; it's pervasive. I personally have no confidence that it's true.

* use same timer approach as for health checks Signed-off-by: Greg Greenway <[email protected]>

ggreenway · 2020-09-21T15:24:27Z

@mattklein123 I added jitter, but it blew up the size of the PR, due to needing to pass RandomGenerator to the codec now. But I think it's worth it; jitter is rather important.

Signed-off-by: Greg Greenway <[email protected]>

mattklein123 · 2020-09-22T17:53:55Z

Can you merge main and check CI and I can take a look?

/wait

ggreenway · 2020-09-22T17:57:47Z

Can you merge main and check CI and I can take a look?

/wait

I'm still trying to figure out a unit test (but I keep getting distracted by other things).

ggreenway · 2020-09-22T19:42:57Z

It would also be good to add codec_impl_test.cc tests for the PINGs. There is a bit of an overlap with the integration tests, but having these lower level tests would be good too.

@yanavlasov I tried, but I can't figure out what/how the tests work. I think I can test the jitter, and that the initial timer is set, and what happens when the timeout timer fires, but I don't have any idea how to verify that a PING was either sent, or how to send/not-send a response PING.

Signed-off-by: Greg Greenway <[email protected]>

mattklein123 · 2020-09-22T22:48:10Z

@yanavlasov I tried, but I can't figure out what/how the tests work. I think I can test the jitter, and that the initial timer is set, and what happens when the timeout timer fires, but I don't have any idea how to verify that a PING was either sent, or how to send/not-send a response PING.

The codec tests use a fake codec client for the other half of the connection. It should respond to pings just like a real connection does. Thus, if you don't hold any data, it should respond, and you for example held the the response data, it should not send any ping response. Whether this is worth it or not I will defer to both of you.

mattklein123

API LGTM modulo small comments. I will take a look at the code once you sort out the CI issues.

/wait

mattklein123 · 2020-09-22T22:49:53Z

api/envoy/config/core/v3/protocol.proto

+    gt {nanos: 1000000}
+  }];
+
+  // An optional jitter amount as a percentage of interval_ms. If specified,


s/interval_ms/interval here and below

api/envoy/config/core/v3/protocol.proto

mattklein123 · 2020-09-22T22:50:51Z

api/envoy/config/core/v3/protocol.proto

@@ -345,6 +365,9 @@ message Http2ProtocolOptions {
  // <https://www.iana.org/assignments/http2-parameters/http2-parameters.xhtml#settings>`_ for
  // standardized identifiers.
  repeated SettingsParameter custom_settings_parameters = 13;
+
+  // Send HTTP/2 PING frames to verify that the connection is still healthy.


nit: can you clarify what happens if the keep alive fails?

ggreenway · 2020-09-24T21:57:29Z

Got another clang_tidy error about not being able to find a header on the last run. Hoping that merging master will make it pass. No idea why it failed, since no related files changed.

ggreenway · 2020-09-24T22:41:36Z

/retest

repokitteh-read-only · 2020-09-24T22:41:39Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux-x64 compile_time_options), please wait.

🐱

Caused by: a #13152 (comment) was created by @ggreenway.

see: more, trace.

mattklein123 · 2020-09-24T23:09:08Z

If you merge main it should fix gcc. I think your other issues are an AZP outage.

/wait

htuch

/lgtm api

docs/root/version_history/current.rst

Signed-off-by: Greg Greenway <[email protected]>

mattklein123

LGTM with small comments.

/wait

mattklein123 · 2020-09-25T17:12:29Z

api/xds_protocol.rst

+            interval: 1s
+            timeout: 1s


These seem pretty aggressive. I would go either a longer interval and longer timeout? Same elsewhere.

It's just an example; people should come up with their own values. But if you want it changed, what values should we use?

People just copy the docs... I would probably use 30s interval with 5s timeout?

mattklein123 · 2020-09-25T17:12:57Z

api/xds_protocol.rst

-          # configure a TCP keep-alive to detect and reconnect to the admin
-          # server in the event of a TCP socket disconnection
-          tcp_keepalive:
-            ...


Do we want to also demonstrate TCP keep alive and describe both options? I think TCP keep alive would work fine in many cases and is lower overhead.

Yeah, that makes sense. I'll update.

Signed-off-by: Greg Greenway <[email protected]>

ggreenway · 2020-09-25T20:50:07Z

I don't understand why, but //test/config_test:example_configs_test starting failing after I touched the examples, because they use deprecated field watchdog. That field was deprecated a week ago; why did it start failing now? Was bazel caching the test result based only on the content of the example .yaml, and not re-running when Envoy changed? @htuch

mattklein123 · 2020-09-25T21:42:42Z

I don't understand why, but //test/config_test:example_configs_test starting failing after I touched the examples, because they use deprecated field watchdog. That field was deprecated a week ago; why did it start failing now? Was bazel caching the test result based only on the content of the example .yaml, and not re-running when Envoy changed? @htuch

Hmm interesting. cc @phlax who has been working on this.

mattklein123 · 2020-09-26T15:15:08Z

LGTM. Can you merge main one more time which should fix ASAN? I don't understand the example config issue but we can look at that separately. I think it must have to do with the fact that watchdog is a bootstrap field, but I don't understand why the failure would not be deterministic.

/wait

addressed

http2: add optional connection ping/keepalive

b426501

Signed-off-by: Greg Greenway <[email protected]>

repokitteh-read-only bot added the api label Sep 17, 2020

ggreenway assigned mattklein123 and htuch Sep 17, 2020

ggreenway added 3 commits September 17, 2020 11:51

fix_format release note

17be32e

Signed-off-by: Greg Greenway <[email protected]>

Merge remote-tracking branch 'upstream/master' into h2-keepalive

b880098

fix_format release note after merge broke it again :(

7037b1d

Signed-off-by: Greg Greenway <[email protected]>

mattklein123 requested changes Sep 17, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 17, 2020

yanavlasov requested changes Sep 19, 2020

View reviewed changes

yanavlasov self-assigned this Sep 19, 2020

htuch reviewed Sep 21, 2020

View reviewed changes

* add jitter to timing

505eed7

* use same timer approach as for health checks Signed-off-by: Greg Greenway <[email protected]>

ggreenway requested a review from alyssawilk as a code owner September 21, 2020 15:21

repokitteh-read-only bot removed the waiting label Sep 21, 2020

ggreenway added 2 commits September 21, 2020 09:04

Merge remote-tracking branch 'upstream/master' into h2-keepalive

239878e

Signed-off-by: Greg Greenway <[email protected]>

fix doc links

4063204

Signed-off-by: Greg Greenway <[email protected]>

repokitteh-read-only bot added the waiting label Sep 22, 2020

repokitteh-read-only bot removed the waiting label Sep 22, 2020

unit tests

a26b20a

Signed-off-by: Greg Greenway <[email protected]>

mattklein123 requested changes Sep 22, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 22, 2020

ggreenway dismissed mattklein123’s stale review via 675d3f5 September 24, 2020 20:30

repokitteh-read-only bot added the api label Sep 24, 2020

Merge remote-tracking branch 'upstream/master' into h2-keepalive

8e172b2

repokitteh-read-only bot added the waiting label Sep 24, 2020

Merge remote-tracking branch 'upstream/master' into h2-keepalive

179a671

repokitteh-read-only bot removed the waiting label Sep 24, 2020

htuch reviewed Sep 25, 2020

View reviewed changes

docs/root/version_history/current.rst Show resolved Hide resolved

repokitteh-read-only bot removed the api label Sep 25, 2020

docs: use this in xds

3596d4a

Signed-off-by: Greg Greenway <[email protected]>

mattklein123 requested changes Sep 25, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 25, 2020

show both types of keepalive in example

c6c5c6b

Signed-off-by: Greg Greenway <[email protected]>

repokitteh-read-only bot removed the waiting label Sep 25, 2020

ggreenway added 2 commits September 25, 2020 11:32

change time values used in examples

cb23e98

Signed-off-by: Greg Greenway <[email protected]>

fix example-configs test by not using deprecated field watchdog

d876c44

Signed-off-by: Greg Greenway <[email protected]>

repokitteh-read-only bot added the waiting label Sep 26, 2020

Merge remote-tracking branch 'upstream/master' into h2-keepalive

25e667d

repokitteh-read-only bot removed the waiting label Sep 28, 2020

mattklein123 approved these changes Sep 28, 2020

View reviewed changes

ggreenway merged commit b63855d into envoyproxy:master Sep 28, 2020

skonto mentioned this pull request Apr 17, 2023

Fix keepalive interval knative-extensions/net-kourier#1025

Merged

		// If configured, this value must be less than :ref:`connection_keepalive_interval
		// <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.connection_keepalive_interval>`.

		// Send HTTP/2 PING frames at this period, in order to test that the connection is still alive.
		// If not specified, no keepalive PING frames will be sent.

http2: add optional connection ping/keepalive #13152

http2: add optional connection ping/keepalive #13152

Conversation

ggreenway commented Sep 17, 2020

repokitteh-read-only bot commented Sep 17, 2020

ggreenway commented Sep 17, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Sep 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Sep 21, 2020

mattklein123 commented Sep 22, 2020

ggreenway commented Sep 22, 2020

ggreenway commented Sep 22, 2020

mattklein123 commented Sep 22, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Sep 24, 2020

ggreenway commented Sep 24, 2020

repokitteh-read-only bot commented Sep 24, 2020

mattklein123 commented Sep 24, 2020

htuch left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway commented Sep 25, 2020

mattklein123 commented Sep 25, 2020

mattklein123 commented Sep 26, 2020