Introduce dispatch timeouts around buffers by olix0r · Pull Request #246 · linkerd/linkerd2-proxy

olix0r · 2019-05-04T00:00:18Z

The proxy has several buffers, especially where it routes requests over
shared stacks. If any of these routes is unavailable, then a request may
remain buffered indefinitely. Previously, before service profiles were
introduced, there was a default response timeout that would cause
these requests to fail; but since this response timeout is now optional
(and is only applied once the request has been routed within a proxy),
then we need a new mechanism to prevent requests from getting "stuck".

This change does the following:

all proxied requests are annotated with a dispatch deadline;
each time a request is bufered, a timeout is registered.
if the timeout fires, the response exception fails, a 503 is returned,
and the request is dropped.
if the request is processed into the inner stack, the timeout is
ignored.

The dispatch timeout limits the time a request is buffered in a proxy.
This is distinct from the response timeout, as the server's response may
naturally be delayed for any number of (non-proxy-related) reasons.

The insert_target module has been generalized to insert to support
setting the DispatchDeadline extension.

The buffer module has been augmented with generic deadline-extraction
logic.

The svc module now exposes its own builder type that notably adds
a buffer_pending helper. It's helpful to pull a builder type into the
proxy to assist debugging type errors when modifying stacks.

Fixes linkerd/linkerd2#2779 linkerd/linkerd2#2795

src/proxy/buffer.rs

The proxy has several buffers, especially where it routes requests over shared stacks. If any of these routes is unavailable, then a request may remain buffered indefinitely. Previously, before service profiles were introduced, there was a default _response_ timeout that would cause these requests to fail; but since this response timeout is now optional (and is only applied once the request has been routed within a proxy), then we need a new mechanism to prevent requests from getting "stuck". This change does the following: - all proxied requests are annotated with a dispatch deadline; - each time a request is bufered, a timeout is registered. - if the timeout fires, the response exception fails, a 503 is returned, and the request is dropped. - if the request is processed into the inner stack, the timeout is ignored. The dispatch timeout limits the _time a request is buffered in a proxy_. This is distinct from the response timeout, as the server's response may naturally be delayed for any number of (non-proxy-related) reasons. The `insert_target` module has been generalized to `insert` to support setting the DispatchDeadline extension. The `buffer` module has been augmented with generic deadline-extraction logic. The `svc` module now exposes its own builder type that notably adds a `buffer_pending` helper. It's helpful to pull a builder type into the proxy to assist debugging type errors when modifying stacks.

olix0r · 2019-05-04T20:27:36Z

src/proxy/pending.rs

-pub fn layer<M>() -> impl svc::Layer<M, Service = MakePending<M>> + Copy {
-    svc::layer::mk(|inner| MakePending { inner })
+pub fn layer() -> Layer {
+    Layer(())


Added so it has a nameable type for the Builder

kleimkuhler

This is really cool. Most of my comments are questions about functionality or behavior, but I want to have answers for those before an approval. Once I understand those parts, I should be good to approve this.

Also, as a point of discussion I left a comment about the changes in src/svc.rs. I really like it right now since it keeps noise down in building inbound/outbound stacks, but I worry a little that it introduces another place where layer logic is defined. I don't have a specific point I'd like to make about that, but it seems like we'll need to strike a balance between what methods go in Builder, and what belongs in app/main.rs.

src/app/main.rs

src/proxy/buffer.rs

src/proxy/http/insert.rs

src/svc.rs

src/app/config.rs

src/app/main.rs

src/proxy/buffer.rs

hawkw · 2019-05-07T21:34:23Z

src/proxy/http/insert.rs

+}
+
+/// Wraps an HTTP `Service` so that the `T -typed value` is cloned into
+/// each request's extensions.


The way you generalized this is extremely cool :)

hawkw · 2019-05-07T22:38:54Z

src/proxy/buffer.rs

                }
            }
