Move resolve api to async-stream #599

zaharidichev · 2020-07-21T10:43:59Z

This PR implements the suggestion left by @hawkw right here. The result is that the Resolution struct is now gone and replaces by mpl Stream<Item = Result<resolve::Update<Metadata>, grpc::Status>> that is generates by the try_stream macro

hawkw · 2020-07-21T16:09:49Z

Integration test failure isn't your fault, BTW — still trying to figure out what's up with that test consistently failing on CI.

olix0r · 2020-07-23T21:22:11Z

Does this actually depend on the other PR?

zaharidichev · 2020-07-23T21:24:38Z

@olix0r now that I look at it, I do not think it does, I will change the base

zaharidichev · 2020-07-24T06:40:36Z

@olix0r It does depend because resolve::FromStream was introduced in the other PR

olix0r · 2020-08-18T16:40:04Z

Tests hang

test http1::absolute_uris::outbound_asks_controller_without_orig_dst ... test http1::absolute_uris::outbound_asks_controller_without_orig_dst has been running for over 60 seconds
test http1::absolute_uris::outbound_asks_controller_api ... test http1::absolute_uris::outbound_asks_controller_api has been running for over 60 seconds
test http1::absolute_uris::outbound_destinations_reset_on_reconnect_followed_by_empty ... test http1::absolute_uris::outbound_destinations_reset_on_reconnect_followed_by_empty has been running for over 60 seconds
test http1::absolute_uris::outbound_destinations_reset_on_reconnect_followed_by_dne ... test http1::absolute_uris::outbound_destinations_reset_on_reconnect_followed_by_dne has been running for over 60 seconds
test http1::absolute_uris::outbound_fails_fast_when_destination_does_not_exist ... test http1::absolute_uris::outbound_fails_fast_when_destination_does_not_exist has been running for over 60 seconds
test http1::absolute_uris::outbound_fails_fast_when_destination_has_no_endpoints ... test http1::absolute_uris::outbound_fails_fast_when_destination_has_no_endpoints has been running for over 60 seconds
test http1::absolute_uris::outbound_reconnects_if_controller_stream_ends ... test http1::absolute_uris::outbound_reconnects_if_controller_stream_ends has been running for over 60 seconds
test http1::absolute_uris::remote_header::outbound_should_strip ... test http1::absolute_uris::remote_header::outbound_should_strip has been running for over 60 seconds
test http1::outbound_asks_controller_api ... test http1::outbound_asks_controller_api has been running for over 60 seconds
test http1::outbound_asks_controller_without_orig_dst ... test http1::outbound_asks_controller_without_orig_dst has been running for over 60 seconds
test http1::outbound_destinations_reset_on_reconnect_followed_by_dne ... test http1::outbound_destinations_reset_on_reconnect_followed_by_dne has been running for over 60 seconds
test http1::outbound_destinations_reset_on_reconnect_followed_by_empty ... test http1::outbound_destinations_reset_on_reconnect_followed_by_empty has been running for over 60 seconds

zaharidichev · 2020-08-18T18:18:03Z

@olix0r yes I just saw that. It is strange that simply moving that to async stream caused this hang. Must be doing something wrong. Will dig in

linkerd/proxy/api-resolve/src/resolve.rs

hawkw

Adding the loop appears to change the stream's behavior so that the underlying gRPC stream is polled again even after it returns None. I'm surprised that this fixes a hang --- that implies that the underlying gRPC stream will sometimes return None and then return Some again after being polled again. That's not the behaviour I would have expected.

I'm also surprised that this manifests as a hang --- "falling off" the end of the try_stream! block should result in the stream returning None and terminating. If this is the case, I think that adding the loop will result in the resolve stream returning Poll::Pending rather than Poll::Ready(None) when the underlying stream terminates, and then never being woken up again. If the gRPC stream returns None, then the while loop terminates. The outer loop then loops around and executes the while loop again, polling the (now terminated) gRPC stream, which I guess must return Poll::Pending or something?

