overload: scale http stream idle timeout #14155
Conversation
The connection timeout applies to the idle timeout in the common HTTP protocol options message in the HttpConnectionManager, not to the RouteAction idle timeout. Signed-off-by: Alex Konradi <akonradi@google.com>
Add a new enum value to allow scaling the timeout applied to incoming HTTP streams from downstream clients. Signed-off-by: Alex Konradi <akonradi@google.com>
|
I'm happy to add an entry in the release notes, but the current version already includes one about scaled timers, which are new in this version. Since this PR justs add an additional timer to be scaled, it didn't seem worth another entry. |
|
Are there any integration tests for scaled timer? |
|
I don't think so. I'd expect to find them in test/integration/overload_integration_test.cc |
It would be ideal to have tests that would fail if the changes to source were reverted for whatever reason. Same feedback applies to all other scaled timeouts. |
|
I think having integration tests would be very beneficial as they test a much more complete system and you can have clear signal which timeout was affected. I honestly could not figure out if the existing unit tests even covers new functionality. |
|
I've proposed an integration test in #14290 for the existing scaled timer. I'm ambivalent towards either merging the changes in here to then add a test on top of it, or waiting for the PR to land on its own first. |
|
See akonradi@530d653 for an integration test (based on merging in #14290 otherwise I'd just push it here) |
…-timer Signed-off-by: Alex Konradi <akonradi@google.com>
Add an integration test for scaling the HTTP stream timeout. Signed-off-by: Alex Konradi <akonradi@google.com>
|
|
||
| EXPECT_EQ(response->headers().getStatusValue(), "408"); | ||
| EXPECT_THAT(response->body(), HasSubstr("stream timeout")); | ||
| } |
There was a problem hiding this comment.
Can you merge master and run this under tsan a few times to make sure this new test doesn't suffer problems similar to the other one? I think we should be good since the tests have different structure but it'll be good to be safe.
…le-http-stream-timer Signed-off-by: Alex Konradi <akonradi@google.com>
|
Sorry, forgot to reply. I ran this locally under TSAN and it passed 100 times. |
|
@envoyproxy/api-shepherds, could you please take a look? |
WDYT? |
|
I think it would be good to be clear in the release notes about all of the timers that can be scaled so updating the existing one would be good? |
Signed-off-by: Alex Konradi <akonradi@google.com>
Commit Message: Scale HTTP stream idle timeout
Additional Description:
Allow scaling the timeout used for HTTP streams in response to overload conditions.
This allows Envoy to reclaim resources from long-lived but otherwise idle streams
when the proxy is overloaded.
Risk Level: low
Testing: ran tests
Docs Changes: API documentation changes to fix a bug and add the new enum
Release Notes: none
Platform Specific Features: none
Fixes #11427
/cc @antoniovicente