Remove Destination service query concurrency limit#244
Conversation
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 *not* removed in this branch, as it is also used for configuring the buffer layer around the destination client, which has not been changed. Signed-off-by: Eliza Weisman <[email protected]>
olix0r
left a comment
There was a problem hiding this comment.
yay! assuming CI is happy, so am i ;)
TIOLI: it's probably best to rename destination_concurrency_limit, e.g. to destination_buffer_capacity
There was a problem hiding this comment.
Good to see deletions! Quick note: Maybe leave a TODO comment on LINKERD2_PROXY_DESTINATION_CLIENT_CONCURRENCY_LIMIT to rename this in a further refactor? My understanding is this now more implies the destination client buffer capacity, not the concurrency limit?
Edit: Or as Oliver mentioned just rename it now :)
|
@olix0r just for clarity's sake, I'm assuming you mean renaming the field on |
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
|
I've removed the env var entirely (as suggested by @olix0r offline), as it turns out that neither the proxy injector nor the CLI ever actually set it. |
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]>
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::destinationmodule. Although it should now be possible tosimplify 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_LIMITenvironment 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]