+        } else {
+            // Drop the timeout future so the timer doesn't need to track it.


seanmonstar · 2019-05-08T20:40:55Z

src/app/errors.rs

        warn!("server overloaded, max-in-flight reached");
        http::StatusCode::SERVICE_UNAVAILABLE
+    } else if let Some(_) = e.downcast_ref::<buffer::Aborted>() {
+        warn!("dispatch aborted");


Should this message say a little more? Such a terse message could be confusing in logs.

commit 5f89351 Author: Oliver Gould <[email protected]> Date: Thu May 9 16:39:24 2019 -0700 Upgrade tower dependencies (linkerd#249) Tower must be updated in order to pickup tower-rs/tower#281 to address linkerd/linkerd2#2804. This adopts released crates where possible. commit 5d5eed6 Author: Zahari Dichev <[email protected]> Date: Thu May 9 01:08:34 2019 +0300 Assert that TLS connection is refused if identity is not certified yet (linkerd#243) This branch adds tls capability to the support cient used in tests. In addition to that it adds two tests verifying that a TLS connection is refused in case the identity is not certified yet. This attempts to fix #linkerd/linkerd2#2598 and provide facility to write tests for linkerd/linkerd2#2676. As these are still some of my first lines of Rust code, it is advised to approach everything with a healthy dose of doubt :) Signed-off-by: Zahari Dichev <[email protected]> commit 1b9bb37 Author: Oliver Gould <[email protected]> Date: Wed May 8 14:28:37 2019 -0700 Introduce dispatch timeouts around buffers (linkerd#246) The proxy has several buffers, especially where it routes requests over shared stacks. If any of these routes is unavailable, then a request may remain buffered indefinitely. Previously, before service profiles were introduced, there was a default _response_ timeout that would cause these requests to fail; but since this response timeout is now optional (and is only applied once the request has been routed within a proxy), then we need a new mechanism to prevent requests from getting "stuck". This change does the following: - all proxied requests are annotated with a dispatch deadline; - each time a request is bufered, a timeout is registered. - if the timeout fires, the response exception fails, a 503 is returned, and the request is dropped. - if the request is processed into the inner stack, the timeout is ignored. The dispatch timeout limits the _time a request is buffered in a proxy_. This is distinct from the response timeout, as the server's response may naturally be delayed for any number of (non-proxy-related) reasons. The `insert_target` module has been generalized to `insert` to support setting the DispatchDeadline extension. The `buffer` module has been augmented with generic deadline-extraction logic. The `svc` module now exposes its own builder type that notably adds a `buffer_pending` helper. It's helpful to pull a builder type into the proxy to assist debugging type errors when modifying stacks. Fixes linkerd/linkerd2#2779 linkerd/linkerd2#2795 commit caf8995 Author: Oliver Gould <[email protected]> Date: Fri May 3 15:55:32 2019 -0700 router: Fail requests when the route is not ready (linkerd#241) In linkerd/linkerd2#2779, we plan to expire requests while they are buffered. However, the router _implicitly_ buffers requests in the executor when the inner service is not ready. This change alters the route to wrap all inner layers in a `LoadShed` so it can expect all services to `poll_ready()` immediately. commit 587bad1 Author: Eliza Weisman <[email protected]> Date: Fri May 3 14:18:08 2019 -0700 Remove Destination service query concurrency limit (linkerd#244) Currently, the proxy enforces a limit on the number of concurrent queries (i.e., the number of gRPC streams) to the Destination service. This limit was added based on information about the behaviour of the Destination service that is now known to be incorrect. This branch removes the limit on concurrent queries from the proxy's `control::destination` module. Although it should now be possible to simplify this code as a result of this change, I've refrained from doing any major refactoring in this branch --- my intention is to do this after the DNS fallback behaviour has also been removed, as together with this change, that will result in a _significant_ simplification of the module. Additionally, I've removed the tests for the concurrency limit, as they are no longer relevant. The `LINKERD2_PROXY_DESTINATION_CLIENT_CONCURRENCY_LIMIT` environment variable was also removed; this is not a breaking change as neither the CLI nor the proxy injector will currently set this env var. Signed-off-by: Eliza Weisman <[email protected]> commit cbdf45b Author: Sean McArthur <[email protected]> Date: Thu May 2 11:47:48 2019 -0700 Remove h2::Error requirement from metrics Signed-off-by: Sean McArthur <[email protected]> commit 3276949 Author: Sean McArthur <[email protected]> Date: Thu May 2 09:44:00 2019 -0700 delete unused proxy::http::metrics::class module Signed-off-by: Sean McArthur <[email protected]> Signed-off-by: Eliza Weisman <[email protected]>

hawkw reviewed May 4, 2019

View reviewed changes

src/proxy/buffer.rs Outdated Show resolved Hide resolved

src/proxy/buffer.rs Outdated Show resolved Hide resolved

olix0r force-pushed the ver/dispatch-timeout branch from e9a5950 to 11e8654 Compare May 4, 2019 20:02

touchups from review

c4bf595

olix0r changed the title ~~wip: Dispatch deadlines~~ Introduce dispatch timeouts around buffers May 4, 2019

olix0r marked this pull request as ready for review May 4, 2019 20:25

olix0r self-assigned this May 4, 2019

olix0r commented May 4, 2019

View reviewed changes

handle errors

90d18ec

olix0r mentioned this pull request May 6, 2019

proxy: Buffers can leak defunct inner services linkerd/linkerd2#2795

Closed

olix0r requested a review from seanmonstar May 6, 2019 23:37

kleimkuhler reviewed May 7, 2019

View reviewed changes

src/app/main.rs Show resolved Hide resolved

src/proxy/buffer.rs Show resolved Hide resolved

src/proxy/buffer.rs Show resolved Hide resolved

src/proxy/http/insert.rs Show resolved Hide resolved

src/svc.rs Show resolved Hide resolved

Test that the inner service is dropped appropriately

2669cf9

hawkw reviewed May 7, 2019

View reviewed changes

olix0r added 2 commits May 7, 2019 21:53

fixup test

fcae967

touchup

18c6b97

hawkw reviewed May 7, 2019

View reviewed changes

src/proxy/buffer.rs

}

}

} else {

// Drop the timeout future so the timer doesn't need to track it.

Copy link

Contributor

hawkw May 7, 2019

Choose a reason for hiding this comment

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

👍

seanmonstar approved these changes May 8, 2019

View reviewed changes

better warning message

6c69251

olix0r merged commit 1b9bb37 into master May 8, 2019

olix0r deleted the ver/dispatch-timeout branch May 8, 2019 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce dispatch timeouts around buffers#246

Introduce dispatch timeouts around buffers#246
olix0r merged 7 commits intomasterfrom
ver/dispatch-timeout

olix0r commented May 4, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

olix0r May 4, 2019

Uh oh!

kleimkuhler left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw May 7, 2019

Uh oh!

hawkw May 7, 2019

Uh oh!

seanmonstar May 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

olix0r commented May 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olix0r May 4, 2019

Choose a reason for hiding this comment

Uh oh!

kleimkuhler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw May 7, 2019

Choose a reason for hiding this comment

Uh oh!

hawkw May 7, 2019

Choose a reason for hiding this comment

Uh oh!

seanmonstar May 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olix0r commented May 4, 2019 •

edited

Loading