discovery: Fall back only on InvalidArgument, in MakeService by hawkw · Pull Request #268 · linkerd/linkerd2-proxy

hawkw · 2019-06-11T22:21:36Z

When the DNS fallback behavior was removed from the proxy in #259, the
proxy was changed to fall back to original destination routing any
time the control plane indicates that no endpoints exist for a
destination. This is incorrect, as the proxy should always treat the
destination service as authoritative. Additionally, this means that
requests to endpoints which are known to not exist will construct an
unnecessary client service, and eventually fail with a 502 error when
the upstream client connection to the non-existent endpoint ultimately
fails. Instead, the proxy should only fall back when the destination is
outside the search suffixes or the Destination service returns an
InvalidArgument response.

This is a much larger change than #263. In particular, the fallback
behavior has been moved from when the the actual client service is
called to when the client service is constructed, as we do not expect
to recieve an InvalidArgument response on a Destination query that
has previously recieved updates.

This was implemented by changing the Resolve trait to return a
Future whose Item type is a Resolution, rather than returning a
Resolution. When the Destination query returns InvalidArgument, the
future will fail. The proxy::http::fallback middleware has been
changed so that rather than making both the primary and fallback service
and falling back on a per-request basis, it tries to make the primary
service, and falls back if the primary MakeService future fails.

Although this is a large change, I think the resulting code is simpler
and more elegant than the previous approach.

Previously, one of the discovery tests expected the incorrect behavior.
I've changed this test to now expect that destinations known to not
exist do not fall back, and renamed it from
outbound_falls_back_to_orig_dst_when_destination_has_no_endpoints to
outbound_fails_fast_when_destination_has_no_endpoints. I've also
confirmed that the updated test fails against master and passes after
this change.

Fixes linkerd/linkerd2#2880
Closes #263

Signed-off-by: Eliza Weisman [email protected]

Signed-off-by: Eliza Weisman <[email protected]>

(and, don't rely on a specific error status to know what happened) Signed-off-by: Eliza Weisman <[email protected]>

When the Destination service returns a `NoEndpoints` response, a field `exists` is set if the destination _does_ exist in service discovery but has no endpoints. The API spec states that: > `no_endpoints{exists: false}` indicates that the service does not exist > and the client MAY try an alternate service discovery method (e.g. DNS). > > `no_endpoints(exists: true)` indicates that the service does exist and > the client MUST NOT fall back to an alternate service discovery > method. When the DNS fallback behaviour was removed from the proxy in #259, this field was overlooked, and the proxy was changed to fall back to original destination routing _any_ time the control plane indicates that no endpoints exist for a destination. This is incorrect, as the proxy should always treat the destination service as authoritative. Additionally, this means that requests to endpoints which are known to not exist will construct an unnecessary client service, and eventually fail with a 502 error when the upstream client connection to the non-existant endpoint ultimately fails. This branch changes the `control::destination::resolution::Daemon` future to only send `Update::NoEndpoints` (which indicates that the load balancer should fall back) to the load balancer when the Destination service response has `exists: false`, or on `InvalidArgument` errors, and _not_ when a `no_endpoints{exists: true}` response is recieved. These requests will now fail fast with a 503 error rather than attempting an ultimately futile fallback request. Previously, one of the discovery tests expected the incorrect behaviour. I've changed this test to now expect that destinations known to not exist do _not_ fall back, and renamed it from `outbound_falls_back_to_orig_dst_when_destination_has_no_endpoints` to `outbound_fails_fast_when_destination_has_no_endpoints`. I've also confirmed that the updated test fails against master and passes after this change. Fixes linkerd/linkerd2#2880 Signed-off-by: Eliza Weisman <[email protected]>

Signed-off-by: Eliza Weisman <[email protected]>

olix0r

This looks really good. I'm going to give it a deeper read; but in the meantime it would be good to get this under some test load for a soak.

src/app/main.rs

Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2019-06-11T23:58:20Z

@olix0r I've kicked off a lifecycle test to run overnight. Will report back.

olix0r

This looks great!

olix0r · 2019-06-12T18:31:58Z

@hawkw How does this interact with suffix matching? Do we throw an unresolveable error when the suffix doesn't match?

hawkw · 2019-06-12T18:36:07Z

@hawkw How does this interact with suffix matching? Do we throw an unresolveable error when the suffix doesn't match?

Yeah, that's right. If the requested name isn't in the search suffixes, Resolver::resolve returns ResolveFuture::unresolvable:

linkerd2-proxy/src/control/destination/mod.rs

Lines 116 to 125 in b5964b5

    
           if self.suffixes.iter().any(|s| s.contains(authority.name())) { 
        
               if let Some(client) = self.client.as_ref().cloned() { 
        
                   return ResolveFuture::new(authority, client); 
        
               } else { 
        
                   trace!("-> control plane client disabled"); 
        
               } 
        
           } else { 
        
               trace!("-> authority {} not in search suffixes", authority); 
        
           } 
        
           ResolveFuture::unresolvable()

ResolveFuture::unresolvable returns a ResolveFuture with the query field set to None.

linkerd2-proxy/src/control/destination/resolution.rs

Lines 111 to 114 in b5964b5

    
               pub(super) fn unresolvable() -> Self { 
        
                   Self { query: None } 
        
               } 
        
           }

