Route requests to original dsts when no endpoints exist in the lb #240

hawkw · 2019-04-30T23:45:17Z

Currently, when no endpoints exist in the load balancer for a
destination, we fail the request. This is because we expect endpoints to
be discovered by both destination service queries and DNS lookups, so
if there are no endpoints for a destination, it is assumed to not exist.

In linkerd/linkerd2#2661, we intend to remove the DNS lookup from the
proxy and instead fall back to routing requests for which no endpoints
exist in the destination service to their SO_ORIGINAL_DST IP address.
This means that the current approach of failing requests when the load
balancer has no endpoints will no longer work.

This branch introduces a new balance::fallback::layer, which marries a
load balancer with a router. When the load balancer has no endpoints, we
fall back to the router, rather than failing the request at the
balancer. We can use this router to recognize endpoints based on the
request's original destination address.

A new trait, HasEndpointStatus, is added in order to allow the
discovery lookup to communicate the "no endpoints" state to the
balancer. In addition, we add a new Update::NoEndpoints variant to
proxy::resolve::Update, so that when the control plane sends a no
endpoints update, we switch from the balancer to the fallback router
immediately, rather than waiting for all the endpoints to be
individually removed.

A future commit will remove the DNS lookups from the discovery module.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Sean McArthur <sean@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2019-04-30T23:46:09Z

I've marked this PR as a draft as I'd still like to add integration tests for this change if possible. However, I thought @olix0r and @kleimkuhler would like to have a look at it sooner rather than later.

hawkw · 2019-04-30T23:48:12Z

src/app/main.rs

@@ -663,7 +692,7 @@ where
            // implementations can use the route-specific configuration.
            let dst_route_stack = svc::builder()
                .layer(buffer::layer(max_in_flight))
-                .layer(pending::layer())
+                .layer(pending::layer::<_, dst::Route, _>())


This type annotation is necessary as a result of changing the pending::layer function to bound the target type.

@kleimkuhler

for @kleimkuhler :)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r · 2019-05-07T23:27:44Z

src/proxy/http/balance.rs

+        self,
+        max_in_flight: usize,
+        recognize: Rec,
+    ) -> fallback::Layer<Rec, Self, A>


just thinking out loud -- I wonder if this could just take another layer type, so the router/buffering/etc logic need not be fixed in this file (and could potentially support arbitrary other layers on the fallback route)

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r · 2019-05-07T23:33:46Z

src/proxy/http/balance.rs

