balance: Configure weights from keys, not services #281
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The initial weighted balancing implementation required that the underlying service implement `HasWeight`. Practically, this doesn't work that well, since this may force middlewares to implement this trait as well. To fix this, we change the type bounds so that _keys_, not services, must implement `HasWeight`. This has a drawback, though, in that Weight, which contains a float, cannot implement `Hash` or `Eq`, which is required by the balancer. This tradeoff seems manageable, though (and is already addressed in linkerd, for instance).
What if |
|
@seanmonstar Yeah, I agree that's a good change we should pursue. I think that's sufficiently separate, though, that we can merge this and address that in a followup. |
seanmonstar
approved these changes
May 9, 2019
hawkw
pushed a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
May 9, 2019
Tower must be updated in order to pickup tower-rs/tower#281 to address linkerd/linkerd2#2804. This adopts released crates where possible.
hawkw
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 10, 2019
commit 5f89351081eff47a4ab8cd88e2e1a69a04f86541
Author: Oliver Gould <ver@buoyant.io>
Date: Thu May 9 16:39:24 2019 -0700
Upgrade tower dependencies (#249)
Tower must be updated in order to pickup tower-rs/tower#281
to address #2804.
This adopts released crates where possible.
commit 5d5eed6f8180b8db4090d995e71fdf7b0890c647
Author: Zahari Dichev <zaharidichev@gmail.com>
Date: Thu May 9 01:08:34 2019 +0300
Assert that TLS connection is refused if identity is not certified yet (#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 ##2598 and provide facility to write tests for #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 <zaharidichev@gmail.com>
commit 1b9bb3745e44c959d1d41d14fed2b2822c82b5ba
Author: Oliver Gould <ver@buoyant.io>
Date: Wed May 8 14:28:37 2019 -0700
Introduce dispatch timeouts around buffers (#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 #2779 #2795
commit caf899557c3b041190f63544da865396231b3e30
Author: Oliver Gould <ver@buoyant.io>
Date: Fri May 3 15:55:32 2019 -0700
router: Fail requests when the route is not ready (#241)
In #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 587bad101d9e5daeacb24b6733097c350a798356
Author: Eliza Weisman <eliza@buoyant.io>
Date: Fri May 3 14:18:08 2019 -0700
Remove Destination service query concurrency limit (#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 <eliza@buoyant.io>
commit cbdf45b44f7e4d852dc0497716062167ab9539fb
Author: Sean McArthur <sean@buoyant.io>
Date: Thu May 2 11:47:48 2019 -0700
Remove h2::Error requirement from metrics
Signed-off-by: Sean McArthur <sean@buoyant.io>
commit 3276949d4608dc4344b7bed3de2fc4b3080c2c6e
Author: Sean McArthur <sean@buoyant.io>
Date: Thu May 2 09:44:00 2019 -0700
delete unused proxy::http::metrics::class module
Signed-off-by: Sean McArthur <sean@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
to linkerd/linkerd2
that referenced
this pull request
May 10, 2019
commit 5f89351081eff47a4ab8cd88e2e1a69a04f86541
Author: Oliver Gould <ver@buoyant.io>
Date: Thu May 9 16:39:24 2019 -0700
Upgrade tower dependencies (#249)
Tower must be updated in order to pickup tower-rs/tower#281
to address #2804.
This adopts released crates where possible.
commit 5d5eed6f8180b8db4090d995e71fdf7b0890c647
Author: Zahari Dichev <zaharidichev@gmail.com>
Date: Thu May 9 01:08:34 2019 +0300
Assert that TLS connection is refused if identity is not certified yet (#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 ##2598 and provide facility to write tests for #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 <zaharidichev@gmail.com>
commit 1b9bb3745e44c959d1d41d14fed2b2822c82b5ba
Author: Oliver Gould <ver@buoyant.io>
Date: Wed May 8 14:28:37 2019 -0700
Introduce dispatch timeouts around buffers (#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 #2779 #2795
commit caf899557c3b041190f63544da865396231b3e30
Author: Oliver Gould <ver@buoyant.io>
Date: Fri May 3 15:55:32 2019 -0700
router: Fail requests when the route is not ready (#241)
In #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 587bad101d9e5daeacb24b6733097c350a798356
Author: Eliza Weisman <eliza@buoyant.io>
Date: Fri May 3 14:18:08 2019 -0700
Remove Destination service query concurrency limit (#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 <eliza@buoyant.io>
commit cbdf45b44f7e4d852dc0497716062167ab9539fb
Author: Sean McArthur <sean@buoyant.io>
Date: Thu May 2 11:47:48 2019 -0700
Remove h2::Error requirement from metrics
Signed-off-by: Sean McArthur <sean@buoyant.io>
commit 3276949d4608dc4344b7bed3de2fc4b3080c2c6e
Author: Sean McArthur <sean@buoyant.io>
Date: Thu May 2 09:44:00 2019 -0700
delete unused proxy::http::metrics::class module
Signed-off-by: Sean McArthur <sean@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial weighted balancing implementation required that the
underlying service implement
HasWeight.Practically, this doesn't work that well, since this may force
middlewares to implement this trait as well.
To fix this, we change the type bounds so that keys, not services,
must implement
HasWeight.This has a drawback, though, in that Weight, which contains a float,
cannot implement
HashorEq, which is required by the balancer. Thistradeoff seems manageable, though (and is already addressed in linkerd,
for instance).