The ResolveFuture won't query the control plane, doesn't spawn a background daemon task, and will fail immediately with Unresolvable when polled:

linkerd2-proxy/src/control/destination/resolution.rs

Lines 147 to 150 in 29f8e68

    
           None => { 
        
               trace!("name is unresolvable"); 
        
               return Err(Unresolvable { _p: () }); 
        
           }

the log line tried to format the value of `self.query.authority()` in the match arm where `self.query` was `None`. this would panic. also, it logged that the name was not in search suffixes, which MAY be the case, but `ResolveFuture::unresolvable` might also be returned when the controller client is disabled. Signed-off-by: Eliza Weisman <[email protected]>

Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2019-06-12T19:58:46Z

I deployed a burn-in test overnight, and everything seems fine. I'm going to go ahead and merge this if there are no objections?

439fbfed Update to rust-1.35.0 (linkerd/linkerd2-proxy#265) db26495e Honor `l5d-override-dst` for inbound service profiles (linkerd/linkerd2-proxy#267) a476e995 metrics: Include the prefix of a Report in log lines (linkerd/linkerd2-proxy#262) 1a52a5e6 discovery: Fall back in MakeService, only on InvalidArgument (linkerd/linkerd2-proxy#268) 35df8ab4 metrics: Classify response errors (linkerd/linkerd2-proxy#269)

hawkw added 11 commits June 2, 2019 11:08

fix wrong test for dsts which exist with no eps

af26ee0

Signed-off-by: Eliza Weisman <[email protected]>

make test panic a little clearer

086387d

(and, don't rely on a specific error status to know what happened) Signed-off-by: Eliza Weisman <[email protected]>

destination: fall back only on InvalidArgument

f20846c

Signed-off-by: Eliza Weisman <[email protected]>

Fix incorrect assertion in reset-on-reconnect test

acffbbc

Signed-off-by: Eliza Weisman <[email protected]>

rename does_not_exist -> unresolvable

fe9d612

Signed-off-by: Eliza Weisman <[email protected]>

wip

4d6a6c4

Signed-off-by: Eliza Weisman <[email protected]>

move fallback to the MakeService level

a5af556

Signed-off-by: Eliza Weisman <[email protected]>

rm old stuff

027d5cd

Signed-off-by: Eliza Weisman <[email protected]>

missing muts

f9ad1a5

Signed-off-by: Eliza Weisman <[email protected]>

fixup/docs

82937ad

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested review from kleimkuhler and olix0r June 11, 2019 22:21

hawkw self-assigned this Jun 11, 2019

This was referenced Jun 11, 2019

discovery: Only fall back on InvalidArgument #263

Closed

Use a constant-time load balancer #266

Merged

olix0r reviewed Jun 11, 2019

View reviewed changes

src/app/main.rs Outdated Show resolved Hide resolved

combinatorify fallback

190bfe8

Signed-off-by: Eliza Weisman <[email protected]>

olix0r approved these changes Jun 12, 2019

View reviewed changes

hawkw added 2 commits June 12, 2019 11:20

add/update comments

20d089e

cleanup

b5964b5

hawkw added 2 commits June 12, 2019 11:41

rustfmt

6872df8

Signed-off-by: Eliza Weisman <[email protected]>

hawkw mentioned this pull request Jun 12, 2019

High latency when host header is not present linkerd/linkerd2#2397

Closed

hawkw merged commit 1a52a5e into master Jun 12, 2019

hawkw deleted the liza/invalid-argument-fails-faster branch June 12, 2019 22:04

olix0r mentioned this pull request Jun 13, 2019

proxy: update to linkerd/linkerd2-proxy#35df8ab linkerd/linkerd2#2939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discovery: Fall back only on InvalidArgument, in MakeService#268

discovery: Fall back only on InvalidArgument, in MakeService#268
hawkw merged 16 commits intomasterfrom
liza/invalid-argument-fails-faster

hawkw commented Jun 11, 2019

Uh oh!

olix0r left a comment

Uh oh!

Uh oh!

hawkw commented Jun 11, 2019

Uh oh!

olix0r left a comment

Uh oh!

olix0r commented Jun 12, 2019

Uh oh!

hawkw commented Jun 12, 2019 •

edited

Loading

Uh oh!

hawkw commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hawkw commented Jun 11, 2019

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw commented Jun 11, 2019

Uh oh!

olix0r left a comment

Choose a reason for hiding this comment

Uh oh!

olix0r commented Jun 12, 2019

Uh oh!

hawkw commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkw commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hawkw commented Jun 12, 2019 •

edited

Loading