I'd like to figure out why returning None here causes a hang. My guess is that some higher-level component that polls the resolve stream doesn't expect it to ever return None? I don't know if leaking streams that will never finish is a great solution.

Of course, it's also possible that my understanding is wrong, or I'm missing something.

linkerd/proxy/api-resolve/src/resolve.rs

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev · 2020-08-20T12:48:51Z

linkerd/proxy/discover/src/make_endpoint.rs

            }

            // Before polling the resolution, where we could potentially receive
            // an `Add`, poll_ready to ensure that `make` is ready to build new
            // services. Don't process any updates until we can do so.
            ready!(this.make_endpoint.poll_ready(cx)).map_err(Into::into)?;

-            if let Some(change) = ready!(this.discover.poll_discover(cx)) {


@hawkw This is where it was hanging.

ahhh, glad you found that!

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

linkerd/proxy/api-resolve/src/resolve.rs

terser imports

hawkw

looks good to me! i commented on a couple minor style nits.

hawkw · 2020-08-20T17:58:01Z

linkerd/proxy/discover/src/make_endpoint.rs

-            if let Some(change) = ready!(this.discover.poll_discover(cx)) {
-                match change.map_err(Into::into)? {
+            match ready!(this.discover.poll_discover(cx)) {
+                Some(change) => match change.map_err(Into::into)? {


Take it or leave it: we could maybe collapse the two matches into a single match, like this:

match ready!(this.discover.poll_discover(cx)) { Some(Ok(Change::Insert(key, target))) => // Start building the service and continue. If a pending // service exists for this addr, it will be canceled. let fut = this.make_endpoint.call(target); this.make_futures.push(key, fut); }, Some(Ok(Change::Remove(key))) => { this.pending_removals.push(key); }, Some(Err(e)) => return Poll::Ready(Err(e.into())), None => return Poll::Ready(Ok(None)), }

which might be a little easier to read...what do you think? Is this clearer?

hawkw · 2020-08-20T18:00:35Z

linkerd/proxy/api-resolve/src/resolve.rs

+                    let update = if exists {
+                        Update::Empty
+                    } else {
+                        Update::DoesNotExist
+                    };
+                    yield update.into();


nit, take it or leave it: can this just be

Suggested change

let update = if exists {

Update::Empty

} else {

Update::DoesNotExist

};

yield update.into();

if exists {

yield Update::Empty.into();

}

yield Update::DoesNotExist.into();

not a blocker.

I might be unclear on the semantics of yield, but this at least seems like it would always yield DNE after yielding Empty?

In either case, the original shows the intent much more clearly imo.

olix0r · 2020-08-20T18:03:22Z

Going to merge this once it's green, but happy to ship fixups afterwards

This release includes internal changes to the service discovery system, especially when discovering control plane components (like the destination and identity controllers). Now, the proxy attempts to balance requests across all pods in each control plane service. This requires control plane changes to use "headless" services so that SRV records are exposed. When the control plane services have a `clusterIP` set, the proxy falls back to using normal A-record lookups. --- * tracing: add richer verbose spans to http clients (linkerd/linkerd2-proxy#622) * trace: update tracing dependencies (linkerd/linkerd2-proxy#623) * Remove `Resolution` trait (linkerd/linkerd2-proxy#606) * Update proxy-identity to edge-20.8.2 (linkerd/linkerd2-proxy#627) * Add build arg for skipping identity wrapper (linkerd/linkerd2-proxy#624) * Wait for proxy thread to terminate in integration tests (linkerd/linkerd2-proxy#625) * Remove scrubbing for unused headers (linkerd/linkerd2-proxy#628) * Split orig-proto tests out of discovery tests (linkerd/linkerd2-proxy#629) * Re-enable outbound timeout test (linkerd/linkerd2-proxy#630) * profiles: perform profile resolution for IP addresses (linkerd/linkerd2-proxy#626) * Move resolve api to async-stream (linkerd/linkerd2-proxy#599) * Decouple discovery buffering from endpoint conversion (linkerd/linkerd2-proxy#631) * resolve: Add a Reset state (linkerd/linkerd2-proxy#633) * resolve: Eagerly fail resolutions (linkerd/linkerd2-proxy#634) * test: replace `net2` dependency with `socket2` (linkerd/linkerd2-proxy#635) * dns: Run DNS resolutions on the main runtime (linkerd/linkerd2-proxy#637) * Load balance requests to the control plane (linkerd/linkerd2-proxy#594) * Unify control plane client construction (linkerd/linkerd2-proxy#638)

zaharidichev requested a review from a team July 21, 2020 10:43

zaharidichev changed the title ~~Move resovle api to async-stream~~ Move resolve api to async-stream Jul 24, 2020

zaharidichev force-pushed the zd/move-resolve-api-to-async-stream branch from ccddb4e to b3ea7ad Compare July 24, 2020 08:11

zaharidichev changed the base branch from zd/control-plane-discover to main August 18, 2020 13:43

zaharidichev force-pushed the zd/move-resolve-api-to-async-stream branch from b3ea7ad to 038733e Compare August 18, 2020 13:43

zaharidichev commented Aug 18, 2020

View reviewed changes

linkerd/proxy/api-resolve/src/resolve.rs Show resolved Hide resolved

hawkw reviewed Aug 18, 2020

View reviewed changes

linkerd/proxy/api-resolve/src/resolve.rs Outdated Show resolved Hide resolved

linkerd/proxy/api-resolve/src/resolve.rs Outdated Show resolved Hide resolved

olix0r reviewed Aug 18, 2020

View reviewed changes

linkerd/proxy/api-resolve/src/resolve.rs Outdated Show resolved Hide resolved

zaharidichev added 2 commits August 20, 2020 09:38

Move resolve api to async stream

d53c974

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

Add a loop around stream polling

f3bb641

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/move-resolve-api-to-async-stream branch 3 times, most recently from a28ec48 to e7ac85e Compare August 20, 2020 12:47

zaharidichev commented Aug 20, 2020

View reviewed changes

Remove loop around poll

8307876

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/move-resolve-api-to-async-stream branch from e7ac85e to 8307876 Compare August 20, 2020 12:49

Adjust type limit

83a54b4

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/move-resolve-api-to-async-stream branch from 9ee1946 to 83a54b4 Compare August 20, 2020 14:40

zaharidichev requested a review from hawkw August 20, 2020 17:33

olix0r approved these changes Aug 20, 2020

View reviewed changes

linkerd/proxy/api-resolve/src/resolve.rs Outdated Show resolved Hide resolved

Update linkerd/proxy/api-resolve/src/resolve.rs

9d6031a

terser imports

hawkw approved these changes Aug 20, 2020

View reviewed changes

olix0r merged commit 591191c into main Aug 20, 2020

olix0r deleted the zd/move-resolve-api-to-async-stream branch August 20, 2020 18:20

olix0r mentioned this pull request Aug 26, 2020

proxy: v2.107.0 linkerd/linkerd2#4917

Merged

Move resolve api to async-stream #599

Move resolve api to async-stream #599

Uh oh!

Conversation

zaharidichev commented Jul 21, 2020

Uh oh!

hawkw commented Jul 21, 2020

Uh oh!

olix0r commented Jul 23, 2020

Uh oh!

zaharidichev commented Jul 23, 2020

Uh oh!

zaharidichev commented Jul 24, 2020

Uh oh!

olix0r commented Aug 18, 2020

Uh oh!

zaharidichev commented Aug 18, 2020

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zaharidichev Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

hawkw Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

hawkw Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

olix0r commented Aug 20, 2020

Uh oh!

Uh oh!