router: allow retry of streaming/incomplete requests by ggreenway · Pull Request #10725 · envoyproxy/envoy

ggreenway · 2020-04-09T17:30:13Z

Signed-off-by: Greg Greenway ggreenway@apple.com

Risk Level: Medium
Testing: New integration tests
Docs Changes:
Release Notes: added
Fixes: #10211

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-04-09T18:38:54Z

@mattklein123 We discussed this change briefly a few months ago, and I finally got around to it. Can you take a quick skim and see if I missed anything obvious, or any other high-level concerns about this?

alyssawilk

Huh, I would have thought this would be way more complicated. Would definitely like you to test the heck out of this but looks good so far!

alyssawilk · 2020-04-13T18:47:17Z

source/common/router/router.cc

    buffering = false;
    active_shadow_policies_.clear();
+
+    // If we had to abandon buffering and there's no request in progress, abort the request and


Maybe a bit of commenting of how we might get here? is this just during the retry-timer interval?
Also cleanup -> clean up?

alyssawilk · 2020-04-13T19:00:48Z

source/common/router/router.cc

-  ASSERT(upstream_requests_.size() == 1);
-  upstream_requests_.front()->encodeMetadata(std::move(metadata_map_ptr));
+  if (!upstream_requests_.empty()) {
+    // TODO: buffer this if there's no request? It doesn't look like the upstream request has


router.h: TODO(soya3129): Save metadata for retry, redirect and shadowing case.
Fine to double it here since it added confusion for you

alyssawilk · 2020-04-13T19:01:51Z

source/common/router/router.cc


-    if (retry_status == RetryStatus::Yes && setupRetry()) {
+    if (retry_status == RetryStatus::Yes) {
      setupRetry();


curious, did we double count stats here?

Yes, I think so; I found this while investigating this a few months ago. See the Fixed issue in description.

alyssawilk · 2020-04-13T19:20:11Z

Also cc @AndresGuedez as I think this was on one of your milestones

ggreenway · 2020-04-13T19:47:06Z

Huh, I would have thought this would be way more complicated.

Yeah, me too. I got this far and realized it seemed to mostly-work, and said to myself "That's it!?!"

Would definitely like you to test the heck out of this but looks good so far!

Agreed, I just wanted to make sure I wasn't way off on the wrong path before writing extensive tests; I've never touched this area of the codebase before.

mattklein123

Nice that this wasn't too hard in the end. Yes this looks good to me at a high level so let's circle back when the tests are done? Thank you!

/wait

…uest Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

…uest

mattklein123 · 2020-04-15T23:47:08Z

FYI looks like legit CI failure.

/wait

Signed-off-by: Greg Greenway <ggreenway@apple.com>

…uest

ggreenway · 2020-04-16T16:52:07Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-16T16:52:16Z

Azure Pipelines successfully started running 1 pipeline(s).

alyssawilk

Yay tests! This PR really came out cleanly. I owe a pass on the unit tests, but I had one question on the overall flow I think I'd like us to sort out first.

alyssawilk · 2020-04-16T16:52:54Z

source/common/router/router.cc

-    config_.stats_.rq_retry_skipped_request_not_complete_.inc();
-    return false;
-  }
+void Filter::setupRetry() {


I wonder if we should rename, or just do this inline - it's not really setting up anything or performing the retry.

alyssawilk · 2020-04-16T16:57:02Z

test/integration/protocol_integration_test.cc

+  // Overflow the request buffer limit. Because the retry base interval is infinity, no
+  // request will be in progress. This will cause the request to be aborted and an error
+  // to be returned to the client.
+  std::string data2(2048, 'b');


I could see this for HTTP/2, where we do flow control via stream window, but only stop acking when we're at the limit, but for HTTP/1, we generally readDisable when we don't want more data. I'd think that if we're in a case where we were waiting on upstream, we'd want to readDisable (which would immediately cause data to cease) and we'd get the retry.

I thought about this, and briefly looked into it, but I think it will be much more complicated. The problematic case is when the router has an upstream request, but we don't yet know whether it will need to be retried. The upstream may require more data than we can buffer to determine if it will return a 5xx or not.

Given that, we could stop flow control in the router when we do not have an upstream request. But then it's racy whether this behavior is in effect, because there are (at least) 3 states: no upstream request at all, an upstream request on a connection that isn't established yet, and an upstream request where we're sending the request currently.

So I think all that means it is out-of-scope for this PR. But worth pursuing at some point.

alyssawilk · 2020-04-16T16:58:13Z

test/common/router/router_test.cc

-        EXPECT_EQ(host_address_, host->address());
+  expectResponseTimerCreate();
+
+  Http::TestRequestHeaderMapImpl headers{


Do you think we should add this to the base test class, since so many tests use it?

There are a lot of variations on the request headers to configure different things in these tests.

I thought about writing a base class or helper function for the several similar tests I'm adding in this PR, but it was becoming more confusing to follow what each test did, even though it reduced duplication. So I'm inclined to leave it as-is.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

alyssawilk

LGTM (I'd still mildly prefer setupRetry be renamed but it's a minor nit and I'm out tomorrow =P)

ggreenway · 2020-04-16T18:33:06Z

LGTM (I'd still mildly prefer setupRetry be renamed but it's a minor nit and I'm out tomorrow =P)

I'll push a change with it removed in a few minutes (waiting for tests to run). I also found one other minor cleanup where a comment referring to setupRetry() was outdated/wrong.

Move the debug log statement into doRetry(); it's more accurate there anyways. Minor cleanup of one function to put all stats-related code in a block together. Signed-off-by: Greg Greenway <ggreenway@apple.com>

…uest

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2020-04-16T18:46:09Z

ggreenway added 2 commits 11 minutes ago

github is showing one of the commits plus a merge; there's another commit that it isn't showing here for some reason (but does show up correctly in the "commits" tab).

…uest

ggreenway · 2020-04-17T20:32:17Z

/retest

repokitteh-read-only · 2020-04-17T20:32:21Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: pengg <pengg@google.com>

router: allow retry of streaming/incomplete requests

88f0248

Signed-off-by: Greg Greenway <ggreenway@apple.com>

mattklein123 assigned mattklein123, snowp and alyssawilk Apr 10, 2020

alyssawilk reviewed Apr 13, 2020

View reviewed changes

mattklein123 reviewed Apr 13, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 13, 2020

mattklein123 mentioned this pull request Apr 14, 2020

http: max stream duration upstream support #10531

Merged

ggreenway added 6 commits April 14, 2020 11:27

Merge remote-tracking branch 'upstream/master' into retryStreamingReq…

3ccf534

…uest Signed-off-by: Greg Greenway <ggreenway@apple.com>

comments

fd0ccc9

Signed-off-by: Greg Greenway <ggreenway@apple.com>

fix_format

e6e1e4b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

add more specific error message

a5be725

Signed-off-by: Greg Greenway <ggreenway@apple.com>

add test coverage

52ff4ad

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into retryStreamingReq…

b922b96

…uest

repokitteh-read-only bot removed the waiting label Apr 15, 2020

ggreenway marked this pull request as ready for review April 15, 2020 20:55

repokitteh-read-only bot added the waiting label Apr 15, 2020

ggreenway added 2 commits April 15, 2020 17:00

fix test

aba43ca

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into retryStreamingReq…

3f6aa36

…uest

repokitteh-read-only bot removed the waiting label Apr 16, 2020

alyssawilk reviewed Apr 16, 2020

View reviewed changes

cleanup now that control flow has changed

055fe62

Signed-off-by: Greg Greenway <ggreenway@apple.com>

alyssawilk previously approved these changes Apr 16, 2020

View reviewed changes

ggreenway added 2 commits April 16, 2020 11:34

Remove setupRetry() and inline it into the callers.

19a74ec

Move the debug log statement into doRetry(); it's more accurate there anyways. Minor cleanup of one function to put all stats-related code in a block together. Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into retryStreamingReq…

3954bc3

…uest

ggreenway dismissed alyssawilk’s stale review via 3954bc3 April 16, 2020 18:38

Note in releasenotes that a stat was removed.

c9f4e38

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into retryStreamingReq…

c4382e2

…uest

alyssawilk approved these changes Apr 20, 2020

View reviewed changes

ggreenway merged commit 37e9a52 into envoyproxy:master Apr 20, 2020

rgs1 mentioned this pull request Apr 22, 2020

Crash regression from #10362 #10901

Closed

penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020

router: allow retry of streaming/incomplete requests (envoyproxy#10725)

052ae4d

Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: pengg <pengg@google.com>

Conversation

ggreenway commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Apr 9, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Apr 13, 2020

Uh oh!

ggreenway commented Apr 13, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 15, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

azure-pipelines bot commented Apr 16, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 17, 2020

Uh oh!

repokitteh-read-only bot commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ggreenway commented Apr 9, 2020 •

edited

Loading