honour routes timeout if max stream duration is set with out stream duration by ramaraochavali · Pull Request #15585 · envoyproxy/envoy

ramaraochavali · 2021-03-20T09:06:31Z

Commit Message: honour routes timeout if maxstream duration is set with out specifying max stream duration inside it.
Additional Description:
Risk Level: Low
Testing: Added
Docs Changes: N/A
Release Notes: N/A (There is already a bug fix mentioned for MaxStreamDuration in current.rst)
Platform Specific Features: N/A
Fixes #15530

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-03-20T09:07:04Z

@mattklein123 @markdroth PTAL

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

jmarantz · 2021-03-21T14:35:16Z

@adamsjob FYI

mattklein123 · 2021-03-22T16:48:57Z

Assigning over to @alyssawilk who I think reviewed the other timeout PRs.

alyssawilk

Thanks for catching and fixing this!

alyssawilk · 2021-03-22T16:53:42Z

source/common/router/router.cc

    } else {
      timeout.global_timeout_ = route.timeout();
    }
+  } else {


When we switched timeouts, we moved the instantiation of the timeout to
ConnectionManagerImpl::ActiveStream::refreshDurationTimeout()

Mind moving this logic there?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-03-24T09:27:16Z

test/common/http/conn_manager_impl_test.cc

+// Test timeout when route timeout needs to be used if max_stream_duration is not specified.
+// Regression test for https://github.com/envoyproxy/envoy/issues/15530.
+// TODO(ramaraochavali): move this test to DurationTimeout test.
+TEST_F(HttpConnectionManagerImplTest, DurationTimeoutFromRouteTimeout) {


I had to create a separate test for this because clusterInfo is repeatedly called and it keeps triggering the clearRouteCache so the timer gets fired multiple times. The same problem exists for https://github.com/envoyproxy/envoy/blob/main/test/common/http/conn_manager_impl_test.cc#L2173 where timer is repeatedly enabled. Having two such tests in the same test is not working. So I made it as separate test and TODO to look at the issue later.

ramaraochavali · 2021-03-24T09:27:54Z

@alyssawilk moved to refreshDurationTimeout. PTAL.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

markdroth · 2021-03-24T14:59:41Z

Alyssa knows this code a lot better than I do, so I'll let her review.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-03-25T11:05:29Z

@alyssawilk can you PTAL when you get chance? Also the failure seems not related to this change

alyssawilk · 2021-03-25T16:53:39Z

source/common/http/conn_manager_impl.cc

+    // If we are using new timeouts and MaxStreamDuration is not set, use the route's timeout
+    // value. This is the case when route's MaxStreamDuration is specified without inner
+    // MaxStreamDuration i.e. only grpc_timeout_header_max is set.
+    if (!route->maxStreamDuration()) {


so if grpc_timeout_header_max is set when do we use it? usingNewTimeouts would be true and we'd ignore all the code below, no?

Can you please fix and regression test?

Oh, Yeah.. Missed that. Will add a test

@alyssawilk Can you PTAL at the fix to see if this is correct? One thing I found was this needs changes to most of the test cases because now we will be initiating request timer most of the times. So I just want to ensure this is the right direction before I make changes the tests(some of which I have already done - please take a look at the changes)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

alyssawilk · 2021-03-30T16:40:07Z

source/common/http/conn_manager_impl.cc

-  auto grpc_timeout = Grpc::Common::getGrpcTimeout(*request_headers_);
  std::chrono::milliseconds timeout;
  bool disable_timer = false;
+  auto grpc_timeout = Grpc::Common::getGrpcTimeout(*request_headers_);


Offhand I recall debate around the original PR about how the new fields should be applied and it was the consensus from the gRPC folks that the difference wasn't "is this a gRPC request or not" but "is the gRPC timeout header present or not" so I think switching to this is against the intent of the folks who added this to the API.

Got it. This is good to know. I will change it accordingly

Awesome. It's super complicated and confusing, so thanks for continuing to plug away at it!

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2021-04-02T09:25:46Z

@alyssawilk Sorry for the delay. Fixed. PTAL. I have added a regression test for this and left a TODO to merge to DurationTimeout test later as I am having some issues with mock timer (even for existing tests also this problem exists).

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

source/common/http/conn_manager_impl.cc

antoniovicente

Thanks for the fix to timeout config handling.

ramaraochavali · 2021-04-08T04:09:11Z

@alyssawilk Can you PTAL when you get chance?

alyssawilk

Ah sorry, I thought you meant to address the test TODO in this PR. I'm fine with it being a follow-up!

ramaraochavali · 2021-04-21T15:31:13Z

@alyssawilk @antoniovicente one thing I realized is when timeout happens with new timeouts we call https://github.com/envoyproxy/envoy/blob/main/source/common/http/conn_manager_impl.cc#L762 and that sets http status to 408 (RequestTimeout) instead of gateway timeout (504) that router sets. Is this expected ? or do we need to change it to return 504 for timeouts? if it is expected - should we document it some where?

Adds a runtime flag for disabling the behavior introduced in envoyproxy#15585 Signed-off-by: Snow Pettersen <snowp@lyft.com>

…stream duration (envoyproxy#15585)" This reverts commit ffa1680. Signed-off-by: Snow Pettersen <snowp@lyft.com>

…stream duration (#15585)" (#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <snowp@lyft.com>

rgs1 · 2021-04-27T22:20:33Z

FYI this broke web sockets for us (by forcing them to terminate earlier), wondering if we can contribute more integration tests here to catch these sort of regressions.

…stream duration (#15585)" (#16289) This reverts commit ffa1680. Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…stream duration (envoyproxy#15585)" (envoyproxy#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

fix timeout

8228faa

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/main' into fix/timeout

92d3a18

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

jmarantz assigned markdroth Mar 21, 2021

mattklein123 assigned alyssawilk Mar 22, 2021

alyssawilk reviewed Mar 22, 2021

View reviewed changes

alyssawilk added the waiting label Mar 23, 2021

move to refreshduration

e68a3cf

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Mar 24, 2021

ramaraochavali added 2 commits March 24, 2021 14:51

remove unnecessary couts

e5d8667

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

remove unnecessary remove

fe417ff

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali commented Mar 24, 2021

View reviewed changes

ramaraochavali added 2 commits March 24, 2021 16:19

kick ci

48afbe0

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Merge remote-tracking branch 'upstream/main' into fix/timeout

43642eb

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

markdroth removed their assignment Mar 24, 2021

Merge remote-tracking branch 'upstream/main' into fix/timeout

9f7e3ca

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

alyssawilk reviewed Mar 25, 2021

View reviewed changes

alyssawilk added the waiting label Mar 25, 2021

wip fix

c1be6ae

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

repokitteh-read-only bot removed the waiting label Mar 30, 2021

alyssawilk reviewed Mar 30, 2021

View reviewed changes

fix properly and add test

9a02bf9

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali added 2 commits April 2, 2021 16:46

comment

4f894bb

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

remove unnecessary removal

30630fc

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali added 2 commits April 2, 2021 18:45

format

b7e2b4d

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

fix

d621a4b

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested a review from dio as a code owner April 2, 2021 13:18

unnecessary removal

0314715

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

alyssawilk reviewed Apr 5, 2021

View reviewed changes

source/common/http/conn_manager_impl.cc Show resolved Hide resolved

alyssawilk assigned antoniovicente Apr 5, 2021

antoniovicente approved these changes Apr 7, 2021

View reviewed changes

ramaraochavali mentioned this pull request Apr 7, 2021

Regression: istio.io egress test case gets 408 instead of 504 istio/istio#31616

Closed

alyssawilk approved these changes Apr 8, 2021

View reviewed changes

alyssawilk merged commit ffa1680 into envoyproxy:main Apr 8, 2021

ramaraochavali deleted the fix/timeout branch April 8, 2021 14:51

ericvn mentioned this pull request Apr 14, 2021

[release-1.10] Update Envoy version - Manual CP of #3284 istio/proxy#3287

Merged

TAOXUY mentioned this pull request Apr 20, 2021

b/184142099: upgrade envoy to 18.2 GoogleCloudPlatform/esp-v2#501

Merged

snowp mentioned this pull request Apr 22, 2021

Inconsistent timeout handling between max_stream_timeout and router global timeout #16129

Open

snowp pushed a commit to snowp/envoy that referenced this pull request Apr 22, 2021

http: retroactively add runtime flag for using timeout change

a758746

Adds a runtime flag for disabling the behavior introduced in envoyproxy#15585 Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp mentioned this pull request Apr 22, 2021

http: retroactively add runtime flag for timeout change #16131

Closed

snowp pushed a commit to snowp/envoy that referenced this pull request Apr 22, 2021

Revert "honour routes timeout if max stream duration is set with out …

8d7355d

…stream duration (envoyproxy#15585)" This reverts commit ffa1680. Signed-off-by: Snow Pettersen <snowp@lyft.com>

snowp mentioned this pull request Apr 22, 2021

Revert "honour routes timeout if max stream duration is set with out … #16133

Merged

snowp pushed a commit that referenced this pull request Apr 23, 2021

Revert "honour routes timeout if max stream duration is set with out …

f43a78e

…stream duration (#15585)" (#16133) This reverts commit ffa1680. Signed-off-by: Snow Pettersen <snowp@lyft.com>

dmitri-d mentioned this pull request May 3, 2021

[1.18] Revert "honour routes timeout if max stream duration is set with out … #16289

Merged

htuch pushed a commit that referenced this pull request May 4, 2021

Revert "honour routes timeout if max stream duration is set with out …

6bcb3a0

…stream duration (#15585)" (#16289) This reverts commit ffa1680. Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

sunjayBhatia mentioned this pull request May 20, 2021

Websocket Timeout projectcontour/contour#3692

Closed

ramaraochavali mentioned this pull request Nov 17, 2022

Fix setting route timeout through envoyfilter istio/istio#42035

Closed

Conversation

ramaraochavali commented Mar 20, 2021

Uh oh!

ramaraochavali commented Mar 20, 2021

Uh oh!

jmarantz commented Mar 21, 2021

Uh oh!

mattklein123 commented Mar 22, 2021

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!

ramaraochavali commented Mar 24, 2021

Uh oh!

markdroth commented Mar 24, 2021

Uh oh!

ramaraochavali commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Apr 8, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Apr 21, 2021

Uh oh!

rgs1 commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ramaraochavali commented Mar 25, 2021 •

edited

Loading

ramaraochavali commented Apr 2, 2021 •

edited

Loading