+
+impl ::std::error::Error for NoEndpoints {}
+
+pub mod fallback {


this module doubles the size of this file... can it be split into its own file? maybe balance needs to become a directory...

yeah, I wasn't sure --- will split it out!

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r · 2019-05-08T00:31:58Z

src/proxy/http/balance/fallback.rs

+            if let Some(ref mut router) = self.router {
+                return svc::Service::call(router, req);
+            } else {
+                self.create();


These seems problematic, as we'll loop into call without having invoked poll_ready. In fact, this seems unreachable.

the intention was to guard against a situation where the no endpoints is received between poll_ready and call. I'm not sure if that's actually a concern. but i think you're right that this is incorrect --- the state transition should only ever occur in poll_ready.

in the current situation I don't think this is a problem since routers are always ready? but if that changes this would be a problem

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Currently, when no endpoints exist in the load balancer for a destination, we fail the request. This is because we expect endpoints to be discovered by both destination service queries _and_ DNS lookups, so if there are no endpoints for a destination, it is assumed to not exist. In linkerd/linkerd2#2661, we intend to remove the DNS lookup from the proxy and instead fall back to routing requests for which no endpoints exist in the destination service to their SO_ORIGINAL_DST IP address. This means that the current approach of failing requests when the load balancer has no endpoints will no longer work. This branch introduces a generic `fallback` layer, which composes a primary and secondary service builder into a new layer. The primary service can fail requests with an error type that propages the original request, allowing the fallback middleware to call the fallback service with the same request. Other errors returned by the primary service are still propagated upstream. In contrast to the approach used in #240, this fallback middleware is generic and not tied directly to a load balancer or a router, and can be used for other purposes in the future. It relies on the router cache eviction added in #247 to drain the router when it is not being used, rather than proactively destroying the router when endpoints are available for the lb, and re-creating it when they exist again. A new trait, `HasEndpointStatus`, is added in order to allow the discovery lookup to communicate the "no endpoints" state to the balancer. In addition, we add a new `Update::NoEndpoints` variant to `proxy::resolve::Update`, so that when the control plane sends a no endpoints update, we switch from the balancer to the no endpoints state _immediately_, rather than waiting for all the endpoints to be individually removed. When the balancer has no endpoints, it fails all requests with a fallback error, so that the fallback middleware A subsequent PR (#248) will remove the DNS lookups from the discovery module. Closes #240. Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@olix0r

commit b27dfb2 Author: Takanori Ishibashi <takanori.1112@gmail.com> Date: Wed May 15 05:58:42 2019 +0900 updaes->updates (linkerd#250) Signed-off-by: Takanori Ishibashi <takanori.1112@gmail.com> commit 16441c2 Author: Eliza Weisman <eliza@buoyant.io> Date: Tue May 14 14:40:03 2019 -0700 Pass router::Config directly to router::Layer (linkerd#253) Currently, router `layer`s are constructed with a single argument, a type implementing `Recognize`. Then, the entire router stack is built with a `router::Config`. However, in linkerd#248, it became necessary to provide the config up front when constructing the `router::layer`, as the layer is used in a fallback layer. Rather than providing a separate type for a preconfigured layer, @olix0r suggested we simply change all router layers to accept the `Config` when they're constructed (see linkerd#248 (comment)). This branch changes `router::Layer` to accept the config up front. The `router::Stack` types `make` function now requires no arguments, and the implementation of `Service` for `Stack` can be called with any `T` (as the target is now ignored). Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit b70c68d Author: Eliza Weisman <eliza@buoyant.io> Date: Wed May 15 13:14:04 2019 -0700 Load balancers fall back to ORIG_DST when no endpoints exist (linkerd#248) Currently, when no endpoints exist in the load balancer for a destination, we fail the request. This is because we expect endpoints to be discovered by both destination service queries _and_ DNS lookups, so if there are no endpoints for a destination, it is assumed to not exist. In linkerd/linkerd2#2661, we intend to remove the DNS lookup from the proxy and instead fall back to routing requests for which no endpoints exist in the destination service to their SO_ORIGINAL_DST IP address. This means that the current approach of failing requests when the load balancer has no endpoints will no longer work. This branch introduces a generic `fallback` layer, which composes a primary and secondary service builder into a new layer. The primary service can fail requests with an error type that propages the original request, allowing the fallback middleware to call the fallback service with the same request. Other errors returned by the primary service are still propagated upstream. In contrast to the approach used in linkerd#240, this fallback middleware is generic and not tied directly to a load balancer or a router, and can be used for other purposes in the future. It relies on the router cache eviction added in linkerd#247 to drain the router when it is not being used, rather than proactively destroying the router when endpoints are available for the lb, and re-creating it when they exist again. A new trait, `HasEndpointStatus`, is added in order to allow the discovery lookup to communicate the "no endpoints" state to the balancer. In addition, we add a new `Update::NoEndpoints` variant to `proxy::resolve::Update`, so that when the control plane sends a no endpoints update, we switch from the balancer to the no endpoints state _immediately_, rather than waiting for all the endpoints to be individually removed. When the balancer has no endpoints, it fails all requests with a fallback error, so that the fallback middleware A subsequent PR (linkerd#248) will remove the DNS lookups from the discovery module. Closes linkerd#240. Signed-off-by: Eliza Weisman <eliza@buoyant.io> commit 6525b06 Author: Zahari Dichev <zaharidichev@gmail.com> Date: Wed May 15 23:35:09 2019 +0300 Allow disabling tap by setting an env var (linkerd#252) This PR fixes linkerd/linkerd2#2811. Now if `LINKERD2_PROXY_TAP_DISABLED` is set, the tap is not served at all. The approach taken is that the `ProxyParts` is changed so the `control_listener` is now an `Option` that will be None if tap is disabled as this control_listener seems to be exclusively used to serve the tap. Feel free to suggest a better approach. Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 91f32db Author: Zahari Dichev <zaharidichev@gmail.com> Date: Thu May 16 00:45:23 2019 +0300 Assert that outbound TLS works before identity is certified (linkerd#251) This commit introduces TLS capabilities to the support server as well as tests to ensure that outbound TLS works even when there is no verified certificate for the proxy yet. Fixes linkerd/linkerd2#2599 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> commit 45aadc6 Author: Sean McArthur <sean@buoyant.io> Date: Wed May 15 14:25:39 2019 -0700 Update h2 to v0.1.19 Includes a couple HPACK fixes Signed-off-by: Sean McArthur <sean@buoyant.io> commit 3e0e00c Author: Oliver Gould <ver@buoyant.io> Date: Thu May 16 08:11:06 2019 -0700 Update mio to 0.6.17 (linkerd#257) To pick up tokio-rs/mio#939

This branch updates `tokio-util` to v0.7.0. The dependabot-generated PR for this update doesn't work, because this is a breaking API change. The only update that was necessary here was changing the use of `tokio_util::sync::PollSender` in `linkerd-proxy-discover` to use the updated method names. Closes #240

This branch updates `tokio-util` to v0.7.0. The dependabot-generated PR for this update doesn't work, because this is a breaking API change. The only update that was necessary here was changing the use of `tokio_util::sync::PollSender` in `linkerd-proxy-discover` to use the updated method names. Closes #240 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

This branch updates `tokio-util` to v0.7.0. The dependabot-generated PR for this update doesn't work, because this is a breaking API change. The only update that was necessary here was changing the use of `tokio_util::sync::PollSender` in `linkerd-proxy-discover` to use the updated method names. Closes #240 Signed-off-by: Eliza Weisman <eliza@buoyant.io> (cherry picked from commit 7f17318) Signed-off-by: Oliver Gould <ver@buoyant.io>

seanmonstar and others added 18 commits April 22, 2019 15:53

Replace linkerd2-stack with tower-layer

aeb967f

Signed-off-by: Sean McArthur <sean@buoyant.io>

Add "no endpoints" state to resolve::Discover

5576d78

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Hack destination_set to send NoEndpoints

d20b5c2

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

wire up endpoint status *without* breaking stuff

933581d

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

don't get discover out of stack

3d358d0

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

add no-endpoints error

f731f56

wip

2a24588

start adding tests

facf313

made the tests compile but the proxy still doesn't

ef838d6

Merge branch 'master' into liza/balance-rt-good

0c152a4

thanks @seanmonstar

dad8ad9

fix payload err types

11f8578

make everything compile-y

6c14ac1

fix fallback svc being ready when the LB isn't

c87b088

fix warnings in tests

7069c1d

clean up balancer construction a bit

f7abd4e

Merge branch 'master' into liza/balance-rt-good

741debd

turn warnings back on

4f1abf2

hawkw requested review from seanmonstar, olix0r and kleimkuhler April 30, 2019 23:45

hawkw self-assigned this Apr 30, 2019

hawkw commented Apr 30, 2019

View reviewed changes

hawkw added 2 commits April 30, 2019 17:12

rm vestigial where bounds

9768683

for @kleimkuhler :)

simplify fallback router's recognize closure

02628b3

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r mentioned this pull request May 1, 2019

Remove buffers from endpoint stacks #239

Merged

hawkw added 3 commits May 1, 2019 14:46

create balancer and/or router as needed

6a34871

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

keep balancer alive always to drive discovery

646728d

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Merge branch 'master' into liza/balance-rt-good

d00858f

hawkw added 11 commits May 2, 2019 15:21

also fix syntax

b34cd1d

always send NoEndpoints

5d44601

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fix bad merge

3d500f1

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fix bad merge

378f9d7

add logging to fallback

e24f5fe

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

fix fallback router never being ready

4c75002

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

don't assume no endpoints

3c2fb23

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Merge branch 'master' into liza/balance-rt-good

8130e8d

Remove test asserting no eps times out

fb99e9f

Replace test with one expecting the right behavior

ca64d98

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Merge branch 'master' into liza/balance-rt-good

8cc4d78

hawkw mentioned this pull request May 7, 2019

Simplify destination discovery fallback #245

Closed

hawkw requested a review from olix0r May 7, 2019 20:35

hawkw mentioned this pull request May 7, 2019

Introduce dispatch timeouts around buffers #246

Merged

olix0r reviewed May 7, 2019

View reviewed changes

remove target type parameter from pending::layer

ea1b74a

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r reviewed May 7, 2019

View reviewed changes

put fallback module in its own file

433505b

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

olix0r reviewed May 8, 2019

View reviewed changes

hawkw added 2 commits May 8, 2019 10:26

only update fallback state in poll_ready

599201a

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

compose fallback using ServiceBuilder

b565ae6

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw mentioned this pull request May 8, 2019

Load balancers fall back to ORIG_DST when no endpoints exist #248

Merged

hawkw closed this May 9, 2019

olix0r deleted the liza/balance-rt-good branch August 17, 2019 01:35

hawkw mentioned this pull request Feb 11, 2022

build(deps): update tokio-util to v0.7 #1490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route requests to original dsts when no endpoints exist in the lb #240

Route requests to original dsts when no endpoints exist in the lb #240

hawkw commented Apr 30, 2019

hawkw commented Apr 30, 2019

hawkw Apr 30, 2019

olix0r May 7, 2019

olix0r May 7, 2019

hawkw May 7, 2019

olix0r May 8, 2019

hawkw May 8, 2019


		impl ::std::error::Error for NoEndpoints {}

		pub mod fallback {

Route requests to original dsts when no endpoints exist in the lb #240

Route requests to original dsts when no endpoints exist in the lb #240

Conversation

hawkw commented Apr 30, 2019

hawkw commented Apr 30, 2019

hawkw Apr 30, 2019

Choose a reason for hiding this comment

olix0r May 7, 2019

Choose a reason for hiding this comment

olix0r May 7, 2019

Choose a reason for hiding this comment

hawkw May 7, 2019

Choose a reason for hiding this comment

olix0r May 8, 2019

Choose a reason for hiding this comment

hawkw May 8, 2019

Choose a reason for hiding this comment